On 4 Mar 2026, at 16:50, Ilya Maximets wrote:
> On 3/3/26 4:51 PM, Eelco Chaudron wrote: >> >> >> On 2 Mar 2026, at 18:08, Mike Pattrick wrote: >> >>> On Mon, Mar 2, 2026 at 6:14 AM Eelco Chaudron <[email protected]> wrote: >>> >>>> >>>> >>>> On 27 Feb 2026, at 21:24, Mike Pattrick wrote: >>>> >>>>> On Thu, Feb 26, 2026 at 5:59 AM Eelco Chaudron via dev < >>>>> [email protected]> wrote: >>>>> >>>>>> Coverity reports that ovsdb_idl_txn_extract_mutations() calls >>>>>> ovsdb_datum_find_key() without checking the return value (as is done >>>>>> elsewhere 13 out of 15 times) before using the returned position to >>>>>> index into old_datum->values[pos]. >>>>>> >>>>>> If the key is not found, pos is uninitialized and using it leads to >>>>>> undefined behavior. Fix by checking the return value and combining >>>>>> the conditions, only skip the mutation if the key exists and the >>>>>> value is unchanged. >>>>>> >>>>>> Fixes: 51946d22274c ("ovsdb-data: Optimize union of sets.") >>>>>> Signed-off-by: Eelco Chaudron <[email protected]> >>>>>> --- >>>>>> lib/ovsdb-idl.c | 11 ++++++----- >>>>>> 1 file changed, 6 insertions(+), 5 deletions(-) >>>>>> >>>>>> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c >>>>>> index d8094458d..d86564c08 100644 >>>>>> --- a/lib/ovsdb-idl.c >>>>>> +++ b/lib/ovsdb-idl.c >>>>>> @@ -3054,12 +3054,13 @@ ovsdb_idl_txn_extract_mutations(struct >>>>>> ovsdb_idl_row *row, >>>>>> /* Find out if value really changed. */ >>>>>> struct ovsdb_datum *new_datum; >>>>>> unsigned int pos; >>>>>> + >>>>>> new_datum = map_op_datum(map_op); >>>>>> - ovsdb_datum_find_key(old_datum, >>>> &new_datum->keys[0], >>>>>> - key_type, &pos); >>>>>> - if (ovsdb_atom_equals(&new_datum->values[0], >>>>>> - &old_datum->values[pos], >>>>>> - value_type)) { >>>>>> + if (ovsdb_datum_find_key(old_datum, >>>>>> &new_datum->keys[0], >>>>>> + key_type, &pos) >>>>>> >>>>> >>>>> Shouldn't this be "if (!ovsdb_datum_find_key || ovsdb_atom_equals()" ? >>>> >>>> >>>> I think it's correct, but I'm not the dbase expert. The idea is we only >>>> want to skip the mutation when both conditions are true: the key exists AND >>>> the value is unchanged. >>>> >>>> With !found || equals, we'd end up skipping mutations when adding new >>>> keys, which I guess isn't what we want. >>>> Let's explain my thinking with the options we have: >>>> >>>> - New key being added: find_key() returns false -> whole condition is >>>> false -> mutation is processed >>>> - Existing key, value changed: find_key() returns true, but equals() is >>>> false -> whole condition is false -> mutation is processed >>>> - Existing key, value unchanged: find_key() returns true AND equals() is >>>> true -> whole condition is true -> mutation is skipped >>>> >>>> If we used !found || equals, then when adding a new key (found is false), >>>> the condition would evaluate to true and we'd incorrectly skip the >>>> mutation. >>>> >>>> Does that make sense? >>> >>> >>> If the key doesn't exist then the OP should be an insert not an update. >>> AFAIK it shoudn't be possible to update a non-existent key. > > Yeah, this condition should not be possible. The MAP_OP_UPDATE operation > is only created if the search was already successful. See the code in > ovsdb_idl_txn_write_partial_map(). So, the second time we lookup the same > thing must be successful as well. Unless the user overwrote the whole > column value while also having partial map updates, that would be a an > error in the application. Though it may not be worth the assert here, > so I'd suggest we print a warning if the search failed and continue. E.g. > "Trying to update a value for a key that no longer exists in the map." > Similarly how deletion is handling this. No need to merge the 'continue' > cases into a single 'if' statement, IMO. Thanks this is clear, will sent out a new revision of the series. //Eelco [...] _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
