Re: [PATCH iptables] extensions: libebt_stp: fix range checking

[Phil Sutter <phil@nwl.cc>]

On Wed, Jan 24, 2024 at 05:13:45PM +0100, Phil Sutter wrote:
> On Wed, Jan 24, 2024 at 04:42:16PM +0100, Florian Westphal wrote:
> > Phil Sutter <phil@nwl.cc> wrote:
> > > While you correctly hate the game instead of its player, you probably
> > > hate the wrong game: The code above indeed is confusing. Maybe one
> > > should move that monotonicity check into libxtables which should
> > > simplify it quite a bit. I'll have a look. :)
> > 
> > Something IS broken.  Still not working on FC 39 test machine
> > even after fresh clone.
> > 
> > On a "working" VM:
> > export XTABLES_LIBDIR=$(pwd)/extensions
> > iptables/xtables-nft-multi ebtables -A INPUT --stp-root-cost 1
> > have 1 32765
> > 
> > @@ -150,7 +151,9 @@ static void brstp_parse(struct xt_option_call *cb)
> >                 RANGE_ASSIGN("root-prio", root_prio, cb->val.u16_range);
> >                 break;
> >         case O_RCOST:
> > +               fprintf(stderr, "have %u %u\n", cb->val.u32_range[0], cb->val.u32_range[1]);
> > 
> > I can't even figure out where the correct max value is supposed to be set.
> > 
> > Varying the input:
> > 
> > xtables-nft-multi ebtables -A INPUT --stp-root-cost 1
> > have 1 32764
> > 
> > Looks to me as if the upper value is undefined.
> > 
> > Other users of *RC versions handle it in .parse, e.g. libxt_length.
> > No idea how this is working.
> 
> In xtopt_parse_mint(), there is:
> 
> | const uintmax_t lmax = xtopt_max_by_type(entry->type);
> | [...]
> | if (*arg == '\0' || *arg == sep) {
> |         /* Default range components when field not spec'd. */
> |         end = (char *)arg;
> |         value = (cb->nvals == 1) ? lmax : 0;
> 
> But that branch appears to be dead code. So this is indeed a bug and a
> specific build may or may not hit it as your experience shows. I'll see
> how xtopt_parse_mint() can be fixed.

The big elucidation was the code is called only for ranges and somehow I
managed to miss the point that your sample command doesn't contain a
range in the first place.

So while I still think it makes sense to have the 'low <= high' check
done by the parser, I applied your patch for now as it indeed fixes that
bug in libebt_stp extension parser. Sorry for all the confusion I must
have caused. :(

Meanwhile I've added test cases for ranges in various formats which
uncovered quite a few things to fix.

Thanks, Phil