Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote: > On Wed, Mar 05, 2025 at 04:01:48PM +0100, Florian Westphal wrote: > > This uses the wrong length. This must re-use the length of the datatype, > > not the string length. > > > > The added test cases will fail without the fix due to erroneous > > overlap detection, which in itself is due to incorrect sorting of > > the elements. > > > > Example error: > > netlink: Error: interval overlaps with an existing one > > add element inet testifsets simple_wild { "2-1" } failed. > > table inet testifsets { > > ... elements = { "1-1", "abcdef*", "othername", "ppp0" } > > > > ... but clearly "2-1" doesn't overlap with any existing members. > > The false detection is because of the "acvdef*" wildcard getting sorted > > at the beginning of the list which is because its erronously initialised > > as a 64bit number instead of 128 bits (16 bytes / IFNAMSIZ). > > One question here. > > > Fixes: 5e393ea1fc0a ("segtree: add string "range" reversal support") > > Signed-off-by: Florian Westphal <fw@xxxxxxxxx> > > --- > > src/segtree.c | 2 +- > > tests/shell/testcases/sets/sets_with_ifnames | 62 ++++++++++++++++++++ > > 2 files changed, 63 insertions(+), 1 deletion(-) > > > > diff --git a/src/segtree.c b/src/segtree.c > > index 2e32a3291979..11cf27c55dcb 100644 > > --- a/src/segtree.c > > +++ b/src/segtree.c > > @@ -471,7 +471,7 @@ static struct expr *interval_to_string(struct expr *low, struct expr *i, const m > > > > expr = constant_expr_alloc(&low->location, low->dtype, > > BYTEORDER_HOST_ENDIAN, > > - (str_len + 1) * BITS_PER_BYTE, data); > > + len * BITS_PER_BYTE, data); > > BTW, is this also needed? > > diff --git a/src/segtree.c b/src/segtree.c > index 2e32a3291979..b7a89383fae0 100644 > --- a/src/segtree.c > +++ b/src/segtree.c > @@ -453,7 +453,7 @@ static struct expr *interval_to_string(struct expr *low, struct expr *i, const m > { > unsigned int len = div_round_up(i->len, BITS_PER_BYTE); > unsigned int prefix_len, str_len; > - char data[len + 2]; > + char data[len + 2] = {}; > struct expr *expr; > > prefix_len = expr_value(i)->len - mpz_scan0(range, 0); > > otherwise uninitialized data could be send to the kernel? No, I don't think so, data is filled with len bytes: mpz_export_data(data, expr_value(low)->value, BYTEORDER_BIG_ENDIAN, len); So I don't see the reimport to fetch anything that was onstack garbage.