Re: [PATCH nft 1/2] segtree: Fix range_mask_len() for subnet ranges exceeding unsigned int

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 6 May 2021 11:18:14 +0200
Phil Sutter <phil@nwl.cc> wrote:

> On Thu, May 06, 2021 at 12:23:13AM +0200, Stefano Brivio wrote:
> > As concatenated ranges are fetched from kernel sets and displayed to
> > the user, range_mask_len() evaluates whether the range is suitable for
> > display as netmask, and in that case it calculates the mask length by
> > right-shifting the endpoints until no set bits are left, but in the
> > existing version the temporary copies of the endpoints are derived by
> > copying their unsigned int representation, which doesn't suffice for
> > IPv6 netmask lengths, in general.
> > 
> > PetrB reports that, after inserting a /56 subnet in a concatenated set
> > element, it's listed as a /64 range. In fact, this happens for any
> > IPv6 mask shorter than 64 bits.
> > 
> > Fix this issue by simply sourcing the range endpoints provided by the
> > caller and setting the temporary copies with mpz_init_set(), instead
> > of fetching the unsigned int representation. The issue only affects
> > displaying of the masks, setting elements already works as expected.
> 
> Fixes: 8ac2f3b2fca38 ("src: Add support for concatenated set ranges")

Thanks Phil! I even looked it up and forgot to paste it ;)

> > Reported-by: PetrB <petr.boltik@gmail.com>
> > Bugzilla: https://bugzilla.netfilter.org/show_bug.cgi?id=1520
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > ---
> >  src/segtree.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/segtree.c b/src/segtree.c
> > index ad199355532e..353a0053ebc0 100644
> > --- a/src/segtree.c
> > +++ b/src/segtree.c
> > @@ -838,8 +838,8 @@ static int range_mask_len(const mpz_t start, const mpz_t end, unsigned int len)
> >  	mpz_t tmp_start, tmp_end;
> >  	int ret;
> >  
> > -	mpz_init_set_ui(tmp_start, mpz_get_ui(start));
> > -	mpz_init_set_ui(tmp_end, mpz_get_ui(end));
> > +	mpz_init_set(tmp_start, start);
> > +	mpz_init_set(tmp_end, end);  
> 
> The old code is a bit funny, was there a specific reason why you
> exported the values into a C variable intermediately?

Laziness, ultimately: I didn't remember the name of gmp_printf(),
didn't look it up, and used a fprintf() instead to check 'start' and
'end'... and then whoops, I left the mpz_get_ui() calls there.

-- 
Stefano