James Raphael Tiovalen <[email protected]> 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 <[email protected]>
> ---
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,
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
