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 <jamestio...@gmail.com>
> Reviewed-by: Simon Horman <simon.hor...@corigine.com>
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
> 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
