Re: [PATCH] iptables-restore: move code to add_param_to_argv, cleanup (fix gcc-4.7)

[Pablo Neira Ayuso <pablo@netfilter.org>]

[-- Attachment #1: Type: text/plain, Size: 2192 bytes --]

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).

[-- Attachment #2: 0001-iptables-restore-fix-memory-corruption-in-parameter-.patch --]
[-- Type: text/x-diff, Size: 2836 bytes --]

>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