Hi,
On Fri, Oct 18, 2019 at 10:30:56AM +0200, Pablo Neira Ayuso wrote:
> On Fri, Oct 18, 2019 at 12:48:34AM +0200, Phil Sutter wrote:
> > This was overlooked when merging argv-related code: newargc is
> > initialized at declaration and reset in free_argv() again.
> >
> > Fixes: a2ed880a19d08 ("xshared: Consolidate argv construction routines")
> > Signed-off-by: Phil Sutter <phil@xxxxxx>
> > ---
> > iptables/xtables-restore.c | 3 ---
> > 1 file changed, 3 deletions(-)
> >
> > diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
> > index df8844208c273..bb6ee78933f7a 100644
> > --- a/iptables/xtables-restore.c
> > +++ b/iptables/xtables-restore.c
> > @@ -232,9 +232,6 @@ void xtables_restore_parse(struct nft_handle *h,
> > char *bcnt = NULL;
> > char *parsestart = buffer;
> >
> > - /* reset the newargv */
> > - newargc = 0;
>
> Are you sure this is correct? This resets the variable for each table
> this is entering.
In fact, the removed line resets newargc before parsing each rule line.
But since newargc is initially zero and after each call to do_command a
call to free_argv() happens which resets newargc again, we're really
save here.
> BTW, newargv, newargc are defined as globals which is very hard to
> follow when reading this code. Probably place them in a structure
> definition and pass them to functions to make easier to follow track
> of this code?
Good point, I'll do that.
> That code would qualify for placing it under
> iptables/xtables-restore.c since it is common for the xml and the
> native parser as I suggested before.
These global variables and related functions currently reside in
xshared.c which is the right spot. :)
Thanks, Phil
