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.
Best regards, Ilya Maximets.
>
> 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
