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.
