James Raphael Tiovalen <[email protected]> writes:
> 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 <[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(-)
>
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> index 6e3a8297a..a9cf4bfc1 100644
> --- a/lib/dp-packet.c
> +++ b/lib/dp-packet.c
> @@ -357,7 +357,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;
> }
>
> @@ -368,7 +368,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;
> }
>
> @@ -440,7 +440,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;
> }
>
> @@ -451,7 +451,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/dpctl.c b/lib/dpctl.c
> index 4394653ab..013919572 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -336,6 +336,12 @@ dpctl_add_if(int argc OVS_UNUSED, const char *argv[],
> value = "";
> }
>
> + if (!key) {
> + dpctl_error(dpctl_p, 0, "key is NULL");
> + error = EINVAL;
> + goto next;
> + }
> +
> if (!strcmp(key, "type")) {
> type = value;
> } else if (!strcmp(key, "port_no")) {
> @@ -454,6 +460,12 @@ dpctl_set_if(int argc, const char *argv[], struct
> dpctl_params *dpctl_p)
> value = "";
> }
>
> + if (!key) {
> + dpctl_error(dpctl_p, 0, "key is NULL");
> + error = EINVAL;
> + goto next_destroy_args;
> + }
> +
> if (!strcmp(key, "type")) {
> if (strcmp(value, type)) {
> dpctl_error(dpctl_p, 0,
> diff --git a/lib/shash.c b/lib/shash.c
> index 2bfc8eb50..b62b586fc 100644
> --- a/lib/shash.c
> +++ b/lib/shash.c
> @@ -205,6 +205,10 @@ shash_delete(struct shash *sh, struct shash_node *node)
> char *
> shash_steal(struct shash *sh, struct shash_node *node)
> {
> + if (!node) {
> + return NULL;
> + }
> +
> char *name = node->name;
We typically like to declare variables at the head of the function, even
if it is allowed by the compiler. That said, I guess this probably
isn't too big of a deal - it gets a bit mixed all over the code base.
> hmap_remove(&sh->map, &node->node);
> diff --git a/lib/sset.c b/lib/sset.c
> index aa1790020..fda268129 100644
> --- a/lib/sset.c
> +++ b/lib/sset.c
> @@ -261,6 +261,11 @@ char *
> sset_pop(struct sset *set)
> {
> const char *name = SSET_FIRST(set);
> +
> + if (!name) {
> + return NULL;
> + }
> +
> char *copy = xstrdup(name);
> sset_delete(set, SSET_NODE_FROM_NAME(name));
> return copy;
> 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) {
I guess this is just to shut up coverity, since we check the reply
value.
> reply = jsonrpc_create_reply(ovsdb_schema_to_json(db->schema),
> request->id);
> }
> 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;
> + }
>
> /* 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;
> + }
>
> 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;
> + }
> +
> /* 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);
I think we can't adjust this call. Custom columns can reference data
from tschema nodes, which are xmemdup'd from tables. If we free(tables)
before hand, I think the data values could be included.
free() can accept a null argument, and we encourage not to null check
before calling free() - why move this deallocation?
> ovsdb_schema_destroy(schema);
> }
>
> diff --git a/ovsdb/row.c b/ovsdb/row.c
> index d7bfbdd36..be19e4940 100644
> --- a/ovsdb/row.c
> +++ b/ovsdb/row.c
> @@ -399,7 +399,10 @@ 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) {
> + set->rows[set->n_rows++] = row;
> + }
I'm surprised this was trapped as needing a check. Even if so, it would
be better to put the check as part of the test for allocated rows.
x2nrealloc will automatically flag an OOM condition if it would return
NULL.
> }
>
> struct json *
> diff --git a/vtep/vtep-ctl.c b/vtep/vtep-ctl.c
> index e5d99714d..bb22cc5ce 100644
> --- a/vtep/vtep-ctl.c
> +++ b/vtep/vtep-ctl.c
> @@ -1884,6 +1884,11 @@ del_mcast_entry(struct ctl_context *ctx,
> if (ovs_list_is_empty(&mcast_mac->locators)) {
> struct shash_node *node = shash_find(mcast_shash, mac);
>
> + if (!node) {
> + VLOG_WARN("node is NULL");
> + return;
> + }
> +
I have a suspicion this could make a problematic situation if it ever
hits. I think it would be better to assert here, or put a check before
we free(node->data)
> vteprec_physical_locator_set_delete(ploc_set_cfg);
>
> if (local) {
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
