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