| Message ID | 5ff3ceab3d3a547ab23144adbfa2000f1604c39f.1620252768.git.sbrivio@redhat.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | Fix display of < 64 bits IPv6 masks in concatenated elements | expand |
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") > 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? Cheers, Phil
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.
On Thu, May 06, 2021 at 12:00:21PM +0200, Stefano Brivio wrote: > 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: [...] > > > 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. OK, so this patch not just fixes a bug but also streamlines the code for performance (and something with refactoring, too). ;) Cheers, Phil
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); while (mpz_cmp(tmp_start, tmp_end) <= 0 && !mpz_tstbit(tmp_start, 0) && mpz_tstbit(tmp_end, 0) &&
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. 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(-)