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

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

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.