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. Including Ilya as he knows more on the dbase side. >>> + && ovsdb_atom_equals(&new_datum->values[0], >>>> + &old_datum->values[pos], >>>> + value_type)) { >>>> /* No change in value. Move on to next update. >> */ >>>> continue; >>>> } >>>> -- >>>> 2.52.0 >>>> >>>> _______________________________________________ >>>> dev mailing list >>>> [email protected] >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>> >>>> >> >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
