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

[Phil Sutter <phil@nwl.cc>]

Hi,

On Fri, Oct 18, 2019 at 10:58:08PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > > 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. ;)
> 
> Fair enough, my main point was where the test cases come from, i.e.
> did you see such rule dumps in the wild, or did you create this manually
> to catch all corner cases?
> 
> I see you have a test for things like "-?t", so I wondered where that
> came from.

Ah! Originally this comes from a Red Hat BZ, not sure what reporter
actually tested with but as long as the comment started with a dash and
contained a 't' somewhere it would trigger the bug.

I wrote the test case along with the new implementation, searching for
things that could be mismatched. The '-?t' for instance is to make sure
combined short-options are matched correctly: Since '?' is not a valid
short option (at least not in iptables), this must not match as '-t'
option.

> > What do you think? Or should I respin after adding a bunch of comments
> > to is_table_param() to make it more clear?
> 
> I think thats the best option, I don't have any objections at the check
> per se given older iptables does this too.

I don't quite like this check, hence I don't overly cling to it. As you
see, checking for presence of an option in getopt() format is not easy
and we do that for every option of every rule in a dump. Maybe we should
really just append the explicit table param and accept that user's table
option is not rejected but simply ignored.

Cheers, Phil