On 13 Jun 2023, at 20:34, James Raphael Tiovalen wrote:
> This commit adds various null pointer checks to some files in the `lib`
> and the `ovsdb` directories to fix several Coverity defects. These
> changes are grouped together as they perform similar checks, returning
> early or skipping some action if a null pointer is encountered.
>
> Signed-off-by: James Raphael Tiovalen <jamestio...@gmail.com>
> Reviewed-by: Simon Horman <simon.hor...@corigine.com>
Thanks for the patch James, some small nits below.
Cheers,
Eelco
> ---
> lib/dp-packet.c | 8 ++++----
> lib/shash.c | 7 ++++++-
> ovsdb/jsonrpc-server.c | 2 +-
> ovsdb/monitor.c | 7 ++++++-
> ovsdb/ovsdb-client.c | 7 ++++++-
> ovsdb/row.c | 4 +++-
> 6 files changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> index 445cb4761..ca29b1f90 100644
> --- a/lib/dp-packet.c
> +++ b/lib/dp-packet.c
> @@ -353,7 +353,7 @@ void *
> dp_packet_put_zeros(struct dp_packet *b, size_t size)
> {
> void *dst = dp_packet_put_uninit(b, size);
> - memset(dst, 0, size);
> + nullable_memset(dst, 0, size);
> return dst;
> }
>
> @@ -364,7 +364,7 @@ void *
> dp_packet_put(struct dp_packet *b, const void *p, size_t size)
> {
> void *dst = dp_packet_put_uninit(b, size);
> - memcpy(dst, p, size);
> + nullable_memcpy(dst, p, size);
> return dst;
> }
>
> @@ -436,7 +436,7 @@ void *
> dp_packet_push_zeros(struct dp_packet *b, size_t size)
> {
> void *dst = dp_packet_push_uninit(b, size);
> - memset(dst, 0, size);
> + nullable_memset(dst, 0, size);
> return dst;
> }
>
> @@ -447,7 +447,7 @@ void *
> dp_packet_push(struct dp_packet *b, const void *p, size_t size)
> {
> void *dst = dp_packet_push_uninit(b, size);
> - memcpy(dst, p, size);
> + nullable_memcpy(dst, p, size);
> return dst;
> }
>
> diff --git a/lib/shash.c b/lib/shash.c
> index 2bfc8eb50..b72b5bf27 100644
> --- a/lib/shash.c
> +++ b/lib/shash.c
> @@ -205,8 +205,13 @@ shash_delete(struct shash *sh, struct shash_node *node)
> char *
> shash_steal(struct shash *sh, struct shash_node *node)
> {
> - char *name = node->name;
> + char *name;
>
> + if (!node) {
> + return NULL;
> + }
> +
> + name = node->name;
Maybe just define it here (and remove it above)..
char *name = node->name;
> hmap_remove(&sh->map, &node->node);
> free(node);
> return name;
> diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
> index 17868f5b7..5361b3c76 100644
> --- a/ovsdb/jsonrpc-server.c
> +++ b/ovsdb/jsonrpc-server.c
> @@ -1038,7 +1038,7 @@ ovsdb_jsonrpc_session_got_request(struct
> ovsdb_jsonrpc_session *s,
> request->id);
> } else if (!strcmp(request->method, "get_schema")) {
> struct ovsdb *db = ovsdb_jsonrpc_lookup_db(s, request, &reply);
> - if (!reply) {
> + if (db && !reply) {
> reply = jsonrpc_create_reply(ovsdb_schema_to_json(db->schema),
> request->id);
> }
> diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
> index c32af7b02..b560b0745 100644
> --- a/ovsdb/monitor.c
> +++ b/ovsdb/monitor.c
> @@ -483,7 +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;
> + }
Line feed.
> /* Check for column duplication. Return duplicated column name. */
> if (mt->columns_index_map[column->index] != -1) {
> return column->name;
> @@ -810,6 +812,9 @@ ovsdb_monitor_table_condition_update(
>
> struct ovsdb_monitor_table_condition *mtc =
> shash_find_data(&condition->tables, table->schema->name);
> + if (!mtc) {
> + return NULL;
> + }
Looks a bit odd in the middle of the definitions. Maybe move it down?
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;
}
Or add surrounding new lines:
struct ovsdb_monitor_table_condition *mtc =
shash_find_data(&condition->tables, table->schema->name);
if (!mtc) {
return NULL;
}
struct ovsdb_error *error;
struct ovsdb_condition cond = OVSDB_CONDITION_INITIALIZER(&cond);
> struct ovsdb_error *error;
> struct ovsdb_condition cond = OVSDB_CONDITION_INITIALIZER(&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;
> + }
> +
> /* Construct transaction to retrieve entire database. */
> transaction = json_array_create_1(json_string_create(database));
> for (i = 0; i < n_tables; i++) {
> @@ -1932,8 +1936,9 @@ do_dump(struct jsonrpc *rpc, const char *database,
> }
>
> jsonrpc_msg_destroy(reply);
> - shash_destroy(&custom_columns);
> free(tables);
> +end:
> + shash_destroy(&custom_columns);
> ovsdb_schema_destroy(schema);
> }
>
> diff --git a/ovsdb/row.c b/ovsdb/row.c
> index d7bfbdd36..9f87c832a 100644
> --- a/ovsdb/row.c
> +++ b/ovsdb/row.c
> @@ -399,7 +399,9 @@ ovsdb_row_set_add_row(struct ovsdb_row_set *set, const
> struct ovsdb_row *row)
> set->rows = x2nrealloc(set->rows, &set->allocated_rows,
> sizeof *set->rows);
> }
> - set->rows[set->n_rows++] = row;
> + if (set->rows) {
Is this needed, as x2nrealloc will assert if no memory is available?
> + set->rows[set->n_rows++] = row;
> + }
> }
>
> struct json *
> --
> 2.40.1
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
