| Message ID | 20230330150932.1296105-1-jamestiotio@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [ovs-dev,v6] lib, ovsdb, ovs-vsctl, vtep-ctl: Fix multiple Coverity defects | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/intel-ovs-compilation | success | test: success |
James Raphael Tiovalen <jamestiotio@gmail.com> writes: > This commit addresses several high and medium-impact Coverity defects by > fixing several possible null-pointer dereferences and potentially > uninitialized variables. > > There were cases when crashes were encountered when some null pointers > were dereferenced. Null pointer checks and alternative code paths have > been added to prevent unwanted crashes. For impossible conditions, > non-null assertions are added. Additionally, some struct variables were > not initialized. Zero-initializations have been added to prevent > potential data leaks or undefined behavior. > > Some variables would always be initialized by either the code flow or > the compiler and some pointers would never be null. Thus, some Coverity > reports might be false positives. That said, it is still considered best > practice to perform null pointer checks and initialize variables upfront > just in case to ensure the overall resilience and security of OVS, as > long as they do not impact performance-critical code. As a bonus, it > would also make static analyzer tools, such as Coverity, happy. > > Unit tests have been successfully executed via `make check` and they > successfully passed. > > Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com> > --- Sorry to comment on this so late - one thing I would also say can help with review is to separate the logical changes. For example, each flag from Coverity could be a separate patch in a cleanup series. It could help to keep things organized. Comments to follow. > v6: > - Convert some explicit null pointer checks to assertions since they are > checking for impossible conditions. > - Use `nullable_memset()` and `nullable_memcpy()` instead of manually > performing null checks for `memset()` and `memcpy()`. > > v5: > Improve commit message to better describe the intent of this patch. > > v4: > - Fix some apply-robot checkpatch errors and warnings. > - Fix github-robot build failure: linux clang test asan. > > v3: > Fix some apply-robot checkpatch errors and warnings. > > v2: > Fix some apply-robot checkpatch errors and warnings. > --- > lib/dp-packet.c | 22 ++++++++++++------- > lib/dpctl.c | 16 +++++++++----- > lib/json.c | 22 ++++++++++++------- > lib/latch-unix.c | 2 +- > lib/memory.c | 12 ++++++----- > lib/netdev-native-tnl.c | 17 +++++++++------ > lib/odp-execute.c | 4 ++++ > lib/pcap-file.c | 4 +++- > lib/rtnetlink.c | 5 +++++ > lib/sflow_agent.c | 6 ++++++ > lib/shash.c | 5 ++++- > lib/sset.c | 2 ++ > ovsdb/condition.c | 10 ++++++++- > ovsdb/file.c | 21 ++++++++++++++---- > ovsdb/jsonrpc-server.c | 6 +++++- > ovsdb/monitor.c | 36 +++++++++++++++++++++++++++---- > ovsdb/ovsdb-client.c | 46 +++++++++++++++++++++++++++------------- > ovsdb/ovsdb-server.c | 17 +++++++++------ > ovsdb/ovsdb-util.c | 9 ++++++++ > ovsdb/ovsdb.c | 8 ++++++- > ovsdb/query.c | 2 ++ > ovsdb/replication.c | 15 +++++++------ > ovsdb/row.c | 4 +++- > ovsdb/table.c | 2 +- > ovsdb/transaction.c | 2 ++ > utilities/ovs-vsctl.c | 47 ++++++++++++++++++++++++++++------------- > vtep/vtep-ctl.c | 10 ++++++--- > 27 files changed, 259 insertions(+), 93 deletions(-) > > diff --git a/lib/dp-packet.c b/lib/dp-packet.c > index ae8ab5800..f9c58a72f 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 != NULL); > return dp_packet_clone_with_headroom(buffer, 0); > } > > @@ -183,9 +184,12 @@ dp_packet_clone_with_headroom(const struct dp_packet *buffer, size_t headroom) > struct dp_packet *new_buffer; > uint32_t mark; > > - new_buffer = dp_packet_clone_data_with_headroom(dp_packet_data(buffer), > - dp_packet_size(buffer), > - headroom); > + const void *data_dp = dp_packet_data(buffer); > + ovs_assert(data_dp != NULL); There's a bit of intermingling of null checks here. Please stick to one style. Throughout the dp-packet.c file, there are positive tests (ie 'ovs_assert(data_dp)' would match the style more). > + > + new_buffer = dp_packet_clone_data_with_headroom(data_dp, > + dp_packet_size(buffer), > + headroom); > /* Copy the following fields into the returned buffer: l2_pad_size, > * l2_5_ofs, l3_ofs, l4_ofs, cutlen, packet_type and md. */ > memcpy(&new_buffer->l2_pad_size, &buffer->l2_pad_size, > @@ -323,7 +327,9 @@ dp_packet_shift(struct dp_packet *b, int delta) > > if (delta != 0) { > char *dst = (char *) dp_packet_data(b) + delta; > - memmove(dst, dp_packet_data(b), dp_packet_size(b)); > + const void *data_dp = dp_packet_data(b); > + ovs_assert(data_dp != NULL); Probably better to check this before we use dp_packet_data() in a calculation. It seems odd to check after we've set up dst. > + memmove(dst, data_dp, dp_packet_size(b)); > dp_packet_set_data(b, dst); > } > } > @@ -348,7 +354,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; > } > > @@ -359,7 +365,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; > } > > @@ -431,7 +437,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; > } > > @@ -442,7 +448,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 c501a0cd7..1833d3ba3 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 != NULL); > + > 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 != NULL); > + > if (!strcmp(key, "type")) { > if (strcmp(value, type)) { > dpctl_error(dpctl_p, 0, > @@ -693,12 +697,14 @@ show_dpif(struct dpif *dpif, struct dpctl_params *dpctl_p) > error = netdev_get_config(netdev, &config); > if (!error) { > const struct smap_node **nodes = smap_sort(&config); > - for (size_t j = 0; j < smap_count(&config); j++) { > - const struct smap_node *node = nodes[j]; > - dpctl_print(dpctl_p, "%c %s=%s", j ? ',' : ':', > - node->key, node->value); > + if (nodes) { > + for (size_t j = 0; j < smap_count(&config); j++) { > + const struct smap_node *node = nodes[j]; > + dpctl_print(dpctl_p, "%c %s=%s", j ? ',' : ':', > + node->key, node->value); > + } > + free(nodes); Rather than another level of indentation here, can we not change the test condition to be: for(size_t j = 0; (nodes && j < smap_count(&config)); j++) { } Similar comment in other places. An alternative is to change smap_count() to return '0' if smap is NULL, or assert there (and we can do this for shash as well). > } > - free(nodes); > } else { > dpctl_print(dpctl_p, ", could not retrieve configuration " > "(%s)", ovs_strerror(error)); > diff --git a/lib/json.c b/lib/json.c > index aded8bb01..11471f42a 100644 > --- a/lib/json.c > +++ b/lib/json.c > @@ -498,12 +498,15 @@ json_hash_object(const struct shash *object, size_t basis) > > nodes = shash_sort(object); > n = shash_count(object); > - for (i = 0; i < n; i++) { > - const struct shash_node *node = nodes[i]; > - basis = hash_string(node->name, basis); > - basis = json_hash(node->data, basis); > + > + if (nodes) { > + for (i = 0; i < n; i++) { > + const struct shash_node *node = nodes[i]; > + basis = hash_string(node->name, basis); > + basis = json_hash(node->data, basis); > + } > + free(nodes); > } > - free(nodes); > return basis; > } > > @@ -1655,10 +1658,13 @@ json_serialize_object(const struct shash *object, struct json_serializer *s) > > nodes = shash_sort(object); > n = shash_count(object); > - for (i = 0; i < n; i++) { > - json_serialize_object_member(i, nodes[i], s); > + > + if (nodes) { > + for (i = 0; i < n; i++) { > + json_serialize_object_member(i, nodes[i], s); > + } > + free(nodes); > } > - free(nodes); > } else { > struct shash_node *node; > size_t i; > diff --git a/lib/latch-unix.c b/lib/latch-unix.c > index f4d10c39a..262f111e4 100644 > --- a/lib/latch-unix.c > +++ b/lib/latch-unix.c > @@ -71,7 +71,7 @@ latch_set(struct latch *latch) > bool > latch_is_set(const struct latch *latch) > { > - struct pollfd pfd; > + struct pollfd pfd = {0, 0, 0}; > int retval; > > pfd.fd = latch->fds[0]; > diff --git a/lib/memory.c b/lib/memory.c > index da97476c6..d3bd8c295 100644 > --- a/lib/memory.c > +++ b/lib/memory.c > @@ -24,6 +24,7 @@ > #include "simap.h" > #include "timeval.h" > #include "unixctl.h" > +#include "util.h" > #include "openvswitch/vlog.h" > > VLOG_DEFINE_THIS_MODULE(memory); > @@ -116,13 +117,14 @@ compose_report(const struct simap *usage, struct ds *s) > size_t n = simap_count(usage); > size_t i; > > - for (i = 0; i < n; i++) { > - const struct simap_node *node = nodes[i]; > - > - ds_put_format(s, "%s:%u ", node->name, node->data); > + if (nodes) { > + for (i = 0; i < n; i++) { > + const struct simap_node *node = nodes[i]; > + ds_put_format(s, "%s:%u ", node->name, node->data); > + } > + free(nodes); > } > ds_chomp(s, ' '); > - free(nodes); > } > > /* Logs the contents of 'usage', as a collection of name-count pairs. > diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c > index b89dfdd52..b2283988b 100644 > --- a/lib/netdev-native-tnl.c > +++ b/lib/netdev-native-tnl.c > @@ -43,6 +43,7 @@ > #include "seq.h" > #include "unaligned.h" > #include "unixctl.h" > +#include "util.h" > #include "openvswitch/vlog.h" > > VLOG_DEFINE_THIS_MODULE(native_tnl); > @@ -221,12 +222,13 @@ netdev_tnl_calc_udp_csum(struct udp_header *udp, struct dp_packet *packet, > { > uint32_t csum; > > - if (netdev_tnl_is_header_ipv6(dp_packet_data(packet))) { > - csum = packet_csum_pseudoheader6(netdev_tnl_ipv6_hdr( > - dp_packet_data(packet))); > + void *data_dp = dp_packet_data(packet); > + ovs_assert(data_dp != NULL); > + > + if (netdev_tnl_is_header_ipv6(data_dp)) { > + csum = packet_csum_pseudoheader6(netdev_tnl_ipv6_hdr(data_dp)); > } else { > - csum = packet_csum_pseudoheader(netdev_tnl_ip_hdr( > - dp_packet_data(packet))); > + csum = packet_csum_pseudoheader(netdev_tnl_ip_hdr(data_dp)); > } > > csum = csum_continue(csum, udp, ip_tot_size); > @@ -425,7 +427,10 @@ netdev_gre_pop_header(struct dp_packet *packet) > struct flow_tnl *tnl = &md->tunnel; > int hlen = sizeof(struct eth_header) + 4; > > - hlen += netdev_tnl_is_header_ipv6(dp_packet_data(packet)) ? > + const void *data_dp = dp_packet_data(packet); > + ovs_assert(data_dp != NULL); > + > + hlen += netdev_tnl_is_header_ipv6(data_dp) ? > IPV6_HEADER_LEN : IP_HEADER_LEN; > > pkt_metadata_init_tnl(md); > diff --git a/lib/odp-execute.c b/lib/odp-execute.c > index 5cf6fbec0..97f950c60 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 != NULL); > + > 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 != NULL); > + > if (!mask) { > arp->ar_op = key->arp_op; > arp->ar_sha = key->arp_sha; > diff --git a/lib/pcap-file.c b/lib/pcap-file.c > index 3ed7ea488..bedfaae14 100644 > --- a/lib/pcap-file.c > +++ b/lib/pcap-file.c > @@ -291,7 +291,9 @@ ovs_pcap_write(struct pcap_file *p_file, struct dp_packet *buf) > prh.incl_len = dp_packet_size(buf); > prh.orig_len = dp_packet_size(buf); > ignore(fwrite(&prh, sizeof prh, 1, p_file->file)); > - ignore(fwrite(dp_packet_data(buf), dp_packet_size(buf), 1, p_file->file)); > + const void *data_dp = dp_packet_data(buf); > + ovs_assert(data_dp != NULL); This can be expressed closer to the first assert in the function: assert(dp_packet_data(buf)); Or possibly(? untested) change the assert from dp_packet_is_eth to assert(dp_packet_eth(buf)); > + ignore(fwrite(data_dp, dp_packet_size(buf), 1, p_file->file)); > fflush(p_file->file); > } > > diff --git a/lib/rtnetlink.c b/lib/rtnetlink.c > index f67352603..d2b2f85af 100644 > --- a/lib/rtnetlink.c > +++ b/lib/rtnetlink.c > @@ -26,6 +26,7 @@ > #include "netlink-notifier.h" > #include "openvswitch/ofpbuf.h" > #include "packets.h" > +#include "util.h" > > #if IFLA_INFO_MAX < 5 > #define IFLA_INFO_SLAVE_KIND 4 > @@ -131,6 +132,8 @@ rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change) > change->irrelevant = true; > } > > + ovs_assert(ifinfo != NULL); > + Can we instead change the ofpbuf_at to ofpbuf_at_assert() ? > change->nlmsg_type = nlmsg->nlmsg_type; > change->if_index = ifinfo->ifi_index; > change->ifname = nl_attr_get_string(attrs[IFLA_IFNAME]); > @@ -177,6 +180,8 @@ rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change) > > ifaddr = ofpbuf_at(buf, NLMSG_HDRLEN, sizeof *ifaddr); > > + ovs_assert(ifaddr != NULL); > + Same here > change->nlmsg_type = nlmsg->nlmsg_type; > change->if_index = ifaddr->ifa_index; > change->ifname = (attrs[IFA_LABEL] > diff --git a/lib/sflow_agent.c b/lib/sflow_agent.c > index c95f654a5..2199b52a5 100644 > --- a/lib/sflow_agent.c > +++ b/lib/sflow_agent.c > @@ -153,6 +153,8 @@ void sfl_agent_tick(SFLAgent *agent, time_t now) > SFLReceiver *sfl_agent_addReceiver(SFLAgent *agent) > { > SFLReceiver *rcv = (SFLReceiver *)sflAlloc(agent, sizeof(SFLReceiver)); > + ovs_assert(rcv != NULL); > + memset(rcv, 0, sizeof(*rcv)); Maybe instead of this, we should change the SFL_ALLOC define to be xzalloc which will guarantee a zero'd buffer that is != NULL and simply leave the assert. > sfl_receiver_init(rcv, agent); > /* add to end of list - to preserve the receiver index numbers for existing receivers */ > { > @@ -203,6 +205,8 @@ SFLSampler *sfl_agent_addSampler(SFLAgent *agent, SFLDataSource_instance *pdsi) > > { > SFLSampler *newsm = (SFLSampler *)sflAlloc(agent, sizeof(SFLSampler)); > + ovs_assert(newsm != NULL); > + memset(newsm, 0, sizeof(*newsm)); Indentation on this is wrong. > sfl_sampler_init(newsm, agent, pdsi); > if(prev) prev->nxt = newsm; > else agent->samplers = newsm; > @@ -242,6 +246,8 @@ SFLPoller *sfl_agent_addPoller(SFLAgent *agent, > /* either we found the insert point, or reached the end of the list... */ > { > SFLPoller *newpl = (SFLPoller *)sflAlloc(agent, sizeof(SFLPoller)); > + ovs_assert(newpl != NULL); > + memset(newpl, 0, sizeof(*newpl)); Same here. > sfl_poller_init(newpl, agent, pdsi, magic, getCountersFn); > if(prev) prev->nxt = newpl; > else agent->pollers = newpl; > diff --git a/lib/shash.c b/lib/shash.c > index a7b2c6458..070bc3e82 100644 > --- a/lib/shash.c > +++ b/lib/shash.c > @@ -17,6 +17,7 @@ > #include <config.h> > #include "openvswitch/shash.h" > #include "hash.h" > +#include "util.h" > > static struct shash_node *shash_find__(const struct shash *, > const char *name, size_t name_len, > @@ -194,7 +195,9 @@ shash_replace_nocopy(struct shash *sh, char *name, const void *data) > void > shash_delete(struct shash *sh, struct shash_node *node) > { > - free(shash_steal(sh, node)); > + if (node) { > + free(shash_steal(sh, node)); > + } Consider we should move this type of check to shash steal, or possibly change shash_steal to assert node != NULL (although, shash_steal doesn't seem to have any other users in-tree but it is a published API). > } > > /* Deletes 'node' from 'sh'. Neither the node's name nor its data is freed; > diff --git a/lib/sset.c b/lib/sset.c > index aa1790020..81cacefc1 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 != NULL); > char *copy = xstrdup(name); > sset_delete(set, SSET_NODE_FROM_NAME(name)); > return copy; > diff --git a/ovsdb/condition.c b/ovsdb/condition.c > index d0016fa7f..ada8f50dc 100644 > --- a/ovsdb/condition.c > +++ b/ovsdb/condition.c > @@ -47,7 +47,15 @@ ovsdb_clause_from_json(const struct ovsdb_table_schema *ts, > > /* Column and arg fields are not being used with boolean functions. > * Use dummy values */ > - clause->column = ovsdb_table_schema_get_column(ts, "_uuid"); > + const struct ovsdb_column *uuid_column = > + ovsdb_table_schema_get_column(ts, "_uuid"); > + if (uuid_column) { > + clause->column = uuid_column; > + } else { > + return ovsdb_syntax_error(json, > + "unknown column", > + "No column _uuid in table schema."); > + } > clause->index = clause->column->index; > ovsdb_datum_init_default(&clause->arg, &clause->column->type); > return NULL; > diff --git a/ovsdb/file.c b/ovsdb/file.c > index 2d887e53e..85de6295a 100644 > --- a/ovsdb/file.c > +++ b/ovsdb/file.c > @@ -522,9 +522,16 @@ ovsdb_file_txn_add_row(struct ovsdb_file_txn *ftxn, > } > > if (row) { > - struct ovsdb_table *table = new ? new->table : old->table; > + struct ovsdb_table *table = NULL; > + if (new) { > + table = new->table; > + } else if (old) { > + table = old->table; > + } > char uuid[UUID_LEN + 1]; > > + ovs_assert(table != NULL); > + > if (table != ftxn->table) { > /* Create JSON object for transaction overall. */ > if (!ftxn->json) { > @@ -538,9 +545,15 @@ ovsdb_file_txn_add_row(struct ovsdb_file_txn *ftxn, > } > > /* Add row to transaction for this table. */ > - snprintf(uuid, sizeof uuid, > - UUID_FMT, UUID_ARGS(ovsdb_row_get_uuid(new ? new : old))); > - json_object_put(ftxn->table_json, uuid, row); > + if (new) { > + snprintf(uuid, sizeof uuid, > + UUID_FMT, UUID_ARGS(ovsdb_row_get_uuid(new))); > + json_object_put(ftxn->table_json, uuid, row); > + } else if (old) { > + snprintf(uuid, sizeof uuid, > + UUID_FMT, UUID_ARGS(ovsdb_row_get_uuid(old))); > + json_object_put(ftxn->table_json, uuid, row); > + } > } > } > > diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c > index 17868f5b7..a3ca48a7b 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); > } > @@ -1131,6 +1131,8 @@ static void > ovsdb_jsonrpc_trigger_create(struct ovsdb_jsonrpc_session *s, struct ovsdb *db, > struct jsonrpc_msg *request) > { > + 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 191befcae..7d70a39b9 100644 > --- a/ovsdb/monitor.c > +++ b/ovsdb/monitor.c > @@ -478,7 +478,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) { > return column->name; > @@ -781,11 +783,15 @@ ovsdb_monitor_table_condition_update( > const struct json *cond_json) > { > if (!condition) { > - return NULL; > + return ovsdb_syntax_error(cond_json, NULL, > + "Parse error, condition empty."); > } > > 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); > > @@ -1279,6 +1285,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 != NULL); > mt->select |= select; > } > > @@ -1329,8 +1336,23 @@ ovsdb_monitor_changes_update(const struct ovsdb_row *old, > const struct ovsdb_monitor_table *mt, > struct ovsdb_monitor_change_set_for_table *mcst) > { > - const struct uuid *uuid = ovsdb_row_get_uuid(new ? new : old); > - struct ovsdb_monitor_row *change; > + const struct uuid *uuid = NULL; > + > + if (!new && !old) { > + return; > + } else { > + if (new) { > + uuid = ovsdb_row_get_uuid(new); > + } else if (old) { > + uuid = ovsdb_row_get_uuid(old); > + } > + } > + > + if (!uuid) { > + return; > + } > + > + struct ovsdb_monitor_row *change = NULL; > > change = ovsdb_monitor_changes_row_find(mcst, uuid); > if (!change) { > @@ -1657,9 +1679,15 @@ ovsdb_monitor_hash(const struct ovsdb_monitor *dbmon, size_t basis) > nodes = shash_sort(&dbmon->tables); > n = shash_count(&dbmon->tables); > > + if (!nodes) { > + return basis; > + } > + > for (i = 0; i < n; i++) { > struct ovsdb_monitor_table *mt = nodes[i]->data; > > + ovs_assert(mt != NULL); > + > basis = hash_pointer(mt->table, basis); > basis = hash_3words(mt->select, mt->n_columns, basis); > > diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c > index f1b8d6491..a93913ed5 100644 > --- a/ovsdb/ovsdb-client.c > +++ b/ovsdb/ovsdb-client.c > @@ -1223,17 +1223,26 @@ parse_monitor_columns(char *arg, const char *server, const char *database, > > n = shash_count(&table->columns); > nodes = shash_sort(&table->columns); > - for (i = 0; i < n; i++) { > - const struct ovsdb_column *column = nodes[i]->data; > - if (column->index != OVSDB_COL_UUID > - && column->index != OVSDB_COL_VERSION) { > - add_column(server, column, columns, columns_json); > + > + if (nodes) { > + for (i = 0; i < n; i++) { > + const struct ovsdb_column *column = nodes[i]->data; > + if (column->index != OVSDB_COL_UUID > + && column->index != OVSDB_COL_VERSION) { > + add_column(server, column, columns, columns_json); > + } > } > + free(nodes); > } > - free(nodes); > > - add_column(server, ovsdb_table_schema_get_column(table, "_version"), > - columns, columns_json); > + const struct ovsdb_column *version_column = > + ovsdb_table_schema_get_column(table, "_version"); > + > + if (version_column) { > + add_column(server, version_column, columns, columns_json); > + } else { > + VLOG_ERR("Table does not contain _version column."); > + } > } > > if (!initial || !insert || !delete || !modify) { > @@ -1439,14 +1448,16 @@ do_monitor__(struct jsonrpc *rpc, const char *database, > ovs_fatal(0, "ALL tables are not allowed with condition"); > } > > - for (i = 0; i < n; i++) { > - struct ovsdb_table_schema *table = nodes[i]->data; > + if (nodes) { > + for (i = 0; i < n; i++) { > + struct ovsdb_table_schema *table = nodes[i]->data; > > - add_monitored_table(argc, argv, server, database, NULL, table, > - monitor_requests, > - &mts, &n_mts, &allocated_mts); > + add_monitored_table(argc, argv, server, database, NULL, table, > + monitor_requests, > + &mts, &n_mts, &allocated_mts); > + } > + free(nodes); > } > - free(nodes); > } > > send_db_change_aware(rpc); > @@ -1870,6 +1881,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++) { > @@ -1929,8 +1944,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/ovsdb-server.c b/ovsdb/ovsdb-server.c > index 33ca4910d..69b151548 100644 > --- a/ovsdb/ovsdb-server.c > +++ b/ovsdb/ovsdb-server.c > @@ -1784,14 +1784,17 @@ ovsdb_server_list_databases(struct unixctl_conn *conn, int argc OVS_UNUSED, > ds_init(&s); > > nodes = shash_sort(all_dbs); > - for (i = 0; i < shash_count(all_dbs); i++) { > - const struct shash_node *node = nodes[i]; > - struct db *db = node->data; > - if (db->db) { > - ds_put_format(&s, "%s\n", node->name); > + > + if (nodes) { > + for (i = 0; i < shash_count(all_dbs); i++) { > + const struct shash_node *node = nodes[i]; > + struct db *db = node->data; > + if (db->db) { > + ds_put_format(&s, "%s\n", node->name); > + } > } > + free(nodes); > } > - free(nodes); > > unixctl_command_reply(conn, ds_cstr(&s)); > ds_destroy(&s); > @@ -2191,6 +2194,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-util.c b/ovsdb/ovsdb-util.c > index 303191dc8..148230da8 100644 > --- a/ovsdb/ovsdb-util.c > +++ b/ovsdb/ovsdb-util.c > @@ -291,6 +291,15 @@ ovsdb_util_write_string_string_column(struct ovsdb_row *row, > size_t i; > > column = ovsdb_table_schema_get_column(row->table->schema, column_name); > + if (!column) { > + VLOG_ERR("No %s column present in the %s table", > + column_name, row->table->schema->name); > + for (i = 0; i < n; i++) { > + free(keys[i]); > + free(values[i]); > + } > + return; > + } > datum = ovsdb_util_get_datum(row, column_name, OVSDB_TYPE_STRING, > OVSDB_TYPE_STRING, UINT_MAX); > if (!datum) { > diff --git a/ovsdb/ovsdb.c b/ovsdb/ovsdb.c > index afec96264..8152f9e41 100644 > --- a/ovsdb/ovsdb.c > +++ b/ovsdb/ovsdb.c > @@ -39,6 +39,7 @@ > #include "transaction.h" > #include "transaction-forward.h" > #include "trigger.h" > +#include "util.h" > > #include "openvswitch/vlog.h" > VLOG_DEFINE_THIS_MODULE(ovsdb); > @@ -195,7 +196,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; > @@ -215,6 +216,9 @@ ovsdb_schema_from_json(const struct json *json, struct ovsdb_schema **schemap) > return error; > } > > + ovs_assert(name != NULL); > + ovs_assert(tables != NULL); > + > if (version_json) { > version = json_string(version_json); > if (!ovsdb_is_valid_version(version)) { > @@ -248,6 +252,8 @@ ovsdb_schema_from_json(const struct json *json, struct ovsdb_schema **schemap) > shash_add(&schema->tables, table->name, table); > } > > + ovs_assert(schema != NULL); > + > /* "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..b4f818b41 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 != NULL); > 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/replication.c b/ovsdb/replication.c > index 477c69d70..da82e08c5 100644 > --- a/ovsdb/replication.c > +++ b/ovsdb/replication.c > @@ -35,6 +35,7 @@ > #include "table.h" > #include "transaction.h" > #include "uuid.h" > +#include "util.h" > > VLOG_DEFINE_THIS_MODULE(replication); > > @@ -575,15 +576,17 @@ create_monitor_request(struct ovsdb_schema *schema) > size_t n = shash_count(&schema->tables); > const struct shash_node **nodes = shash_sort(&schema->tables); > > - for (int j = 0; j < n; j++) { > - struct ovsdb_table_schema *table = nodes[j]->data; > + if (nodes) { > + for (int j = 0; j < n; j++) { > + struct ovsdb_table_schema *table = nodes[j]->data; > > - /* Monitor all tables not excluded. */ > - if (!excluded_tables_find(db_name, table->name)) { > - add_monitored_table(table, monitor_request); > + /* Monitor all tables not excluded. */ > + if (!excluded_tables_find(db_name, table->name)) { > + add_monitored_table(table, monitor_request); > + } > } > + free(nodes); > } > - free(nodes); > > /* Create a monitor request. */ > monitor = json_array_create_3( > 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) { > + set->rows[set->n_rows++] = row; > + } > } > > struct json * > diff --git a/ovsdb/table.c b/ovsdb/table.c > index 66071ce2f..990d49ea4 100644 > --- a/ovsdb/table.c > +++ b/ovsdb/table.c > @@ -158,7 +158,7 @@ ovsdb_table_schema_from_json(const struct json *json, const char *name, > n_max_rows = UINT_MAX; > } > > - if (shash_is_empty(json_object(columns))) { > + if (!columns || shash_is_empty(json_object(columns))) { > return ovsdb_syntax_error(json, NULL, > "table must have at least one column"); > } > diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c > index 03541af85..72a6b29cb 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 != NULL); > 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 2f5ac1a26..baa308661 100644 > --- a/utilities/ovs-vsctl.c > +++ b/utilities/ovs-vsctl.c > @@ -334,6 +334,7 @@ parse_options(int argc, char *argv[], struct shash *local_options) > exit(EXIT_SUCCESS); > > case 't': > + ovs_assert(optarg != NULL); > if (!str_to_uint(optarg, 10, &timeout) || !timeout) { > ctl_fatal("value %s on -t or --timeout is invalid", optarg); > } > @@ -349,10 +350,12 @@ parse_options(int argc, char *argv[], struct shash *local_options) > STREAM_SSL_OPTION_HANDLERS > > case OPT_PEER_CA_CERT: > + ovs_assert(optarg != NULL); > stream_ssl_set_peer_ca_cert_file(optarg); > break; > > case OPT_BOOTSTRAP_CA_CERT: > + ovs_assert(optarg != NULL); > stream_ssl_set_ca_cert_file(optarg, true); > break; > > @@ -575,7 +578,7 @@ add_bridge_to_cache(struct vsctl_context *vsctl_ctx, > struct ovsrec_bridge *br_cfg, const char *name, > struct vsctl_bridge *parent, int vlan) > { > - struct vsctl_bridge *br = xmalloc(sizeof *br); > + struct vsctl_bridge *br = xzalloc(sizeof *br); > br->br_cfg = br_cfg; > br->name = xstrdup(name); > ovs_list_init(&br->ports); > @@ -659,7 +662,7 @@ static struct vsctl_port * > add_port_to_cache(struct vsctl_context *vsctl_ctx, struct vsctl_bridge *parent, > struct ovsrec_port *port_cfg) > { > - struct vsctl_port *port; > + struct vsctl_port *port = xzalloc(sizeof *port); > > if (port_cfg->tag > && *port_cfg->tag >= 0 && *port_cfg->tag <= 4095) { > @@ -671,7 +674,7 @@ add_port_to_cache(struct vsctl_context *vsctl_ctx, struct vsctl_bridge *parent, > } > } > > - port = xmalloc(sizeof *port); > + ovs_assert(parent != NULL); > ovs_list_push_back(&parent->ports, &port->ports_node); > ovs_list_init(&port->ifaces); > port->port_cfg = port_cfg; > @@ -818,6 +821,7 @@ vsctl_context_populate_cache(struct ctl_context *ctx) > continue; > } > br = shash_find_data(&vsctl_ctx->bridges, br_cfg->name); > + ovs_assert(br != NULL); > for (j = 0; j < br_cfg->n_ports; j++) { > struct ovsrec_port *port_cfg = br_cfg->ports[j]; > struct vsctl_port *port; > @@ -844,6 +848,7 @@ vsctl_context_populate_cache(struct ctl_context *ctx) > } > > port = add_port_to_cache(vsctl_ctx, br, port_cfg); > + ovs_assert(port != NULL); > for (k = 0; k < port_cfg->n_interfaces; k++) { > struct ovsrec_interface *iface_cfg = port_cfg->interfaces[k]; > struct vsctl_iface *iface; > @@ -889,14 +894,23 @@ check_conflicts(struct vsctl_context *vsctl_ctx, const char *name, > > port = shash_find_data(&vsctl_ctx->ports, name); > if (port) { > - ctl_fatal("%s because a port named %s already exists on " > - "bridge %s", msg, name, port->bridge->name); > + if (port->bridge) { > + ctl_fatal("%s because a port named %s already exists on " > + "bridge %s", msg, name, port->bridge->name); > + } else { > + ctl_fatal("%s because a port named %s already exists", msg, name); > + } > } > > iface = shash_find_data(&vsctl_ctx->ifaces, name); > if (iface) { > - ctl_fatal("%s because an interface named %s already exists " > - "on bridge %s", msg, name, iface->port->bridge->name); > + if (iface->port->bridge) { > + ctl_fatal("%s because an interface named %s already exists " > + "on bridge %s", msg, name, iface->port->bridge->name); > + } else { > + ctl_fatal("%s because an interface named %s already exists", msg, > + name); > + } > } > > free(msg); > @@ -936,7 +950,7 @@ find_port(struct vsctl_context *vsctl_ctx, const char *name, bool must_exist) > ovs_assert(vsctl_ctx->cache_valid); > > port = shash_find_data(&vsctl_ctx->ports, name); > - if (port && !strcmp(name, port->bridge->name)) { > + if (port && port->bridge && !strcmp(name, port->bridge->name)) { > port = NULL; > } > if (must_exist && !port) { > @@ -954,7 +968,8 @@ find_iface(struct vsctl_context *vsctl_ctx, const char *name, bool must_exist) > ovs_assert(vsctl_ctx->cache_valid); > > iface = shash_find_data(&vsctl_ctx->ifaces, name); > - if (iface && !strcmp(name, iface->port->bridge->name)) { > + if (iface && iface->port->bridge && > + !strcmp(name, iface->port->bridge->name)) { > iface = NULL; > } > if (must_exist && !iface) { > @@ -1679,14 +1694,16 @@ get_external_id(struct smap *smap, const char *prefix, const char *key, > size_t prefix_len = strlen(prefix); > size_t i; > > - for (i = 0; i < smap_count(smap); i++) { > - const struct smap_node *node = sorted[i]; > - if (!strncmp(node->key, prefix, prefix_len)) { > - ds_put_format(output, "%s=%s\n", node->key + prefix_len, > - node->value); > + if (sorted) { > + for (i = 0; i < smap_count(smap); i++) { > + const struct smap_node *node = sorted[i]; > + if (!strncmp(node->key, prefix, prefix_len)) { > + ds_put_format(output, "%s=%s\n", node->key + prefix_len, > + node->value); > + } > } > + free(sorted); > } > - free(sorted); > } > } > > diff --git a/vtep/vtep-ctl.c b/vtep/vtep-ctl.c > index e5d99714d..765355726 100644 > --- a/vtep/vtep-ctl.c > +++ b/vtep/vtep-ctl.c > @@ -250,6 +250,7 @@ parse_options(int argc, char *argv[], struct shash *local_options) > exit(EXIT_SUCCESS); > > case 't': > + ovs_assert(optarg != NULL); I wonder how coverity's model broke here. It seems to inconsistently flag the optarg tests? I didn't check every last one but it does seem that some which are marked "required_arg" should should be generating the correct flags for getopt are skipped and some aren't. Maybe this needs some additional investigation? > if (!str_to_uint(optarg, 10, &timeout) || !timeout) { > ctl_fatal("value %s on -t or --timeout is invalid", optarg); > } > @@ -1065,6 +1066,7 @@ vtep_ctl_context_populate_cache(struct ctl_context *ctx) > continue; > } > ps = shash_find_data(&vtepctl_ctx->pswitches, ps_cfg->name); > + ovs_assert(ps != NULL); > 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 +1089,7 @@ vtep_ctl_context_populate_cache(struct ctl_context *ctx) > } > > port = add_port_to_cache(vtepctl_ctx, ps, port_cfg); > - > + ovs_assert(port != NULL); > for (k = 0; k < port_cfg->n_vlan_bindings; k++) { > struct vtep_ctl_lswitch *ls; > char *vlan; > @@ -1892,8 +1894,10 @@ del_mcast_entry(struct ctl_context *ctx, > vteprec_mcast_macs_remote_delete(mcast_mac->remote_cfg); > } > > - free(node->data); > - shash_delete(mcast_shash, node); > + if (node) { > + free(node->data); > + shash_delete(mcast_shash, node); > + } Instead, consider using an ovs_assert() where the code calls shash_find(), which should make reading this a bit more pleasant. > } else { > if (local) { > vteprec_mcast_macs_local_set_locator_set(mcast_mac->local_cfg,
Hi Aaron, On Mon, Apr 3, 2023 at 10:15 PM Aaron Conole <aconole@redhat.com> wrote: > > Sorry to comment on this so late - one thing I would also say can help > with review is to separate the logical changes. For example, each flag > from Coverity could be a separate patch in a cleanup series. It could > help to keep things organized. Comments to follow. > Sure thing. I will split this into a patch series and I will group similar changes into separate patches. > > There's a bit of intermingling of null checks here. Please stick to > one style. Throughout the dp-packet.c file, there are positive tests > (ie 'ovs_assert(data_dp)' would match the style more). > Got it. I will change null checks in this patch series to positive tests. > > Probably better to check this before we use dp_packet_data() in a > calculation. It seems odd to check after we've set up dst. > Sure. > > Rather than another level of indentation here, can we not change the > test condition to be: > > for(size_t j = 0; (nodes && > j < smap_count(&config)); j++) { > } > > Similar comment in other places. > > An alternative is to change smap_count() to return '0' if smap is NULL, > or assert there (and we can do this for shash as well). > I will use the approach of changing `smap_count()` to return `0` if `smap_is_empty`. I will do that for `shash` and `simap` as well and update the relevant changes. > > This can be expressed closer to the first assert in the function: > > assert(dp_packet_data(buf)); > > Or possibly(? untested) change the assert from dp_packet_is_eth to > assert(dp_packet_eth(buf)); > I have tested it and changing the assert from `dp_packet_is_eth` to `assert(dp_packet_eth(buf))` would not work and would break some tests. I will move the `assert(dp_packet_data(buf))` nearer to the first assert instead. > > Can we instead change the ofpbuf_at to ofpbuf_at_assert() ? > Sure. > > Same here > Sure. > > Maybe instead of this, we should change the SFL_ALLOC define to be > xzalloc which will guarantee a zero'd buffer that is != NULL and simply > leave the assert. > Sure, will implement this in the next version of the patch. > > Indentation on this is wrong. > Got it, will fix it in the next version. > > Same here. > Got it. > > Consider we should move this type of check to shash steal, or possibly > change shash_steal to assert node != NULL (although, shash_steal doesn't > seem to have any other users in-tree but it is a published API). > Sure. I will do a check instead of an assert since `shash_steal` is a published API. > > I wonder how coverity's model broke here. It seems to inconsistently > flag the optarg tests? I didn't check every last one but it does seem > that some which are marked "required_arg" should should be generating > the correct flags for getopt are skipped and some aren't. Maybe this > needs some additional investigation? > Hmm, I will not include these assertions for `optarg` for this patch series. Further investigation could be done separately, and if these are false positives, we can indicate them as so to Coverity. > > Instead, consider using an ovs_assert() where the code calls > shash_find(), which should make reading this a bit more pleasant. > Sure. Best regards, James Raphael Tiovalen
diff --git a/lib/dp-packet.c b/lib/dp-packet.c index ae8ab5800..f9c58a72f 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 != NULL); return dp_packet_clone_with_headroom(buffer, 0); } @@ -183,9 +184,12 @@ dp_packet_clone_with_headroom(const struct dp_packet *buffer, size_t headroom) struct dp_packet *new_buffer; uint32_t mark; - new_buffer = dp_packet_clone_data_with_headroom(dp_packet_data(buffer), - dp_packet_size(buffer), - headroom); + const void *data_dp = dp_packet_data(buffer); + ovs_assert(data_dp != NULL); + + new_buffer = dp_packet_clone_data_with_headroom(data_dp, + dp_packet_size(buffer), + headroom); /* Copy the following fields into the returned buffer: l2_pad_size, * l2_5_ofs, l3_ofs, l4_ofs, cutlen, packet_type and md. */ memcpy(&new_buffer->l2_pad_size, &buffer->l2_pad_size, @@ -323,7 +327,9 @@ dp_packet_shift(struct dp_packet *b, int delta) if (delta != 0) { char *dst = (char *) dp_packet_data(b) + delta; - memmove(dst, dp_packet_data(b), dp_packet_size(b)); + const void *data_dp = dp_packet_data(b); + ovs_assert(data_dp != NULL); + memmove(dst, data_dp, dp_packet_size(b)); dp_packet_set_data(b, dst); } } @@ -348,7 +354,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; } @@ -359,7 +365,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; } @@ -431,7 +437,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; } @@ -442,7 +448,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 c501a0cd7..1833d3ba3 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 != NULL); + 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 != NULL); + if (!strcmp(key, "type")) { if (strcmp(value, type)) { dpctl_error(dpctl_p, 0, @@ -693,12 +697,14 @@ show_dpif(struct dpif *dpif, struct dpctl_params *dpctl_p) error = netdev_get_config(netdev, &config); if (!error) { const struct smap_node **nodes = smap_sort(&config); - for (size_t j = 0; j < smap_count(&config); j++) { - const struct smap_node *node = nodes[j]; - dpctl_print(dpctl_p, "%c %s=%s", j ? ',' : ':', - node->key, node->value); + if (nodes) { + for (size_t j = 0; j < smap_count(&config); j++) { + const struct smap_node *node = nodes[j]; + dpctl_print(dpctl_p, "%c %s=%s", j ? ',' : ':', + node->key, node->value); + } + free(nodes); } - free(nodes); } else { dpctl_print(dpctl_p, ", could not retrieve configuration " "(%s)", ovs_strerror(error)); diff --git a/lib/json.c b/lib/json.c index aded8bb01..11471f42a 100644 --- a/lib/json.c +++ b/lib/json.c @@ -498,12 +498,15 @@ json_hash_object(const struct shash *object, size_t basis) nodes = shash_sort(object); n = shash_count(object); - for (i = 0; i < n; i++) { - const struct shash_node *node = nodes[i]; - basis = hash_string(node->name, basis); - basis = json_hash(node->data, basis); + + if (nodes) { + for (i = 0; i < n; i++) { + const struct shash_node *node = nodes[i]; + basis = hash_string(node->name, basis); + basis = json_hash(node->data, basis); + } + free(nodes); } - free(nodes); return basis; } @@ -1655,10 +1658,13 @@ json_serialize_object(const struct shash *object, struct json_serializer *s) nodes = shash_sort(object); n = shash_count(object); - for (i = 0; i < n; i++) { - json_serialize_object_member(i, nodes[i], s); + + if (nodes) { + for (i = 0; i < n; i++) { + json_serialize_object_member(i, nodes[i], s); + } + free(nodes); } - free(nodes); } else { struct shash_node *node; size_t i; diff --git a/lib/latch-unix.c b/lib/latch-unix.c index f4d10c39a..262f111e4 100644 --- a/lib/latch-unix.c +++ b/lib/latch-unix.c @@ -71,7 +71,7 @@ latch_set(struct latch *latch) bool latch_is_set(const struct latch *latch) { - struct pollfd pfd; + struct pollfd pfd = {0, 0, 0}; int retval; pfd.fd = latch->fds[0]; diff --git a/lib/memory.c b/lib/memory.c index da97476c6..d3bd8c295 100644 --- a/lib/memory.c +++ b/lib/memory.c @@ -24,6 +24,7 @@ #include "simap.h" #include "timeval.h" #include "unixctl.h" +#include "util.h" #include "openvswitch/vlog.h" VLOG_DEFINE_THIS_MODULE(memory); @@ -116,13 +117,14 @@ compose_report(const struct simap *usage, struct ds *s) size_t n = simap_count(usage); size_t i; - for (i = 0; i < n; i++) { - const struct simap_node *node = nodes[i]; - - ds_put_format(s, "%s:%u ", node->name, node->data); + if (nodes) { + for (i = 0; i < n; i++) { + const struct simap_node *node = nodes[i]; + ds_put_format(s, "%s:%u ", node->name, node->data); + } + free(nodes); } ds_chomp(s, ' '); - free(nodes); } /* Logs the contents of 'usage', as a collection of name-count pairs. diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c index b89dfdd52..b2283988b 100644 --- a/lib/netdev-native-tnl.c +++ b/lib/netdev-native-tnl.c @@ -43,6 +43,7 @@ #include "seq.h" #include "unaligned.h" #include "unixctl.h" +#include "util.h" #include "openvswitch/vlog.h" VLOG_DEFINE_THIS_MODULE(native_tnl); @@ -221,12 +222,13 @@ netdev_tnl_calc_udp_csum(struct udp_header *udp, struct dp_packet *packet, { uint32_t csum; - if (netdev_tnl_is_header_ipv6(dp_packet_data(packet))) { - csum = packet_csum_pseudoheader6(netdev_tnl_ipv6_hdr( - dp_packet_data(packet))); + void *data_dp = dp_packet_data(packet); + ovs_assert(data_dp != NULL); + + if (netdev_tnl_is_header_ipv6(data_dp)) { + csum = packet_csum_pseudoheader6(netdev_tnl_ipv6_hdr(data_dp)); } else { - csum = packet_csum_pseudoheader(netdev_tnl_ip_hdr( - dp_packet_data(packet))); + csum = packet_csum_pseudoheader(netdev_tnl_ip_hdr(data_dp)); } csum = csum_continue(csum, udp, ip_tot_size); @@ -425,7 +427,10 @@ netdev_gre_pop_header(struct dp_packet *packet) struct flow_tnl *tnl = &md->tunnel; int hlen = sizeof(struct eth_header) + 4; - hlen += netdev_tnl_is_header_ipv6(dp_packet_data(packet)) ? + const void *data_dp = dp_packet_data(packet); + ovs_assert(data_dp != NULL); + + hlen += netdev_tnl_is_header_ipv6(data_dp) ? IPV6_HEADER_LEN : IP_HEADER_LEN; pkt_metadata_init_tnl(md); diff --git a/lib/odp-execute.c b/lib/odp-execute.c index 5cf6fbec0..97f950c60 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 != NULL); + 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 != NULL); + if (!mask) { arp->ar_op = key->arp_op; arp->ar_sha = key->arp_sha; diff --git a/lib/pcap-file.c b/lib/pcap-file.c index 3ed7ea488..bedfaae14 100644 --- a/lib/pcap-file.c +++ b/lib/pcap-file.c @@ -291,7 +291,9 @@ ovs_pcap_write(struct pcap_file *p_file, struct dp_packet *buf) prh.incl_len = dp_packet_size(buf); prh.orig_len = dp_packet_size(buf); ignore(fwrite(&prh, sizeof prh, 1, p_file->file)); - ignore(fwrite(dp_packet_data(buf), dp_packet_size(buf), 1, p_file->file)); + const void *data_dp = dp_packet_data(buf); + ovs_assert(data_dp != NULL); + ignore(fwrite(data_dp, dp_packet_size(buf), 1, p_file->file)); fflush(p_file->file); } diff --git a/lib/rtnetlink.c b/lib/rtnetlink.c index f67352603..d2b2f85af 100644 --- a/lib/rtnetlink.c +++ b/lib/rtnetlink.c @@ -26,6 +26,7 @@ #include "netlink-notifier.h" #include "openvswitch/ofpbuf.h" #include "packets.h" +#include "util.h" #if IFLA_INFO_MAX < 5 #define IFLA_INFO_SLAVE_KIND 4 @@ -131,6 +132,8 @@ rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change) change->irrelevant = true; } + ovs_assert(ifinfo != NULL); + change->nlmsg_type = nlmsg->nlmsg_type; change->if_index = ifinfo->ifi_index; change->ifname = nl_attr_get_string(attrs[IFLA_IFNAME]); @@ -177,6 +180,8 @@ rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change) ifaddr = ofpbuf_at(buf, NLMSG_HDRLEN, sizeof *ifaddr); + ovs_assert(ifaddr != NULL); + change->nlmsg_type = nlmsg->nlmsg_type; change->if_index = ifaddr->ifa_index; change->ifname = (attrs[IFA_LABEL] diff --git a/lib/sflow_agent.c b/lib/sflow_agent.c index c95f654a5..2199b52a5 100644 --- a/lib/sflow_agent.c +++ b/lib/sflow_agent.c @@ -153,6 +153,8 @@ void sfl_agent_tick(SFLAgent *agent, time_t now) SFLReceiver *sfl_agent_addReceiver(SFLAgent *agent) { SFLReceiver *rcv = (SFLReceiver *)sflAlloc(agent, sizeof(SFLReceiver)); + ovs_assert(rcv != NULL); + memset(rcv, 0, sizeof(*rcv)); sfl_receiver_init(rcv, agent); /* add to end of list - to preserve the receiver index numbers for existing receivers */ { @@ -203,6 +205,8 @@ SFLSampler *sfl_agent_addSampler(SFLAgent *agent, SFLDataSource_instance *pdsi) { SFLSampler *newsm = (SFLSampler *)sflAlloc(agent, sizeof(SFLSampler)); + ovs_assert(newsm != NULL); + memset(newsm, 0, sizeof(*newsm)); sfl_sampler_init(newsm, agent, pdsi); if(prev) prev->nxt = newsm; else agent->samplers = newsm; @@ -242,6 +246,8 @@ SFLPoller *sfl_agent_addPoller(SFLAgent *agent, /* either we found the insert point, or reached the end of the list... */ { SFLPoller *newpl = (SFLPoller *)sflAlloc(agent, sizeof(SFLPoller)); + ovs_assert(newpl != NULL); + memset(newpl, 0, sizeof(*newpl)); sfl_poller_init(newpl, agent, pdsi, magic, getCountersFn); if(prev) prev->nxt = newpl; else agent->pollers = newpl; diff --git a/lib/shash.c b/lib/shash.c index a7b2c6458..070bc3e82 100644 --- a/lib/shash.c +++ b/lib/shash.c @@ -17,6 +17,7 @@ #include <config.h> #include "openvswitch/shash.h" #include "hash.h" +#include "util.h" static struct shash_node *shash_find__(const struct shash *, const char *name, size_t name_len, @@ -194,7 +195,9 @@ shash_replace_nocopy(struct shash *sh, char *name, const void *data) void shash_delete(struct shash *sh, struct shash_node *node) { - free(shash_steal(sh, node)); + if (node) { + free(shash_steal(sh, node)); + } } /* Deletes 'node' from 'sh'. Neither the node's name nor its data is freed; diff --git a/lib/sset.c b/lib/sset.c index aa1790020..81cacefc1 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 != NULL); char *copy = xstrdup(name); sset_delete(set, SSET_NODE_FROM_NAME(name)); return copy; diff --git a/ovsdb/condition.c b/ovsdb/condition.c index d0016fa7f..ada8f50dc 100644 --- a/ovsdb/condition.c +++ b/ovsdb/condition.c @@ -47,7 +47,15 @@ ovsdb_clause_from_json(const struct ovsdb_table_schema *ts, /* Column and arg fields are not being used with boolean functions. * Use dummy values */ - clause->column = ovsdb_table_schema_get_column(ts, "_uuid"); + const struct ovsdb_column *uuid_column = + ovsdb_table_schema_get_column(ts, "_uuid"); + if (uuid_column) { + clause->column = uuid_column; + } else { + return ovsdb_syntax_error(json, + "unknown column", + "No column _uuid in table schema."); + } clause->index = clause->column->index; ovsdb_datum_init_default(&clause->arg, &clause->column->type); return NULL; diff --git a/ovsdb/file.c b/ovsdb/file.c index 2d887e53e..85de6295a 100644 --- a/ovsdb/file.c +++ b/ovsdb/file.c @@ -522,9 +522,16 @@ ovsdb_file_txn_add_row(struct ovsdb_file_txn *ftxn, } if (row) { - struct ovsdb_table *table = new ? new->table : old->table; + struct ovsdb_table *table = NULL; + if (new) { + table = new->table; + } else if (old) { + table = old->table; + } char uuid[UUID_LEN + 1]; + ovs_assert(table != NULL); + if (table != ftxn->table) { /* Create JSON object for transaction overall. */ if (!ftxn->json) { @@ -538,9 +545,15 @@ ovsdb_file_txn_add_row(struct ovsdb_file_txn *ftxn, } /* Add row to transaction for this table. */ - snprintf(uuid, sizeof uuid, - UUID_FMT, UUID_ARGS(ovsdb_row_get_uuid(new ? new : old))); - json_object_put(ftxn->table_json, uuid, row); + if (new) { + snprintf(uuid, sizeof uuid, + UUID_FMT, UUID_ARGS(ovsdb_row_get_uuid(new))); + json_object_put(ftxn->table_json, uuid, row); + } else if (old) { + snprintf(uuid, sizeof uuid, + UUID_FMT, UUID_ARGS(ovsdb_row_get_uuid(old))); + json_object_put(ftxn->table_json, uuid, row); + } } } diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c index 17868f5b7..a3ca48a7b 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); } @@ -1131,6 +1131,8 @@ static void ovsdb_jsonrpc_trigger_create(struct ovsdb_jsonrpc_session *s, struct ovsdb *db, struct jsonrpc_msg *request) { + 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 191befcae..7d70a39b9 100644 --- a/ovsdb/monitor.c +++ b/ovsdb/monitor.c @@ -478,7 +478,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) { return column->name; @@ -781,11 +783,15 @@ ovsdb_monitor_table_condition_update( const struct json *cond_json) { if (!condition) { - return NULL; + return ovsdb_syntax_error(cond_json, NULL, + "Parse error, condition empty."); } 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); @@ -1279,6 +1285,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 != NULL); mt->select |= select; } @@ -1329,8 +1336,23 @@ ovsdb_monitor_changes_update(const struct ovsdb_row *old, const struct ovsdb_monitor_table *mt, struct ovsdb_monitor_change_set_for_table *mcst) { - const struct uuid *uuid = ovsdb_row_get_uuid(new ? new : old); - struct ovsdb_monitor_row *change; + const struct uuid *uuid = NULL; + + if (!new && !old) { + return; + } else { + if (new) { + uuid = ovsdb_row_get_uuid(new); + } else if (old) { + uuid = ovsdb_row_get_uuid(old); + } + } + + if (!uuid) { + return; + } + + struct ovsdb_monitor_row *change = NULL; change = ovsdb_monitor_changes_row_find(mcst, uuid); if (!change) { @@ -1657,9 +1679,15 @@ ovsdb_monitor_hash(const struct ovsdb_monitor *dbmon, size_t basis) nodes = shash_sort(&dbmon->tables); n = shash_count(&dbmon->tables); + if (!nodes) { + return basis; + } + for (i = 0; i < n; i++) { struct ovsdb_monitor_table *mt = nodes[i]->data; + ovs_assert(mt != NULL); + basis = hash_pointer(mt->table, basis); basis = hash_3words(mt->select, mt->n_columns, basis); diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c index f1b8d6491..a93913ed5 100644 --- a/ovsdb/ovsdb-client.c +++ b/ovsdb/ovsdb-client.c @@ -1223,17 +1223,26 @@ parse_monitor_columns(char *arg, const char *server, const char *database, n = shash_count(&table->columns); nodes = shash_sort(&table->columns); - for (i = 0; i < n; i++) { - const struct ovsdb_column *column = nodes[i]->data; - if (column->index != OVSDB_COL_UUID - && column->index != OVSDB_COL_VERSION) { - add_column(server, column, columns, columns_json); + + if (nodes) { + for (i = 0; i < n; i++) { + const struct ovsdb_column *column = nodes[i]->data; + if (column->index != OVSDB_COL_UUID + && column->index != OVSDB_COL_VERSION) { + add_column(server, column, columns, columns_json); + } } + free(nodes); } - free(nodes); - add_column(server, ovsdb_table_schema_get_column(table, "_version"), - columns, columns_json); + const struct ovsdb_column *version_column = + ovsdb_table_schema_get_column(table, "_version"); + + if (version_column) { + add_column(server, version_column, columns, columns_json); + } else { + VLOG_ERR("Table does not contain _version column."); + } } if (!initial || !insert || !delete || !modify) { @@ -1439,14 +1448,16 @@ do_monitor__(struct jsonrpc *rpc, const char *database, ovs_fatal(0, "ALL tables are not allowed with condition"); } - for (i = 0; i < n; i++) { - struct ovsdb_table_schema *table = nodes[i]->data; + if (nodes) { + for (i = 0; i < n; i++) { + struct ovsdb_table_schema *table = nodes[i]->data; - add_monitored_table(argc, argv, server, database, NULL, table, - monitor_requests, - &mts, &n_mts, &allocated_mts); + add_monitored_table(argc, argv, server, database, NULL, table, + monitor_requests, + &mts, &n_mts, &allocated_mts); + } + free(nodes); } - free(nodes); } send_db_change_aware(rpc); @@ -1870,6 +1881,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++) { @@ -1929,8 +1944,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/ovsdb-server.c b/ovsdb/ovsdb-server.c index 33ca4910d..69b151548 100644 --- a/ovsdb/ovsdb-server.c +++ b/ovsdb/ovsdb-server.c @@ -1784,14 +1784,17 @@ ovsdb_server_list_databases(struct unixctl_conn *conn, int argc OVS_UNUSED, ds_init(&s); nodes = shash_sort(all_dbs); - for (i = 0; i < shash_count(all_dbs); i++) { - const struct shash_node *node = nodes[i]; - struct db *db = node->data; - if (db->db) { - ds_put_format(&s, "%s\n", node->name); + + if (nodes) { + for (i = 0; i < shash_count(all_dbs); i++) { + const struct shash_node *node = nodes[i]; + struct db *db = node->data; + if (db->db) { + ds_put_format(&s, "%s\n", node->name); + } } + free(nodes); } - free(nodes); unixctl_command_reply(conn, ds_cstr(&s)); ds_destroy(&s); @@ -2191,6 +2194,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-util.c b/ovsdb/ovsdb-util.c index 303191dc8..148230da8 100644 --- a/ovsdb/ovsdb-util.c +++ b/ovsdb/ovsdb-util.c @@ -291,6 +291,15 @@ ovsdb_util_write_string_string_column(struct ovsdb_row *row, size_t i; column = ovsdb_table_schema_get_column(row->table->schema, column_name); + if (!column) { + VLOG_ERR("No %s column present in the %s table", + column_name, row->table->schema->name); + for (i = 0; i < n; i++) { + free(keys[i]); + free(values[i]); + } + return; + } datum = ovsdb_util_get_datum(row, column_name, OVSDB_TYPE_STRING, OVSDB_TYPE_STRING, UINT_MAX); if (!datum) { diff --git a/ovsdb/ovsdb.c b/ovsdb/ovsdb.c index afec96264..8152f9e41 100644 --- a/ovsdb/ovsdb.c +++ b/ovsdb/ovsdb.c @@ -39,6 +39,7 @@ #include "transaction.h" #include "transaction-forward.h" #include "trigger.h" +#include "util.h" #include "openvswitch/vlog.h" VLOG_DEFINE_THIS_MODULE(ovsdb); @@ -195,7 +196,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; @@ -215,6 +216,9 @@ ovsdb_schema_from_json(const struct json *json, struct ovsdb_schema **schemap) return error; } + ovs_assert(name != NULL); + ovs_assert(tables != NULL); + if (version_json) { version = json_string(version_json); if (!ovsdb_is_valid_version(version)) { @@ -248,6 +252,8 @@ ovsdb_schema_from_json(const struct json *json, struct ovsdb_schema **schemap) shash_add(&schema->tables, table->name, table); } + ovs_assert(schema != NULL); + /* "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..b4f818b41 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 != NULL); 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/replication.c b/ovsdb/replication.c index 477c69d70..da82e08c5 100644 --- a/ovsdb/replication.c +++ b/ovsdb/replication.c @@ -35,6 +35,7 @@ #include "table.h" #include "transaction.h" #include "uuid.h" +#include "util.h" VLOG_DEFINE_THIS_MODULE(replication); @@ -575,15 +576,17 @@ create_monitor_request(struct ovsdb_schema *schema) size_t n = shash_count(&schema->tables); const struct shash_node **nodes = shash_sort(&schema->tables); - for (int j = 0; j < n; j++) { - struct ovsdb_table_schema *table = nodes[j]->data; + if (nodes) { + for (int j = 0; j < n; j++) { + struct ovsdb_table_schema *table = nodes[j]->data; - /* Monitor all tables not excluded. */ - if (!excluded_tables_find(db_name, table->name)) { - add_monitored_table(table, monitor_request); + /* Monitor all tables not excluded. */ + if (!excluded_tables_find(db_name, table->name)) { + add_monitored_table(table, monitor_request); + } } + free(nodes); } - free(nodes); /* Create a monitor request. */ monitor = json_array_create_3( 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) { + set->rows[set->n_rows++] = row; + } } struct json * diff --git a/ovsdb/table.c b/ovsdb/table.c index 66071ce2f..990d49ea4 100644 --- a/ovsdb/table.c +++ b/ovsdb/table.c @@ -158,7 +158,7 @@ ovsdb_table_schema_from_json(const struct json *json, const char *name, n_max_rows = UINT_MAX; } - if (shash_is_empty(json_object(columns))) { + if (!columns || shash_is_empty(json_object(columns))) { return ovsdb_syntax_error(json, NULL, "table must have at least one column"); } diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c index 03541af85..72a6b29cb 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 != NULL); 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 2f5ac1a26..baa308661 100644 --- a/utilities/ovs-vsctl.c +++ b/utilities/ovs-vsctl.c @@ -334,6 +334,7 @@ parse_options(int argc, char *argv[], struct shash *local_options) exit(EXIT_SUCCESS); case 't': + ovs_assert(optarg != NULL); if (!str_to_uint(optarg, 10, &timeout) || !timeout) { ctl_fatal("value %s on -t or --timeout is invalid", optarg); } @@ -349,10 +350,12 @@ parse_options(int argc, char *argv[], struct shash *local_options) STREAM_SSL_OPTION_HANDLERS case OPT_PEER_CA_CERT: + ovs_assert(optarg != NULL); stream_ssl_set_peer_ca_cert_file(optarg); break; case OPT_BOOTSTRAP_CA_CERT: + ovs_assert(optarg != NULL); stream_ssl_set_ca_cert_file(optarg, true); break; @@ -575,7 +578,7 @@ add_bridge_to_cache(struct vsctl_context *vsctl_ctx, struct ovsrec_bridge *br_cfg, const char *name, struct vsctl_bridge *parent, int vlan) { - struct vsctl_bridge *br = xmalloc(sizeof *br); + struct vsctl_bridge *br = xzalloc(sizeof *br); br->br_cfg = br_cfg; br->name = xstrdup(name); ovs_list_init(&br->ports); @@ -659,7 +662,7 @@ static struct vsctl_port * add_port_to_cache(struct vsctl_context *vsctl_ctx, struct vsctl_bridge *parent, struct ovsrec_port *port_cfg) { - struct vsctl_port *port; + struct vsctl_port *port = xzalloc(sizeof *port); if (port_cfg->tag && *port_cfg->tag >= 0 && *port_cfg->tag <= 4095) { @@ -671,7 +674,7 @@ add_port_to_cache(struct vsctl_context *vsctl_ctx, struct vsctl_bridge *parent, } } - port = xmalloc(sizeof *port); + ovs_assert(parent != NULL); ovs_list_push_back(&parent->ports, &port->ports_node); ovs_list_init(&port->ifaces); port->port_cfg = port_cfg; @@ -818,6 +821,7 @@ vsctl_context_populate_cache(struct ctl_context *ctx) continue; } br = shash_find_data(&vsctl_ctx->bridges, br_cfg->name); + ovs_assert(br != NULL); for (j = 0; j < br_cfg->n_ports; j++) { struct ovsrec_port *port_cfg = br_cfg->ports[j]; struct vsctl_port *port; @@ -844,6 +848,7 @@ vsctl_context_populate_cache(struct ctl_context *ctx) } port = add_port_to_cache(vsctl_ctx, br, port_cfg); + ovs_assert(port != NULL); for (k = 0; k < port_cfg->n_interfaces; k++) { struct ovsrec_interface *iface_cfg = port_cfg->interfaces[k]; struct vsctl_iface *iface; @@ -889,14 +894,23 @@ check_conflicts(struct vsctl_context *vsctl_ctx, const char *name, port = shash_find_data(&vsctl_ctx->ports, name); if (port) { - ctl_fatal("%s because a port named %s already exists on " - "bridge %s", msg, name, port->bridge->name); + if (port->bridge) { + ctl_fatal("%s because a port named %s already exists on " + "bridge %s", msg, name, port->bridge->name); + } else { + ctl_fatal("%s because a port named %s already exists", msg, name); + } } iface = shash_find_data(&vsctl_ctx->ifaces, name); if (iface) { - ctl_fatal("%s because an interface named %s already exists " - "on bridge %s", msg, name, iface->port->bridge->name); + if (iface->port->bridge) { + ctl_fatal("%s because an interface named %s already exists " + "on bridge %s", msg, name, iface->port->bridge->name); + } else { + ctl_fatal("%s because an interface named %s already exists", msg, + name); + } } free(msg); @@ -936,7 +950,7 @@ find_port(struct vsctl_context *vsctl_ctx, const char *name, bool must_exist) ovs_assert(vsctl_ctx->cache_valid); port = shash_find_data(&vsctl_ctx->ports, name); - if (port && !strcmp(name, port->bridge->name)) { + if (port && port->bridge && !strcmp(name, port->bridge->name)) { port = NULL; } if (must_exist && !port) { @@ -954,7 +968,8 @@ find_iface(struct vsctl_context *vsctl_ctx, const char *name, bool must_exist) ovs_assert(vsctl_ctx->cache_valid); iface = shash_find_data(&vsctl_ctx->ifaces, name); - if (iface && !strcmp(name, iface->port->bridge->name)) { + if (iface && iface->port->bridge && + !strcmp(name, iface->port->bridge->name)) { iface = NULL; } if (must_exist && !iface) { @@ -1679,14 +1694,16 @@ get_external_id(struct smap *smap, const char *prefix, const char *key, size_t prefix_len = strlen(prefix); size_t i; - for (i = 0; i < smap_count(smap); i++) { - const struct smap_node *node = sorted[i]; - if (!strncmp(node->key, prefix, prefix_len)) { - ds_put_format(output, "%s=%s\n", node->key + prefix_len, - node->value); + if (sorted) { + for (i = 0; i < smap_count(smap); i++) { + const struct smap_node *node = sorted[i]; + if (!strncmp(node->key, prefix, prefix_len)) { + ds_put_format(output, "%s=%s\n", node->key + prefix_len, + node->value); + } } + free(sorted); } - free(sorted); } } diff --git a/vtep/vtep-ctl.c b/vtep/vtep-ctl.c index e5d99714d..765355726 100644 --- a/vtep/vtep-ctl.c +++ b/vtep/vtep-ctl.c @@ -250,6 +250,7 @@ parse_options(int argc, char *argv[], struct shash *local_options) exit(EXIT_SUCCESS); case 't': + ovs_assert(optarg != NULL); if (!str_to_uint(optarg, 10, &timeout) || !timeout) { ctl_fatal("value %s on -t or --timeout is invalid", optarg); } @@ -1065,6 +1066,7 @@ vtep_ctl_context_populate_cache(struct ctl_context *ctx) continue; } ps = shash_find_data(&vtepctl_ctx->pswitches, ps_cfg->name); + ovs_assert(ps != NULL); 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 +1089,7 @@ vtep_ctl_context_populate_cache(struct ctl_context *ctx) } port = add_port_to_cache(vtepctl_ctx, ps, port_cfg); - + ovs_assert(port != NULL); for (k = 0; k < port_cfg->n_vlan_bindings; k++) { struct vtep_ctl_lswitch *ls; char *vlan; @@ -1892,8 +1894,10 @@ del_mcast_entry(struct ctl_context *ctx, vteprec_mcast_macs_remote_delete(mcast_mac->remote_cfg); } - free(node->data); - shash_delete(mcast_shash, node); + if (node) { + free(node->data); + shash_delete(mcast_shash, node); + } } else { if (local) { vteprec_mcast_macs_local_set_locator_set(mcast_mac->local_cfg,
This commit addresses several high and medium-impact Coverity defects by fixing several possible null-pointer dereferences and potentially uninitialized variables. There were cases when crashes were encountered when some null pointers were dereferenced. Null pointer checks and alternative code paths have been added to prevent unwanted crashes. For impossible conditions, non-null assertions are added. Additionally, some struct variables were not initialized. Zero-initializations have been added to prevent potential data leaks or undefined behavior. Some variables would always be initialized by either the code flow or the compiler and some pointers would never be null. Thus, some Coverity reports might be false positives. That said, it is still considered best practice to perform null pointer checks and initialize variables upfront just in case to ensure the overall resilience and security of OVS, as long as they do not impact performance-critical code. As a bonus, it would also make static analyzer tools, such as Coverity, happy. Unit tests have been successfully executed via `make check` and they successfully passed. Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com> --- v6: - Convert some explicit null pointer checks to assertions since they are checking for impossible conditions. - Use `nullable_memset()` and `nullable_memcpy()` instead of manually performing null checks for `memset()` and `memcpy()`. v5: Improve commit message to better describe the intent of this patch. v4: - Fix some apply-robot checkpatch errors and warnings. - Fix github-robot build failure: linux clang test asan. v3: Fix some apply-robot checkpatch errors and warnings. v2: Fix some apply-robot checkpatch errors and warnings. --- lib/dp-packet.c | 22 ++++++++++++------- lib/dpctl.c | 16 +++++++++----- lib/json.c | 22 ++++++++++++------- lib/latch-unix.c | 2 +- lib/memory.c | 12 ++++++----- lib/netdev-native-tnl.c | 17 +++++++++------ lib/odp-execute.c | 4 ++++ lib/pcap-file.c | 4 +++- lib/rtnetlink.c | 5 +++++ lib/sflow_agent.c | 6 ++++++ lib/shash.c | 5 ++++- lib/sset.c | 2 ++ ovsdb/condition.c | 10 ++++++++- ovsdb/file.c | 21 ++++++++++++++---- ovsdb/jsonrpc-server.c | 6 +++++- ovsdb/monitor.c | 36 +++++++++++++++++++++++++++---- ovsdb/ovsdb-client.c | 46 +++++++++++++++++++++++++++------------- ovsdb/ovsdb-server.c | 17 +++++++++------ ovsdb/ovsdb-util.c | 9 ++++++++ ovsdb/ovsdb.c | 8 ++++++- ovsdb/query.c | 2 ++ ovsdb/replication.c | 15 +++++++------ ovsdb/row.c | 4 +++- ovsdb/table.c | 2 +- ovsdb/transaction.c | 2 ++ utilities/ovs-vsctl.c | 47 ++++++++++++++++++++++++++++------------- vtep/vtep-ctl.c | 10 ++++++--- 27 files changed, 259 insertions(+), 93 deletions(-)