Re: [PATCH nft] expression: don't try to import empty string

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Mon, Mar 31, 2025 at 02:37:15PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Hi Florian,
> >
> > On Thu, Mar 27, 2025 at 04:17:11PM +0100, Florian Westphal wrote:
> > > The bogon will trigger the assertion in mpz_import_data:
> > > src/expression.c:418: constant_expr_alloc: Assertion `(((len) + (8) - 1) / (8)) > 0' failed.
> >
> > I took a quick look searching for {s:s} in src/parser_json.c
> >
> > The common idiom is json_parse_err() then a helper parser function to
> > validate the string.
> >
> > It seems it is missing in this case. Maybe tigthen json parser instead?
> >
> > Caller invoking constant_expr_alloc() with data != NULL but no len
> > looks broken to me.
>
>         return constant_expr_alloc(int_loc, &string_type, BYTEORDER_HOST_ENDIAN,
>                                    strlen(chain) * BITS_PER_BYTE, chain);
>
> chain name is '""'.
>
> There are other spots where we possibly call into constant_expr_alloc()
> with a 0 argument.

Reviewed-by: Pablo Neira Ayuso <pablo@netfilter.org>

> I think it would be a lot more work and bloat to add all the checks on
> the json side while its a one-liner in constant_expr_alloc().

I don't think this is needed, the idiom in src/parser_json.c is to:

1) fetch string
2) validate it

For example:

        if (!json_unpack(root, "{s:s}", "ttl", &ttl)) {
                if (!strcmp(ttl, "loose")) {
                        ttlval = 1;
                } else if (!strcmp(ttl, "skip")) {
                        ttlval = 2;
                } else {
                        json_error(ctx, "Invalid osf ttl option '%s'.", ttl);
                        return NULL;
                }
        }

> I could also add json_constant_expr_alloc() but it seems kinda silly to me.

I agree, I don't think json_constant_expr_alloc is needed.