On 8/24/23 12:17, Eelco Chaudron wrote:
>
>
> On 3 Aug 2023, at 18:19, James Raphael Tiovalen wrote:
>
>> This commit adds various null pointer checks to some files in the `lib`,
>> `ovsdb`, and `vtep` directories to fix several Coverity defects. These
>> changes are grouped together as they perform similar checks, returning
>> early, skipping some action, or logging a warning if a null pointer is
>> encountered.
>
> Thanks for the patch, see the comments below. Also Ilya can you take a look
> at the DB API questions I have?
>
> Cheers,
>
> Eelco
>
>> Signed-off-by: James Raphael Tiovalen <[email protected]>
>> Reviewed-by: Simon Horman <[email protected]>
>> ---
>> lib/dp-packet.c | 8 ++++----
>> lib/dpctl.c | 12 ++++++++++++
>> lib/shash.c | 4 ++++
>> lib/sset.c | 5 +++++
>> ovsdb/jsonrpc-server.c | 2 +-
>> ovsdb/monitor.c | 11 +++++++++--
>> ovsdb/ovsdb-client.c | 7 ++++++-
>> ovsdb/row.c | 5 ++++-
>> vtep/vtep-ctl.c | 5 +++++
>> 9 files changed, 50 insertions(+), 9 deletions(-)
<snip>
>> diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
>> index 01091fabe..af88b96e3 100644
>> --- a/ovsdb/monitor.c
>> +++ b/ovsdb/monitor.c
>> @@ -483,6 +483,9 @@ ovsdb_monitor_add_column(struct ovsdb_monitor *dbmon,
>> struct ovsdb_monitor_column *c;
>>
>> mt = shash_find_data(&dbmon->tables, table->schema->name);
>> + if (!mt) {
>> + return NULL;
>> + }
>
> This might change the API behavior? Currently the API returns NULL on success
> and on duplicate column name detection the name of the column. Now on data
> find failure it also returns NULL, is this ok (I guess so, as in the past it
> would have crashed ;)?
I'd convert this into assertion. The table should have been already
checked before calling this function.
>
>>
>> /* Check for column duplication. Return duplicated column name. */
>> if (mt->columns_index_map[column->index] != -1) {
>> @@ -808,10 +811,14 @@ ovsdb_monitor_table_condition_update(
>> return NULL;
>> }
>>
>> - struct ovsdb_monitor_table_condition *mtc =
>> - shash_find_data(&condition->tables, table->schema->name);
>> struct ovsdb_error *error;
>> struct ovsdb_condition cond = OVSDB_CONDITION_INITIALIZER(&cond);
>> + struct ovsdb_monitor_table_condition *mtc =
>> + shash_find_data(&condition->tables, table->schema->name);
>> +
>> + if (!mtc) {
>> + return NULL;
>> + }
>>
>
> Same as above.
Same here. Assertion is probably better, to avoid the API discussion.
>
>> error = ovsdb_condition_from_json(table->schema, cond_json,
>> NULL, &cond);
>> diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c
>> index 46484630d..2b12907b6 100644
>> --- a/ovsdb/ovsdb-client.c
>> +++ b/ovsdb/ovsdb-client.c
>> @@ -1873,6 +1873,10 @@ do_dump(struct jsonrpc *rpc, const char *database,
>> n_tables = shash_count(&schema->tables);
>> }
>>
>> + if (!tables) {
>> + goto end;
>
> Would doing n_tables = 0 instead be more clean, as it keeps the current
> behavior? Guess it all depends on the DB API stuff, so Ilya any comments?
This one is strange. It's actually an impossible condition, AFAICT.
I suppose, Coverity is unable to draw a parallel between shash_sort()
and shash_count(). n_tables will be zero already if 'tables' in NULL.
So, setting it to zero doesn't make much sense. Maybe we can do
something like:
} else {
n_tables = shash_count(&schema->tables);
if (n_tables) {
tables = shash_sort(&schema->tables);
}
}
That will create a dependency between these two that maybe Coverity
will be able to identify?
>From the perspective of the ovsdb-client, we should still perform a
transaction, I think, even though it will not contain any operations.
That should be a normal condition for a database.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
