diff mbox

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

Message ID 1343039903-7230-1-git-send-email-pablo@netfilter.org
State Rejected
Headers show

Commit Message

Pablo Neira Ayuso July 23, 2012, 10:38 a.m. UTC 
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.

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

Comments

Eric Dumazet July 23, 2012, 11:29 a.m. UTC | #1 
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.



--
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
Eric Dumazet July 23, 2012, 11:38 a.m. UTC | #2 
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.



--
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 July 23, 2012, 12:19 p.m. UTC | #3 
On Monday 2012-07-23 12:38, 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.

How does the problem manifest?
--
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 July 23, 2012, 1:04 p.m. UTC | #4 
On Mon, Jul 23, 2012 at 02:19:11PM +0200, Jan Engelhardt wrote:
> On Monday 2012-07-23 12:38, 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.
> 
> How does the problem manifest?

You can check:

http://bugzilla.netfilter.org/show_bug.cgi?id=774

It tastes like a memory corruption.
--
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 July 23, 2012, 1:08 p.m. UTC | #5 
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 think so, I don't find any possible dereference to param_buffer out
of that loop and it does strdup accordingly.
--
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

diff --git a/iptables/ip6tables-restore.c b/iptables/ip6tables-restore.c
index 3894d68..9a03dff 100644
--- a/iptables/ip6tables-restore.c
+++ b/iptables/ip6tables-restore.c
@@ -114,6 +114,70 @@  static void free_argv(void) {
 		free(newargv[i]);
 }
 
+static void add_param_to_argv(char *parsestart)
+{
+	int quote_open = 0, escaped = 0, param_len = 0;
+	char param_buffer[1024], *curchar;
+
+	/* After fighting with strtok enough, here's now
+	 * a 'real' parser. According to Rusty I'm now no
+	 * longer a real hacker, but I can live with that */
+
+	for (curchar = parsestart; *curchar; curchar++) {
+		if (quote_open) {
+			if (escaped) {
+				param_buffer[param_len++] = *curchar;
+				escaped = 0;
+				continue;
+			} else if (*curchar == '\\') {
+				escaped = 1;
+				continue;
+			} else if (*curchar == '"') {
+				quote_open = 0;
+				*curchar = ' ';
+			} else {
+				param_buffer[param_len++] = *curchar;
+				continue;
+			}
+		} else {
+			if (*curchar == '"') {
+				quote_open = 1;
+				continue;
+			}
+		}
+
+		if (*curchar == ' '
+		    || *curchar == '\t'
+		    || * curchar == '\n') {
+			if (!param_len) {
+				/* two spaces? */
+				continue;
+			}
+
+			param_buffer[param_len] = '\0';
+
+			/* check if table name specified */
+			if (!strncmp(param_buffer, "-t", 2)
+                            || !strncmp(param_buffer, "--table", 8)) {
+				xtables_error(PARAMETER_PROBLEM,
+				   "Line %u seems to have a "
+				   "-t table option.\n", line);
+				exit(1);
+			}
+
+			add_argv(param_buffer);
+			param_len = 0;
+		} else {
+			/* regular character, copy to buffer */
+			param_buffer[param_len++] = *curchar;
+
+			if (param_len >= sizeof(param_buffer))
+				xtables_error(PARAMETER_PROBLEM,
+				   "Parameter too long!");
+		}
+	}
+}
+
 int ip6tables_restore_main(int argc, char *argv[])
 {
 	struct xtc_handle *handle = NULL;
@@ -325,11 +389,6 @@  int ip6tables_restore_main(int argc, char *argv[])
 			char *bcnt = NULL;
 			char *parsestart;
 
-			/* the parser */
-			char *curchar;
-			int quote_open, escaped;
-			size_t param_len;
-
 			/* reset the newargv */
 			newargc = 0;
 
@@ -370,69 +429,7 @@  int ip6tables_restore_main(int argc, char *argv[])
 				add_argv((char *) bcnt);
 			}
 
-			/* After fighting with strtok enough, here's now
-			 * a 'real' parser. According to Rusty I'm now no
-			 * longer a real hacker, but I can live with that */
-
-			quote_open = 0;
-			escaped = 0;
-			param_len = 0;
-
-			for (curchar = parsestart; *curchar; curchar++) {
-				char param_buffer[1024];
-
-				if (quote_open) {
-					if (escaped) {
-						param_buffer[param_len++] = *curchar;
-						escaped = 0;
-						continue;
-					} else if (*curchar == '\\') {
-						escaped = 1;
-						continue;
-					} else if (*curchar == '"') {
-						quote_open = 0;
-						*curchar = ' ';
-					} else {
-						param_buffer[param_len++] = *curchar;
-						continue;
-					}
-				} else {
-					if (*curchar == '"') {
-						quote_open = 1;
-						continue;
-					}
-				}
-
-				if (*curchar == ' '
-				    || *curchar == '\t'
-				    || * curchar == '\n') {
-					if (!param_len) {
-						/* two spaces? */
-						continue;
-					}
-
-					param_buffer[param_len] = '\0';
-
-					/* check if table name specified */
-					if (!strncmp(param_buffer, "-t", 2)
-                                            || !strncmp(param_buffer, "--table", 8)) {
-						xtables_error(PARAMETER_PROBLEM,
-						   "Line %u seems to have a "
-						   "-t table option.\n", line);
-						exit(1);
-					}
-
-					add_argv(param_buffer);
-					param_len = 0;
-				} else {
-					/* regular character, copy to buffer */
-					param_buffer[param_len++] = *curchar;
-
-					if (param_len >= sizeof(param_buffer))
-						xtables_error(PARAMETER_PROBLEM,
-						   "Parameter too long!");
-				}
-			}
+			add_param_to_argv(parsestart);
 
 			DEBUGP("calling do_command6(%u, argv, &%s, handle):\n",
 				newargc, curtable);
diff --git a/iptables/iptables-restore.c b/iptables/iptables-restore.c
index 034f960..c974cb3 100644
--- a/iptables/iptables-restore.c
+++ b/iptables/iptables-restore.c
@@ -113,6 +113,70 @@  static void free_argv(void) {
 		free(newargv[i]);
 }
 
+static void add_param_to_argv(char *parsestart)
+{
+	int quote_open = 0, escaped = 0, param_len = 0;
+	char param_buffer[1024], *curchar;
+
+	/* After fighting with strtok enough, here's now
+	 * a 'real' parser. According to Rusty I'm now no
+	 * longer a real hacker, but I can live with that */
+
+	for (curchar = parsestart; *curchar; curchar++) {
+		if (quote_open) {
+			if (escaped) {
+				param_buffer[param_len++] = *curchar;
+				escaped = 0;
+				continue;
+			} else if (*curchar == '\\') {
+				escaped = 1;
+				continue;
+			} else if (*curchar == '"') {
+				quote_open = 0;
+				*curchar = ' ';
+			} else {
+				param_buffer[param_len++] = *curchar;
+				continue;
+			}
+		} else {
+			if (*curchar == '"') {
+				quote_open = 1;
+				continue;
+			}
+		}
+
+		if (*curchar == ' '
+		    || *curchar == '\t'
+		    || * curchar == '\n') {
+			if (!param_len) {
+				/* two spaces? */
+				continue;
+			}
+
+			param_buffer[param_len] = '\0';
+
+			/* check if table name specified */
+			if (!strncmp(param_buffer, "-t", 2)
+			    || !strncmp(param_buffer, "--table", 8)) {
+				xtables_error(PARAMETER_PROBLEM,
+				   "Line %u seems to have a "
+				   "-t table option.\n", line);
+				exit(1);
+			}
+
+			add_argv(param_buffer);
+			param_len = 0;
+		} else {
+			/* regular character, copy to buffer */
+			param_buffer[param_len++] = *curchar;
+
+			if (param_len >= sizeof(param_buffer))
+				xtables_error(PARAMETER_PROBLEM,
+				   "Parameter too long!");
+		}
+	}
+}
+
 int
 iptables_restore_main(int argc, char *argv[])
 {
@@ -325,11 +389,6 @@  iptables_restore_main(int argc, char *argv[])
 			char *bcnt = NULL;
 			char *parsestart;
 
-			/* the parser */
-			char *curchar;
-			int quote_open, escaped;
-			size_t param_len;
-
 			/* reset the newargv */
 			newargc = 0;
 
@@ -370,69 +429,7 @@  iptables_restore_main(int argc, char *argv[])
 				add_argv((char *) bcnt);
 			}
 
-			/* After fighting with strtok enough, here's now
-			 * a 'real' parser. According to Rusty I'm now no
-			 * longer a real hacker, but I can live with that */
-
-			quote_open = 0;
-			escaped = 0;
-			param_len = 0;
-
-			for (curchar = parsestart; *curchar; curchar++) {
-				char param_buffer[1024];
-
-				if (quote_open) {
-					if (escaped) {
-						param_buffer[param_len++] = *curchar;
-						escaped = 0;
-						continue;
-					} else if (*curchar == '\\') {
-						escaped = 1;
-						continue;
-					} else if (*curchar == '"') {
-						quote_open = 0;
-						*curchar = ' ';
-					} else {
-						param_buffer[param_len++] = *curchar;
-						continue;
-					}
-				} else {
-					if (*curchar == '"') {
-						quote_open = 1;
-						continue;
-					}
-				}
-
-				if (*curchar == ' '
-				    || *curchar == '\t'
-				    || * curchar == '\n') {
-					if (!param_len) {
-						/* two spaces? */
-						continue;
-					}
-
-					param_buffer[param_len] = '\0';
-
-					/* check if table name specified */
-					if (!strncmp(param_buffer, "-t", 2)
-					    || !strncmp(param_buffer, "--table", 8)) {
-						xtables_error(PARAMETER_PROBLEM,
-						   "Line %u seems to have a "
-						   "-t table option.\n", line);
-						exit(1);
-					}
-
-					add_argv(param_buffer);
-					param_len = 0;
-				} else {
-					/* regular character, copy to buffer */
-					param_buffer[param_len++] = *curchar;
-
-					if (param_len >= sizeof(param_buffer))
-						xtables_error(PARAMETER_PROBLEM,
-						   "Parameter too long!");
-				}
-			}
+			add_param_to_argv(parsestart);
 
 			DEBUGP("calling do_command4(%u, argv, &%s, handle):\n",
 				newargc, curtable);