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.
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!
