On Wed, Sep 26, 2018 at 04:32:56PM +0200, Pablo Neira Ayuso wrote:
> Hi Phil,
>
> On Tue, Sep 25, 2018 at 02:37:15PM +0200, Phil Sutter wrote:
> > Hi Pablo,
> >
> > When dealing with a covscan report for nft, I was pointed at the loop's
> > else-clause of get_set_decompose() as it overwrites 'left' without
> > freeing it first. The code in question is this:
> >
> > | list_for_each_entry_safe(i, next, &set->init->expressions, list) {
> > | if (i->flags & EXPR_F_INTERVAL_END && left) {
> > | list_del(&left->list);
> > | list_del(&i->list);
> > | mpz_sub_ui(i->key->value, i->key->value, 1);
> > | new = range_expr_alloc(&internal_location, left, i);
> > | compound_expr_add(new_init, new);
> > | left = NULL;
> > | } else {
> > | if (left) {
> > | left = get_set_interval_end(table,
> > | set->handle.set,
> > | left);
> > | compound_expr_add(new_init, left);
> > | }
> > | left = i;
> > | }
> > | }
> >
> > IIUC, the else-clause should be hit for non-interval-end items, and the
> > call to get_set_interval_end() happens if we have two consecutive
> > non-interval-end items. I tried to trigger it, but failed, hence I
> > wonder if this situation is possible at all. Do you perhaps remember why
> > you put the code like this when implementing 'get element' command
> > (commit a43cc8d53096d)?
>
> To deal with the range case from get_set_interval_end()
>
> list_for_each_entry(i, &set->init->expressions, list) {
> switch (i->key->ops->type) {
> case EXPR_RANGE:
> range_expr_value_low(low, i);
> if (mpz_cmp(low, left->key->value) == 0) {
> left = range_expr_alloc(&internal_location,
> expr_clone(left->key),
> expr_clone(i->key->right));
> break;
> }
> break;
> default:
> break;
> }
> }
>
> Which is creating the range expression once we see the left side. In
> that case, we go find the left hand side in the existing set cache.
minor correction:
... we go find the /right/ hand side ...
> There is not other way to make with the current approach given the
> existing netlink interface detaches intervals in two independent
> elements, where one of the has the INTERVAL_END flag set on.
>
> In case this is not a range, left is not modified.
>
> In case it is a range, we can release it.
>
> Try creating a set with intervals and using the get command to fetch
> either one single element (included in the interval) or the interval
> itself. I remember nft get element supports both case, to test for
> element inclusion in existing range and to test explicitly for a range
> in the set.
>
> Let me know if this helps, thanks!
