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(-) > > 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"); This is more of a programmer’s error message, maybe change it to “Invalid option format”. > + 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"); This is more of a programmer’s error message, maybe change it to “Invalid option format”. > + 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; > > 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) { > 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; > + } 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 ;)? > > /* 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. > 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? > + } > + > /* 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..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; > + } > } > > 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 cleaned this up a but, so it does not need any warning, etc. What do you think of the below: @@ -1858,18 +1858,21 @@ del_mcast_entry(struct ctl_context *ctx, const char *encap, const char *dst_ip, bool local) { struct vtep_ctl_context *vtepctl_ctx = vtep_ctl_context_cast(ctx); + struct vteprec_physical_locator_set *ploc_set_cfg; + struct vteprec_physical_locator *ploc_cfg; struct vtep_ctl_mcast_mac *mcast_mac; struct shash *mcast_shash; - struct vteprec_physical_locator *ploc_cfg; - struct vteprec_physical_locator_set *ploc_set_cfg; + struct shash_node *mcast_node; mcast_shash = local ? &ls->mcast_local : &ls->mcast_remote; - mcast_mac = shash_find_data(mcast_shash, mac); - if (!mcast_mac) { + mcast_node = shash_find(mcast_shash, mac); + if (!mcast_node || !mcast_node->data) { return; } + mcast_mac = mcast_node->data; + ploc_cfg = find_ploc(vtepctl_ctx, encap, dst_ip); if (!ploc_cfg) { /* Couldn't find the physical locator, so just ignore. */ @@ -1882,13 +1885,6 @@ del_mcast_entry(struct ctl_context *ctx, del_ploc_from_mcast_mac(mcast_mac, ploc_cfg); 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; - } - vteprec_physical_locator_set_delete(ploc_set_cfg); if (local) { @@ -1897,8 +1893,8 @@ del_mcast_entry(struct ctl_context *ctx, vteprec_mcast_macs_remote_delete(mcast_mac->remote_cfg); } - free(node->data); - shash_delete(mcast_shash, node); + free(mcast_node->data); + shash_delete(mcast_shash, mcast_node); } else { if (local) { vteprec_mcast_macs_local_set_locator_set(mcast_mac->local_cfg, > vteprec_physical_locator_set_delete(ploc_set_cfg); > > if (local) { > -- > 2.41.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
