diff mbox

iptables-restore: move code to add_param_to_argv, cleanup (fix gcc-4.7)

Message ID 20120730013707.GA2350@1984
State Not Applicable
Headers show

Commit Message

Pablo Neira Ayuso July 30, 2012, 1:37 a.m. UTC 
On Mon, Jul 23, 2012 at 01:38:48PM +0200, Eric Dumazet wrote:
> On Mon, 2012-07-23 at 13:29 +0200, Eric Dumazet wrote:
> > On Mon, 2012-07-23 at 12:38 +0200, pablo@netfilter.org wrote:
> > > From: Pablo Neira Ayuso <pablo@netfilter.org>
> > > 
> > > This patch seems to be a mere cleanup that moves the parameter parsing
> > > code to add_param_to_argv.
> > > 
> > > But, in reality, it also fixes iptables whe compiled with gcc-4.7.
> > > 
> > > Moving param_buffer declaration out of the loop seems to resolve the
> > > issue. gcc-4.7 seems to be generating bad code regarding param_buffer.
> > > 
> > > @@ -380,9 +380,9 @@
> > >                         quote_open = 0;
> > >                         escaped = 0;
> > >                         param_len = 0;
> > > +                       char param_buffer[1024];
> > > 
> > >                         for (curchar = parsestart; *curchar; curchar++) {
> > > -                               char param_buffer[1024];
> > > 
> > >                                 if (quote_open) {
> > >                                         if (escaped) {
> > > 
> > > But I have hard time to apply this patch in such a way. Instead, I came
> > > up with the idea of this cleanup, which does not harm after all (and fixes
> > > the issue for us).
> > > 
> > > Sorry, I didn't have the time to further debug this issue, but it would be
> > > worth to investigate what's going wrong and ping gcc people.
> > 
> > Bug seems that iptables forgot that "char param_buffer[1024];" can
> > disappear at the end of the block :
> > 
> > for (curchar = parsestart; *curchar; curchar++) {
> > 	char param_buffer[1024];
> > 	...
> > }
> > 
> > // here param_buffer[1024] is lost, so any var pointing 
> > // to it can mess stack
> > 
> > previous gcc were probably not so aggressive.
> 
> Oh well, add_argv() does a strdup(), so iptables code seems fine.

I thought the same, but one contributor has put some on light on this.

I'm going to revert the patch that I applied to fix this and apply
the one that comes with this email instead.

It contains a simple description of the problem, I think it's good for
the record (distro maintainers will likely google for this).

Comments

Pablo Neira Ayuso July 30, 2012, 1:40 a.m. UTC | #1 
On Mon, Jul 30, 2012 at 03:37:07AM +0200, Pablo Neira Ayuso wrote:
> From 8cfcb0137f4aee880ec749cd2356148325f6085d Mon Sep 17 00:00:00 2001
> From: Pablo Neira Ayuso <pablo@netfilter.org>
> Date: Mon, 30 Jul 2012 03:08:51 +0200
> Subject: [PATCH] iptables-restore: fix memory corruption in parameter parsing
>  with gcc-4.7

I'm going to change the title of this patch, memory corruption does
not seem appropriate. Instead, the problem seems to be related to
wrong assumptions on uninitialized memory area.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jan Engelhardt Aug. 2, 2012, 7:37 p.m. UTC | #2 
On Monday 2012-07-30 03:37, Pablo Neira Ayuso wrote:
>> > // here param_buffer[1024] is lost, so any var pointing 
>> > // to it can mess stack
>> > 
>> > previous gcc were probably not so aggressive.
>> 
>> Oh well, add_argv() does a strdup(), so iptables code seems fine.
>
>I thought the same, but one contributor has put some on light on this.
>
>I'm going to revert the patch that I applied to fix this and apply
>the one that comes with this email instead.
>
>It contains a simple description of the problem, I think it's good for
>the record (distro maintainers will likely google for this).

Your code cleanup, by moving the code into a separate function,
is however still desired :)
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso Aug. 3, 2012, 9:15 a.m. UTC | #3 
On Thu, Aug 02, 2012 at 09:37:10PM +0200, Jan Engelhardt wrote:
> 
> On Monday 2012-07-30 03:37, Pablo Neira Ayuso wrote:
> >> > // here param_buffer[1024] is lost, so any var pointing 
> >> > // to it can mess stack
> >> > 
> >> > previous gcc were probably not so aggressive.
> >> 
> >> Oh well, add_argv() does a strdup(), so iptables code seems fine.
> >
> >I thought the same, but one contributor has put some on light on this.
> >
> >I'm going to revert the patch that I applied to fix this and apply
> >the one that comes with this email instead.
> >
> >It contains a simple description of the problem, I think it's good for
> >the record (distro maintainers will likely google for this).
> 
> Your code cleanup, by moving the code into a separate function,
> is however still desired :)

OK, it's back:

http://git.netfilter.org/cgi-bin/gitweb.cgi?p=iptables.git;a=commit;h=23a98b56935c42ef460020e37a9ff8006eee58e2
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

From 8cfcb0137f4aee880ec749cd2356148325f6085d Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Mon, 30 Jul 2012 03:08:51 +0200
Subject: [PATCH] iptables-restore: fix memory corruption in parameter parsing
 with gcc-4.7

This patch fixes a memory corruption that has been in iptables-restore since
time ago. The problem has shown up with gcc-4.7. This version of gcc seem to
perform more agressive memory management than previous.

Peter Lekensteyn provided the following sample code similar to the one
in iptables-restore:

int i = 0;

for (;;) {
	char x[5];

	x[i] = '0' + i;
	if (++i == 4) {
		x[i] = '\0'; /* terminate string with null byte */
		printf("%s\n", x);
		break;
	}
}

Many may expect 0123 as output. But GCC 4.7 does not do that when compiling
with optimization enabled (-O1 and higher). It instead puts random data in the
first bytes of the character array, which becomes:

| 0  |  1  |  2  |  3  |  4   |
|     RANDOM     | '3' | '\0' |

Since the array is declared inside the scope of loop's body, you can think of
it as of a new array being allocated in the automatic storage area for each
loop iteration.

The correct code should be:

char x[5];

for (;;) {
	x[i] = '0' + i;
	if (++i == 4) {
		x[i] = '\0'; /* terminate string with null byte */
		printf("%s\n", x);
		break;
	}
}

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 iptables/ip6tables-restore.c |    3 +--
 iptables/iptables-restore.c  |    3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/iptables/ip6tables-restore.c b/iptables/ip6tables-restore.c
index 3894d68..1ec3dd9 100644
--- a/iptables/ip6tables-restore.c
+++ b/iptables/ip6tables-restore.c
@@ -329,6 +329,7 @@  int ip6tables_restore_main(int argc, char *argv[])
 			char *curchar;
 			int quote_open, escaped;
 			size_t param_len;
+			char param_buffer[1024];
 
 			/* reset the newargv */
 			newargc = 0;
@@ -379,8 +380,6 @@  int ip6tables_restore_main(int argc, char *argv[])
 			param_len = 0;
 
 			for (curchar = parsestart; *curchar; curchar++) {
-				char param_buffer[1024];
-
 				if (quote_open) {
 					if (escaped) {
 						param_buffer[param_len++] = *curchar;
diff --git a/iptables/iptables-restore.c b/iptables/iptables-restore.c
index 034f960..9f51f99 100644
--- a/iptables/iptables-restore.c
+++ b/iptables/iptables-restore.c
@@ -329,6 +329,7 @@  iptables_restore_main(int argc, char *argv[])
 			char *curchar;
 			int quote_open, escaped;
 			size_t param_len;
+			char param_buffer[1024];
 
 			/* reset the newargv */
 			newargc = 0;
@@ -379,8 +380,6 @@  iptables_restore_main(int argc, char *argv[])
 			param_len = 0;
 
 			for (curchar = parsestart; *curchar; curchar++) {
-				char param_buffer[1024];
-
 				if (quote_open) {
 					if (escaped) {
 						param_buffer[param_len++] = *curchar;
-- 
1.7.10.4