On 13 Jun 2023, at 20:34, James Raphael Tiovalen wrote:
> This patch adds an assortment of `ovs_assert` statements to check for > null pointers. We use assertions since it should be impossible for any > of these pointers to be NULL. > > Signed-off-by: James Raphael Tiovalen <[email protected]> > Reviewed-by: Simon Horman <[email protected]> Hi James, See comments inline below. Cheers, Eelco > --- > lib/dp-packet.c | 1 + > lib/dpctl.c | 4 ++++ > lib/odp-execute.c | 4 ++++ > lib/rtnetlink.c | 4 ++-- > lib/shash.c | 2 +- > lib/sset.c | 2 ++ > ovsdb/jsonrpc-server.c | 4 ++++ > ovsdb/monitor.c | 3 +++ > ovsdb/ovsdb-server.c | 2 ++ > ovsdb/ovsdb.c | 8 +++++++- > ovsdb/query.c | 2 ++ > ovsdb/transaction.c | 2 ++ > utilities/ovs-vsctl.c | 3 +++ > vtep/vtep-ctl.c | 5 ++++- > 14 files changed, 41 insertions(+), 5 deletions(-) > > diff --git a/lib/dp-packet.c b/lib/dp-packet.c > index ca29b1f90..059db48ba 100644 > --- a/lib/dp-packet.c > +++ b/lib/dp-packet.c > @@ -171,6 +171,7 @@ dp_packet_new_with_headroom(size_t size, size_t headroom) > struct dp_packet * > dp_packet_clone(const struct dp_packet *buffer) > { > + ovs_assert(buffer); > return dp_packet_clone_with_headroom(buffer, 0); > } > > diff --git a/lib/dpctl.c b/lib/dpctl.c > index 15950bd50..51cd6c88c 100644 > --- a/lib/dpctl.c > +++ b/lib/dpctl.c > @@ -336,6 +336,8 @@ dpctl_add_if(int argc OVS_UNUSED, const char *argv[], > value = ""; > } > > + ovs_assert(key); > + I think we should not assert, but do proper error handling here with a dpctl_error() > if (!strcmp(key, "type")) { > type = value; > } else if (!strcmp(key, "port_no")) { > @@ -454,6 +456,8 @@ dpctl_set_if(int argc, const char *argv[], struct > dpctl_params *dpctl_p) > value = ""; > } > > + ovs_assert(key); > + Same as above. > if (!strcmp(key, "type")) { > if (strcmp(value, type)) { > dpctl_error(dpctl_p, 0, > diff --git a/lib/odp-execute.c b/lib/odp-execute.c > index 5cf6fbec0..44b2cd360 100644 > --- a/lib/odp-execute.c > +++ b/lib/odp-execute.c > @@ -147,6 +147,8 @@ odp_set_ipv4(struct dp_packet *packet, const struct > ovs_key_ipv4 *key, > uint8_t new_tos; > uint8_t new_ttl; > > + ovs_assert(nh); > + > if (mask->ipv4_src) { > ip_src_nh = get_16aligned_be32(&nh->ip_src); > new_ip_src = key->ipv4_src | (ip_src_nh & ~mask->ipv4_src); > @@ -276,6 +278,8 @@ set_arp(struct dp_packet *packet, const struct > ovs_key_arp *key, > { > struct arp_eth_header *arp = dp_packet_l3(packet); > > + ovs_assert(arp); > + > if (!mask) { > arp->ar_op = key->arp_op; > arp->ar_sha = key->arp_sha; > diff --git a/lib/rtnetlink.c b/lib/rtnetlink.c > index f67352603..37078d00e 100644 > --- a/lib/rtnetlink.c > +++ b/lib/rtnetlink.c > @@ -112,7 +112,7 @@ rtnetlink_parse(struct ofpbuf *buf, struct > rtnetlink_change *change) > if (parsed) { > const struct ifinfomsg *ifinfo; > > - ifinfo = ofpbuf_at(buf, NLMSG_HDRLEN, sizeof *ifinfo); > + ifinfo = ofpbuf_at_assert(buf, NLMSG_HDRLEN, sizeof *ifinfo); > > /* Wireless events can be spammy and cause a > * lot of unnecessary churn and CPU load in > @@ -175,7 +175,7 @@ rtnetlink_parse(struct ofpbuf *buf, struct > rtnetlink_change *change) > if (parsed) { > const struct ifaddrmsg *ifaddr; > > - ifaddr = ofpbuf_at(buf, NLMSG_HDRLEN, sizeof *ifaddr); > + ifaddr = ofpbuf_at_assert(buf, NLMSG_HDRLEN, sizeof *ifaddr); > > change->nlmsg_type = nlmsg->nlmsg_type; > change->if_index = ifaddr->ifa_index; > diff --git a/lib/shash.c b/lib/shash.c > index b72b5bf27..c712b3769 100644 > --- a/lib/shash.c > +++ b/lib/shash.c > @@ -270,7 +270,7 @@ void * > shash_find_and_delete_assert(struct shash *sh, const char *name) > { > void *data = shash_find_and_delete(sh, name); > - ovs_assert(data != NULL); > + ovs_assert(data); > return data; > } > > diff --git a/lib/sset.c b/lib/sset.c > index aa1790020..afdc4392c 100644 > --- a/lib/sset.c > +++ b/lib/sset.c > @@ -20,6 +20,7 @@ > > #include "openvswitch/dynamic-string.h" > #include "hash.h" > +#include "util.h" > > static uint32_t > hash_name__(const char *name, size_t length) > @@ -261,6 +262,7 @@ char * > sset_pop(struct sset *set) > { > const char *name = SSET_FIRST(set); > + ovs_assert(name); I think if we do a pop from an empty set, we should just return NULL, not assert. > 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 5361b3c76..a3ca48a7b 100644 > --- a/ovsdb/jsonrpc-server.c > +++ b/ovsdb/jsonrpc-server.c > @@ -1131,6 +1131,8 @@ static void > ovsdb_jsonrpc_trigger_create(struct ovsdb_jsonrpc_session *s, struct ovsdb > *db, > struct jsonrpc_msg *request) > { For the below ones, please see earlier comments on ovsdb related code, Ilya? > + ovs_assert(db); > + > /* Check for duplicate ID. */ > size_t hash = json_hash(request->id, 0); > struct ovsdb_jsonrpc_trigger *t > @@ -1391,6 +1393,8 @@ ovsdb_jsonrpc_monitor_create(struct > ovsdb_jsonrpc_session *s, struct ovsdb *db, > enum ovsdb_monitor_version version, > const struct json *request_id) > { > + ovs_assert(db); > + > struct ovsdb_jsonrpc_monitor *m = NULL; > struct ovsdb_monitor *dbmon = NULL; > struct json *monitor_id, *monitor_requests; > diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c > index b560b0745..2180f9e2d 100644 > --- a/ovsdb/monitor.c > +++ b/ovsdb/monitor.c > @@ -1327,6 +1327,7 @@ ovsdb_monitor_table_add_select(struct ovsdb_monitor > *dbmon, > struct ovsdb_monitor_table * mt; > > mt = shash_find_data(&dbmon->tables, table->schema->name); > + ovs_assert(mt); > mt->select |= select; > } > > @@ -1710,6 +1711,8 @@ ovsdb_monitor_hash(const struct ovsdb_monitor *dbmon, > size_t basis) > for (i = 0; i < n; i++) { > struct ovsdb_monitor_table *mt = nodes[i]->data; > > + ovs_assert(mt); > + > basis = hash_pointer(mt->table, basis); > basis = hash_3words(mt->select, mt->n_columns, basis); > > diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c > index 9bad0c8dd..87da41f17 100644 > --- a/ovsdb/ovsdb-server.c > +++ b/ovsdb/ovsdb-server.c > @@ -2229,6 +2229,8 @@ save_config(struct server_config *config) > static void > sset_from_json(struct sset *sset, const struct json *array) > { > + ovs_assert(array); > + > size_t i; > > sset_clear(sset); > diff --git a/ovsdb/ovsdb.c b/ovsdb/ovsdb.c > index f67b836d7..625b4ca68 100644 > --- a/ovsdb/ovsdb.c > +++ b/ovsdb/ovsdb.c > @@ -40,6 +40,7 @@ > #include "transaction-forward.h" > #include "trigger.h" > #include "unixctl.h" > +#include "util.h" > > #include "openvswitch/vlog.h" > VLOG_DEFINE_THIS_MODULE(ovsdb); > @@ -229,7 +230,7 @@ root_set_size(const struct ovsdb_schema *schema) > struct ovsdb_error * > ovsdb_schema_from_json(const struct json *json, struct ovsdb_schema > **schemap) > { > - struct ovsdb_schema *schema; > + struct ovsdb_schema *schema = NULL; > const struct json *name, *tables, *version_json, *cksum; > struct ovsdb_error *error; > struct shash_node *node; > @@ -249,6 +250,9 @@ ovsdb_schema_from_json(const struct json *json, struct > ovsdb_schema **schemap) > return error; > } > > + ovs_assert(name); > + ovs_assert(tables); > + > if (version_json) { > version = json_string(version_json); > if (!ovsdb_is_valid_version(version)) { > @@ -282,6 +286,8 @@ ovsdb_schema_from_json(const struct json *json, struct > ovsdb_schema **schemap) > shash_add(&schema->tables, table->name, table); > } > > + ovs_assert(schema); > + > /* "isRoot" was not part of the original schema definition. Before it > was > * added, there was no support for garbage collection. So, for backward > * compatibility, if the root set is empty then assume that every table > is > diff --git a/ovsdb/query.c b/ovsdb/query.c > index eebe56412..29cc93093 100644 > --- a/ovsdb/query.c > +++ b/ovsdb/query.c > @@ -21,6 +21,7 @@ > #include "condition.h" > #include "row.h" > #include "table.h" > +#include "util.h" > > void > ovsdb_query(struct ovsdb_table *table, const struct ovsdb_condition *cnd, > @@ -91,6 +92,7 @@ ovsdb_query_distinct(struct ovsdb_table *table, > struct ovsdb_row_hash hash; > > ovsdb_row_hash_init(&hash, columns); > + ovs_assert(condition); > ovsdb_query(table, condition, query_distinct_cb, &hash); > HMAP_FOR_EACH (node, hmap_node, &hash.rows) { > ovsdb_row_set_add_row(results, node->row); > diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c > index 7cf4a851a..4fdc5bcea 100644 > --- a/ovsdb/transaction.c > +++ b/ovsdb/transaction.c > @@ -34,6 +34,7 @@ > #include "storage.h" > #include "table.h" > #include "uuid.h" > +#include "util.h" > > VLOG_DEFINE_THIS_MODULE(transaction); > > @@ -576,6 +577,7 @@ ovsdb_txn_update_weak_refs(struct ovsdb_txn *txn > OVS_UNUSED, > dst_row = CONST_CAST(struct ovsdb_row *, > ovsdb_table_get_row(weak->dst_table, &weak->dst)); > > + ovs_assert(dst_row); > ovs_assert(!ovsdb_row_find_weak_ref(dst_row, weak)); > hmap_insert(&dst_row->dst_refs, &weak->dst_node, > ovsdb_weak_ref_hash(weak)); > diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c > index f55c2965a..42203a01c 100644 > --- a/utilities/ovs-vsctl.c > +++ b/utilities/ovs-vsctl.c > @@ -671,6 +671,7 @@ add_port_to_cache(struct vsctl_context *vsctl_ctx, struct > vsctl_bridge *parent, > } > } > > + ovs_assert(parent); Did you analyse we only need the check for parent and not vsctl_bridge or port_config? Maybe it would be nicer to fix the calling functions to avoid this warning, as I guess that is where the real problem exists? > ovs_list_push_back(&parent->ports, &port->ports_node); > ovs_list_init(&port->ifaces); > port->port_cfg = port_cfg; > @@ -817,6 +818,7 @@ vsctl_context_populate_cache(struct ctl_context *ctx) > continue; > } > br = shash_find_data(&vsctl_ctx->bridges, br_cfg->name); > + ovs_assert(br); > for (j = 0; j < br_cfg->n_ports; j++) { > struct ovsrec_port *port_cfg = br_cfg->ports[j]; > struct vsctl_port *port; > @@ -843,6 +845,7 @@ vsctl_context_populate_cache(struct ctl_context *ctx) > } > > port = add_port_to_cache(vsctl_ctx, br, port_cfg); > + ovs_assert(port); Is this really needed? As looking at the code add_port_to_cache() will assert and never return Null? Asking as you did not add a similar assert for line 791 above; ‘br = add_bridge_to_cache(vsctl_ctx, br_cfg, br_cfg->name, NULL, 0);’ > for (k = 0; k < port_cfg->n_interfaces; k++) { > struct ovsrec_interface *iface_cfg = port_cfg->interfaces[k]; > struct vsctl_iface *iface; > diff --git a/vtep/vtep-ctl.c b/vtep/vtep-ctl.c > index e5d99714d..d5a6575d5 100644 > --- a/vtep/vtep-ctl.c > +++ b/vtep/vtep-ctl.c > @@ -1065,6 +1065,7 @@ vtep_ctl_context_populate_cache(struct ctl_context *ctx) > continue; > } > ps = shash_find_data(&vtepctl_ctx->pswitches, ps_cfg->name); > + ovs_assert(ps); > for (j = 0; j < ps_cfg->n_ports; j++) { > struct vteprec_physical_port *port_cfg = ps_cfg->ports[j]; > struct vtep_ctl_port *port; > @@ -1087,7 +1088,7 @@ vtep_ctl_context_populate_cache(struct ctl_context *ctx) > } > > port = add_port_to_cache(vtepctl_ctx, ps, port_cfg); > - > + ovs_assert(port); See above. > for (k = 0; k < port_cfg->n_vlan_bindings; k++) { > struct vtep_ctl_lswitch *ls; > char *vlan; > @@ -1884,6 +1885,8 @@ del_mcast_entry(struct ctl_context *ctx, > if (ovs_list_is_empty(&mcast_mac->locators)) { > struct shash_node *node = shash_find(mcast_shash, mac); > > + ovs_assert(node); Cant we not log a warning and skip freeing the data and the node? > + > vteprec_physical_locator_set_delete(ploc_set_cfg); > > if (local) { > -- > 2.40.1 > > _______________________________________________ > 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
