Re: [iptables PATCH] xtables-restore: Fix --table parameter check

[Phil Sutter <phil@nwl.cc>]

Hi Florian,

On Fri, Oct 18, 2019 at 04:05:08PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > Xtables-restore tries to reject rule commands in input which contain a
> > --table parameter (since it is adding this itself based on the previous
> > table line). Sadly getopt_long's flexibility makes it hard to get this
> > check right: Since the last fix, comments starting with a dash and
> > containing a 't' character somewhere later were rejected. Simple
> > example:
> > 
> > | *filter
> > | -A FORWARD -m comment --comment "- allow this one" -j ACCEPT
> > | COMMIT
> > 
> > To hopefully sort this once and for all, introduce is_table_param()
> > which should cover all possible variants of legal and illegal
> > parameters. Also add a test to make sure it does what it is supposed to.
> 
> Thanks for adding a test for this.
> How did you generate it?  The added code is pure voodoo magic to me,
> so I wonder if we can just remove the 'test for -t in iptables-restore
> files' code.

Sorry, I didn't mean to create such unreadable code. I guess after
managing to wrap my head around to understand the old code, the new one
seemed much more clear to me. ;)

The problem with dropping that check is the potential mess we get when
users add '-t' parameter to rules in dumps. While *tables-restore adds
'-t' option for the current table itself, arg parsing in at least
do_commandeb and do_commandx accepts multiple '-t' options (so the last
one wins).

Assuming that this checking for '-t' presence is a mess and we should
get rid of it, I can imagine two alternatives:

1) Disallow multiple '-t' options. A nice and easy solution, but not
   backwards compatible.
2) Make *tables-restore add the '-t' option last. This is a bit of a
   hack and will cause unexpected behaviour for users trying to add '-t'
   option in dumps.

What do you think? Or should I respin after adding a bunch of comments
to is_table_param() to make it more clear?

Thanks, Phil