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? Cheers, Eelco > + && 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
