| Message ID | 20230316163644.42033-1-jamestiotio@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [ovs-dev,v5] 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 | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
Hi folks, Anything that I can do to move this patch forward? I did consider splitting this patch up into smaller chunks, but I am not sure if it would be helpful since the main changes in this patch are mostly similar, just spread across multiple functions and files. That said, if it would make it easier and faster to review, I can split this patch per changed file. Thank you! Best regards, James Raphael Tiovalen On Fri, Mar 17, 2023 at 12:37 AM James Raphael Tiovalen <jamestiotio@gmail.com> wrote: > > 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. 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> > --- > 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 | 69 ++++++++++++------ > lib/dpctl.c | 66 +++++++++-------- > lib/json.c | 32 +++++--- > lib/latch-unix.c | 2 +- > lib/memory.c | 10 ++- > lib/netdev-native-tnl.c | 48 +++++++----- > lib/odp-execute.c | 91 ++++++++++++----------- > lib/pcap-file.c | 5 +- > lib/rtnetlink.c | 11 ++- > lib/sflow_agent.c | 91 +++++++++++++++-------- > lib/shash.c | 4 +- > lib/sset.c | 7 +- > ovsdb/condition.c | 10 ++- > ovsdb/file.c | 26 +++++-- > ovsdb/jsonrpc-server.c | 5 +- > ovsdb/monitor.c | 49 ++++++++++--- > ovsdb/ovsdb-client.c | 45 ++++++++---- > ovsdb/ovsdb-server.c | 15 ++-- > ovsdb/ovsdb-util.c | 9 +++ > ovsdb/ovsdb.c | 124 ++++++++++++++++--------------- > ovsdb/query.c | 4 +- > ovsdb/replication.c | 14 ++-- > ovsdb/row.c | 4 +- > ovsdb/table.c | 2 +- > ovsdb/transaction.c | 8 +- > utilities/ovs-vsctl.c | 157 ++++++++++++++++++++++++---------------- > vtep/vtep-ctl.c | 79 +++++++++++--------- > 27 files changed, 607 insertions(+), 380 deletions(-) > > diff --git a/lib/dp-packet.c b/lib/dp-packet.c > index ae8ab5800..50cb30681 100644 > --- a/lib/dp-packet.c > +++ b/lib/dp-packet.c > @@ -171,7 +171,11 @@ dp_packet_new_with_headroom(size_t size, size_t headroom) > struct dp_packet * > dp_packet_clone(const struct dp_packet *buffer) > { > - return dp_packet_clone_with_headroom(buffer, 0); > + if (buffer) { > + return dp_packet_clone_with_headroom(buffer, 0); > + } else { > + return NULL; > + } > } > > /* Creates and returns a new dp_packet whose data are copied from 'buffer'. > @@ -183,26 +187,32 @@ 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); > - /* 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, > - sizeof(struct dp_packet) - > - offsetof(struct dp_packet, l2_pad_size)); > + const void *data_dp = dp_packet_data(buffer); > > - *dp_packet_ol_flags_ptr(new_buffer) = *dp_packet_ol_flags_ptr(buffer); > - *dp_packet_ol_flags_ptr(new_buffer) &= DP_PACKET_OL_SUPPORTED_MASK; > + if (data_dp) { > + 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, > + sizeof(struct dp_packet) - > + offsetof(struct dp_packet, l2_pad_size)); > > - if (dp_packet_rss_valid(buffer)) { > - dp_packet_set_rss_hash(new_buffer, dp_packet_get_rss_hash(buffer)); > - } > - if (dp_packet_has_flow_mark(buffer, &mark)) { > - dp_packet_set_flow_mark(new_buffer, mark); > - } > + *dp_packet_ol_flags_ptr(new_buffer) = *dp_packet_ol_flags_ptr(buffer); > + *dp_packet_ol_flags_ptr(new_buffer) &= DP_PACKET_OL_SUPPORTED_MASK; > + > + if (dp_packet_rss_valid(buffer)) { > + dp_packet_set_rss_hash(new_buffer, dp_packet_get_rss_hash(buffer)); > + } > + if (dp_packet_has_flow_mark(buffer, &mark)) { > + dp_packet_set_flow_mark(new_buffer, mark); > + } > > - return new_buffer; > + return new_buffer; > + } else { > + return NULL; > + } > } > > /* Creates and returns a new dp_packet that initially contains a copy of the > @@ -323,8 +333,11 @@ 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)); > - dp_packet_set_data(b, dst); > + const void *data_dp = dp_packet_data(b); > + if (data_dp) { > + memmove(dst, data_dp, dp_packet_size(b)); > + dp_packet_set_data(b, dst); > + } > } > } > > @@ -348,7 +361,9 @@ void * > dp_packet_put_zeros(struct dp_packet *b, size_t size) > { > void *dst = dp_packet_put_uninit(b, size); > - memset(dst, 0, size); > + if (dst) { > + memset(dst, 0, size); > + } > return dst; > } > > @@ -359,7 +374,9 @@ 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); > + if (dst) { > + memcpy(dst, p, size); > + } > return dst; > } > > @@ -431,7 +448,9 @@ void * > dp_packet_push_zeros(struct dp_packet *b, size_t size) > { > void *dst = dp_packet_push_uninit(b, size); > - memset(dst, 0, size); > + if (dst) { > + memset(dst, 0, size); > + } > return dst; > } > > @@ -442,7 +461,9 @@ 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); > + if (dst) { > + memcpy(dst, p, size); > + } > return dst; > } > > diff --git a/lib/dpctl.c b/lib/dpctl.c > index c501a0cd7..6472990cc 100644 > --- a/lib/dpctl.c > +++ b/lib/dpctl.c > @@ -336,12 +336,14 @@ dpctl_add_if(int argc OVS_UNUSED, const char *argv[], > value = ""; > } > > - if (!strcmp(key, "type")) { > - type = value; > - } else if (!strcmp(key, "port_no")) { > - port_no = u32_to_odp(atoi(value)); > - } else if (!smap_add_once(&args, key, value)) { > - dpctl_error(dpctl_p, 0, "duplicate \"%s\" option", key); > + if (key) { > + if (!strcmp(key, "type")) { > + type = value; > + } else if (!strcmp(key, "port_no")) { > + port_no = u32_to_odp(atoi(value)); > + } else if (!smap_add_once(&args, key, value)) { > + dpctl_error(dpctl_p, 0, "duplicate \"%s\" option", key); > + } > } > } > > @@ -454,25 +456,29 @@ dpctl_set_if(int argc, const char *argv[], struct dpctl_params *dpctl_p) > value = ""; > } > > - if (!strcmp(key, "type")) { > - if (strcmp(value, type)) { > - dpctl_error(dpctl_p, 0, > - "%s: can't change type from %s to %s", > - name, type, value); > - error = EINVAL; > - goto next_destroy_args; > - } > - } else if (!strcmp(key, "port_no")) { > - if (port_no != u32_to_odp(atoi(value))) { > - dpctl_error(dpctl_p, 0, "%s: can't change port number from" > - " %"PRIu32" to %d", name, port_no, atoi(value)); > - error = EINVAL; > - goto next_destroy_args; > + if (key) { > + if (!strcmp(key, "type")) { > + if (strcmp(value, type)) { > + dpctl_error(dpctl_p, 0, > + "%s: can't change type from %s to %s", > + name, type, value); > + error = EINVAL; > + goto next_destroy_args; > + } > + } else if (!strcmp(key, "port_no")) { > + if (port_no != u32_to_odp(atoi(value))) { > + dpctl_error(dpctl_p, 0, > + "%s: can't change port number from" > + " %"PRIu32" to %d", name, port_no, > + atoi(value)); > + error = EINVAL; > + goto next_destroy_args; > + } > + } else if (value[0] == '\0') { > + smap_remove(&args, key); > + } else { > + smap_replace(&args, key, value); > } > - } else if (value[0] == '\0') { > - smap_remove(&args, key); > - } else { > - smap_replace(&args, key, value); > } > } > > @@ -693,12 +699,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..09683b54f 100644 > --- a/lib/json.c > +++ b/lib/json.c > @@ -342,8 +342,12 @@ json_object(const struct json *json) > bool > json_boolean(const struct json *json) > { > - ovs_assert(json->type == JSON_TRUE || json->type == JSON_FALSE); > - return json->type == JSON_TRUE; > + if (json) { > + ovs_assert(json->type == JSON_TRUE || json->type == JSON_FALSE); > + return json->type == JSON_TRUE; > + } else { > + return false; > + } > } > > double > @@ -497,13 +501,15 @@ json_hash_object(const struct shash *object, size_t basis) > size_t n, i; > > 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) { > + 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); > + } > + free(nodes); > } > - free(nodes); > return basis; > } > > @@ -1654,11 +1660,13 @@ json_serialize_object(const struct shash *object, struct json_serializer *s) > size_t n, i; > > nodes = shash_sort(object); > - n = shash_count(object); > - for (i = 0; i < n; i++) { > - json_serialize_object_member(i, nodes[i], s); > + if (nodes) { > + n = shash_count(object); > + 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..5ece04807 100644 > --- a/lib/memory.c > +++ b/lib/memory.c > @@ -116,13 +116,15 @@ 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]; > + 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); > + 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..e2dd61880 100644 > --- a/lib/netdev-native-tnl.c > +++ b/lib/netdev-native-tnl.c > @@ -221,16 +221,20 @@ 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))); > - } else { > - csum = packet_csum_pseudoheader(netdev_tnl_ip_hdr( > - dp_packet_data(packet))); > - } > + void *data_dp = dp_packet_data(packet); > + > + if (data_dp) { > + 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( > + data_dp)); > + } > > - csum = csum_continue(csum, udp, ip_tot_size); > - udp->udp_csum = csum_finish(csum); > + csum = csum_continue(csum, udp, ip_tot_size); > + udp->udp_csum = csum_finish(csum); > + } > > if (!udp->udp_csum) { > udp->udp_csum = htons(0xffff); > @@ -425,20 +429,24 @@ 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)) ? > - IPV6_HEADER_LEN : IP_HEADER_LEN; > + const void *data_dp = dp_packet_data(packet); > > - pkt_metadata_init_tnl(md); > - if (hlen > dp_packet_size(packet)) { > - goto err; > - } > + if (data_dp) { > + hlen += netdev_tnl_is_header_ipv6(data_dp) ? > + IPV6_HEADER_LEN : IP_HEADER_LEN; > > - hlen = parse_gre_header(packet, tnl); > - if (hlen < 0) { > - goto err; > - } > + pkt_metadata_init_tnl(md); > + if (hlen > dp_packet_size(packet)) { > + goto err; > + } > > - dp_packet_reset_packet(packet, hlen); > + hlen = parse_gre_header(packet, tnl); > + if (hlen < 0) { > + goto err; > + } > + > + dp_packet_reset_packet(packet, hlen); > + } > > return packet; > err: > diff --git a/lib/odp-execute.c b/lib/odp-execute.c > index 5cf6fbec0..b8cf3db55 100644 > --- a/lib/odp-execute.c > +++ b/lib/odp-execute.c > @@ -147,42 +147,45 @@ odp_set_ipv4(struct dp_packet *packet, const struct ovs_key_ipv4 *key, > uint8_t new_tos; > uint8_t new_ttl; > > - 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); > + if (nh) { > + if (mask->ipv4_src) { > + ip_src_nh = get_16aligned_be32(&nh->ip_src); > + new_ip_src = key->ipv4_src | (ip_src_nh & ~mask->ipv4_src); > > - if (ip_src_nh != new_ip_src) { > - packet_set_ipv4_addr(packet, &nh->ip_src, new_ip_src); > + if (ip_src_nh != new_ip_src) { > + packet_set_ipv4_addr(packet, &nh->ip_src, new_ip_src); > + } > } > - } > > - if (mask->ipv4_dst) { > - ip_dst_nh = get_16aligned_be32(&nh->ip_dst); > - new_ip_dst = key->ipv4_dst | (ip_dst_nh & ~mask->ipv4_dst); > + if (mask->ipv4_dst) { > + ip_dst_nh = get_16aligned_be32(&nh->ip_dst); > + new_ip_dst = key->ipv4_dst | (ip_dst_nh & ~mask->ipv4_dst); > > - if (ip_dst_nh != new_ip_dst) { > - packet_set_ipv4_addr(packet, &nh->ip_dst, new_ip_dst); > + if (ip_dst_nh != new_ip_dst) { > + packet_set_ipv4_addr(packet, &nh->ip_dst, new_ip_dst); > + } > } > - } > > - if (mask->ipv4_tos) { > - new_tos = key->ipv4_tos | (nh->ip_tos & ~mask->ipv4_tos); > + if (mask->ipv4_tos) { > + new_tos = key->ipv4_tos | (nh->ip_tos & ~mask->ipv4_tos); > > - if (nh->ip_tos != new_tos) { > - nh->ip_csum = recalc_csum16(nh->ip_csum, > - htons((uint16_t) nh->ip_tos), > - htons((uint16_t) new_tos)); > - nh->ip_tos = new_tos; > + if (nh->ip_tos != new_tos) { > + nh->ip_csum = recalc_csum16(nh->ip_csum, > + htons((uint16_t) nh->ip_tos), > + htons((uint16_t) new_tos)); > + nh->ip_tos = new_tos; > + } > } > - } > > - if (OVS_LIKELY(mask->ipv4_ttl)) { > - new_ttl = key->ipv4_ttl | (nh->ip_ttl & ~mask->ipv4_ttl); > + if (OVS_LIKELY(mask->ipv4_ttl)) { > + new_ttl = key->ipv4_ttl | (nh->ip_ttl & ~mask->ipv4_ttl); > > - if (OVS_LIKELY(nh->ip_ttl != new_ttl)) { > - nh->ip_csum = recalc_csum16(nh->ip_csum, htons(nh->ip_ttl << 8), > - htons(new_ttl << 8)); > - nh->ip_ttl = new_ttl; > + if (OVS_LIKELY(nh->ip_ttl != new_ttl)) { > + nh->ip_csum = recalc_csum16(nh->ip_csum, > + htons(nh->ip_ttl << 8), > + htons(new_ttl << 8)); > + nh->ip_ttl = new_ttl; > + } > } > } > } > @@ -276,23 +279,25 @@ set_arp(struct dp_packet *packet, const struct ovs_key_arp *key, > { > struct arp_eth_header *arp = dp_packet_l3(packet); > > - if (!mask) { > - arp->ar_op = key->arp_op; > - arp->ar_sha = key->arp_sha; > - put_16aligned_be32(&arp->ar_spa, key->arp_sip); > - arp->ar_tha = key->arp_tha; > - put_16aligned_be32(&arp->ar_tpa, key->arp_tip); > - } else { > - ovs_be32 ar_spa = get_16aligned_be32(&arp->ar_spa); > - ovs_be32 ar_tpa = get_16aligned_be32(&arp->ar_tpa); > - > - arp->ar_op = key->arp_op | (arp->ar_op & ~mask->arp_op); > - ether_addr_copy_masked(&arp->ar_sha, key->arp_sha, mask->arp_sha); > - put_16aligned_be32(&arp->ar_spa, > - key->arp_sip | (ar_spa & ~mask->arp_sip)); > - ether_addr_copy_masked(&arp->ar_tha, key->arp_tha, mask->arp_tha); > - put_16aligned_be32(&arp->ar_tpa, > - key->arp_tip | (ar_tpa & ~mask->arp_tip)); > + if (arp) { > + if (!mask) { > + arp->ar_op = key->arp_op; > + arp->ar_sha = key->arp_sha; > + put_16aligned_be32(&arp->ar_spa, key->arp_sip); > + arp->ar_tha = key->arp_tha; > + put_16aligned_be32(&arp->ar_tpa, key->arp_tip); > + } else { > + ovs_be32 ar_spa = get_16aligned_be32(&arp->ar_spa); > + ovs_be32 ar_tpa = get_16aligned_be32(&arp->ar_tpa); > + > + arp->ar_op = key->arp_op | (arp->ar_op & ~mask->arp_op); > + ether_addr_copy_masked(&arp->ar_sha, key->arp_sha, mask->arp_sha); > + put_16aligned_be32(&arp->ar_spa, > + key->arp_sip | (ar_spa & ~mask->arp_sip)); > + ether_addr_copy_masked(&arp->ar_tha, key->arp_tha, mask->arp_tha); > + put_16aligned_be32(&arp->ar_tpa, > + key->arp_tip | (ar_tpa & ~mask->arp_tip)); > + } > } > } > > diff --git a/lib/pcap-file.c b/lib/pcap-file.c > index 3ed7ea488..7cb6f5b2a 100644 > --- a/lib/pcap-file.c > +++ b/lib/pcap-file.c > @@ -291,7 +291,10 @@ 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); > + if (data_dp) { > + 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..67a0134ed 100644 > --- a/lib/rtnetlink.c > +++ b/lib/rtnetlink.c > @@ -131,10 +131,13 @@ rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change) > change->irrelevant = true; > } > > + if (ifinfo) { > + change->if_index = ifinfo->ifi_index; > + change->ifi_flags = ifinfo->ifi_flags; > + } > + > change->nlmsg_type = nlmsg->nlmsg_type; > - change->if_index = ifinfo->ifi_index; > change->ifname = nl_attr_get_string(attrs[IFLA_IFNAME]); > - change->ifi_flags = ifinfo->ifi_flags; > change->master_ifindex = (attrs[IFLA_MASTER] > ? nl_attr_get_u32(attrs[IFLA_MASTER]) > : 0); > @@ -178,7 +181,9 @@ rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change) > ifaddr = ofpbuf_at(buf, NLMSG_HDRLEN, sizeof *ifaddr); > > change->nlmsg_type = nlmsg->nlmsg_type; > - change->if_index = ifaddr->ifa_index; > + if (ifaddr) { > + change->if_index = ifaddr->ifa_index; > + } > change->ifname = (attrs[IFA_LABEL] > ? nl_attr_get_string(attrs[IFA_LABEL]) > : NULL); > diff --git a/lib/sflow_agent.c b/lib/sflow_agent.c > index c95f654a5..c19434d6c 100644 > --- a/lib/sflow_agent.c > +++ b/lib/sflow_agent.c > @@ -152,15 +152,24 @@ void sfl_agent_tick(SFLAgent *agent, time_t now) > > SFLReceiver *sfl_agent_addReceiver(SFLAgent *agent) > { > - SFLReceiver *rcv = (SFLReceiver *)sflAlloc(agent, sizeof(SFLReceiver)); > - sfl_receiver_init(rcv, agent); > - /* add to end of list - to preserve the receiver index numbers for existing receivers */ > - { > - SFLReceiver *r, *prev = NULL; > - for(r = agent->receivers; r != NULL; prev = r, r = r->nxt); > - if(prev) prev->nxt = rcv; > - else agent->receivers = rcv; > - rcv->nxt = NULL; > + SFLReceiver *rcv = (SFLReceiver *) sflAlloc(agent, sizeof(SFLReceiver)); > + if (rcv) { > + sfl_receiver_init(rcv, agent); > + /* add to end of list - to preserve the receiver index numbers for > + * existing receivers > + */ > + { > + SFLReceiver *r, *prev = NULL; > + for (r = agent->receivers; r != NULL; prev = r, r = r->nxt) { > + /* nothing to do */ > + } > + if (prev) { > + prev->nxt = rcv; > + } else { > + agent->receivers = rcv; > + } > + rcv->nxt = NULL; > + } > } > return rcv; > } > @@ -202,23 +211,35 @@ SFLSampler *sfl_agent_addSampler(SFLAgent *agent, SFLDataSource_instance *pdsi) > /* either we found the insert point, or reached the end of the list...*/ > > { > - SFLSampler *newsm = (SFLSampler *)sflAlloc(agent, sizeof(SFLSampler)); > - sfl_sampler_init(newsm, agent, pdsi); > - if(prev) prev->nxt = newsm; > - else agent->samplers = newsm; > - newsm->nxt = sm; > - > - /* see if we should go in the ifIndex jumpTable */ > - if(SFL_DS_CLASS(newsm->dsi) == 0) { > - SFLSampler *test = sfl_agent_getSamplerByIfIndex(agent, SFL_DS_INDEX(newsm->dsi)); > - if(test && (SFL_DS_INSTANCE(newsm->dsi) < SFL_DS_INSTANCE(test->dsi))) { > - /* replace with this new one because it has a lower ds_instance number */ > - sfl_agent_jumpTableRemove(agent, test); > - test = NULL; > - } > - if(test == NULL) sfl_agent_jumpTableAdd(agent, newsm); > - } > - return newsm; > + SFLSampler *newsm = (SFLSampler *) sflAlloc(agent, sizeof(SFLSampler)); > + if (newsm) { > + memset(newsm, 0, sizeof(*newsm)); > + sfl_sampler_init(newsm, agent, pdsi); > + if (prev) { > + prev->nxt = newsm; > + } else { > + agent->samplers = newsm; > + } > + newsm->nxt = sm; > + > + /* see if we should go in the ifIndex jumpTable */ > + if (SFL_DS_CLASS(newsm->dsi) == 0) { > + SFLSampler *test = sfl_agent_getSamplerByIfIndex(agent, > + SFL_DS_INDEX(newsm->dsi)); > + if (test && (SFL_DS_INSTANCE(newsm->dsi) > + < SFL_DS_INSTANCE(test->dsi))) { > + /* replace with this new one because it has a lower > + * ds_instance number > + */ > + sfl_agent_jumpTableRemove(agent, test); > + test = NULL; > + } > + if (test == NULL) { > + sfl_agent_jumpTableAdd(agent, newsm); > + } > + } > + } > + return newsm; > } > } > > @@ -241,12 +262,18 @@ 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)); > - sfl_poller_init(newpl, agent, pdsi, magic, getCountersFn); > - if(prev) prev->nxt = newpl; > - else agent->pollers = newpl; > - newpl->nxt = pl; > - return newpl; > + SFLPoller *newpl = (SFLPoller *) sflAlloc(agent, sizeof(SFLPoller)); > + if (newpl) { > + memset(newpl, 0, sizeof(*newpl)); > + sfl_poller_init(newpl, agent, pdsi, magic, getCountersFn); > + if (prev) { > + prev->nxt = newpl; > + } else { > + agent->pollers = newpl; > + } > + newpl->nxt = pl; > + } > + return newpl; > } > } > > diff --git a/lib/shash.c b/lib/shash.c > index a7b2c6458..adb388cf7 100644 > --- a/lib/shash.c > +++ b/lib/shash.c > @@ -194,7 +194,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..50f6676df 100644 > --- a/lib/sset.c > +++ b/lib/sset.c > @@ -261,8 +261,11 @@ char * > sset_pop(struct sset *set) > { > const char *name = SSET_FIRST(set); > - char *copy = xstrdup(name); > - sset_delete(set, SSET_NODE_FROM_NAME(name)); > + char *copy = NULL; > + if (name) { > + 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..f36531669 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; > + 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..bccb76098 100644 > --- a/ovsdb/file.c > +++ b/ovsdb/file.c > @@ -522,7 +522,12 @@ 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]; > > if (table != ftxn->table) { > @@ -533,14 +538,23 @@ ovsdb_file_txn_add_row(struct ovsdb_file_txn *ftxn, > > /* Create JSON object for transaction on this table. */ > ftxn->table_json = json_object_create(); > - ftxn->table = table; > - json_object_put(ftxn->json, table->schema->name, ftxn->table_json); > + if (table) { > + ftxn->table = table; > + json_object_put(ftxn->json, table->schema->name, > + ftxn->table_json); > + } > } > > /* 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..b1cbaa227 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,7 @@ 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..1ea92cc0f 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,7 +1285,9 @@ ovsdb_monitor_table_add_select(struct ovsdb_monitor *dbmon, > struct ovsdb_monitor_table * mt; > > mt = shash_find_data(&dbmon->tables, table->schema->name); > - mt->select |= select; > + if (mt) { > + mt->select |= select; > + } > } > > /* > @@ -1329,8 +1337,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,15 +1680,21 @@ 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; > > - basis = hash_pointer(mt->table, basis); > - basis = hash_3words(mt->select, mt->n_columns, basis); > + if (mt) { > + basis = hash_pointer(mt->table, basis); > + basis = hash_3words(mt->select, mt->n_columns, basis); > > - for (j = 0; j < mt->n_columns; j++) { > - basis = hash_pointer(mt->columns[j].column, basis); > - basis = hash_2words(mt->columns[j].select, basis); > + for (j = 0; j < mt->n_columns; j++) { > + basis = hash_pointer(mt->columns[j].column, basis); > + basis = hash_2words(mt->columns[j].select, basis); > + } > } > } > free(nodes); > diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c > index f1b8d6491..ca2becd8d 100644 > --- a/ovsdb/ovsdb-client.c > +++ b/ovsdb/ovsdb-client.c > @@ -1223,17 +1223,25 @@ 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 +1447,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 +1880,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 +1943,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..02e024de4 100644 > --- a/ovsdb/ovsdb-server.c > +++ b/ovsdb/ovsdb-server.c > @@ -1784,14 +1784,16 @@ 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 +2193,7 @@ 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..6dcfc5751 100644 > --- a/ovsdb/ovsdb.c > +++ b/ovsdb/ovsdb.c > @@ -195,7 +195,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,78 +215,86 @@ ovsdb_schema_from_json(const struct json *json, struct ovsdb_schema **schemap) > return error; > } > > - if (version_json) { > - version = json_string(version_json); > - if (!ovsdb_is_valid_version(version)) { > - return ovsdb_syntax_error(json, NULL, "schema version \"%s\" not " > - "in format x.y.z", version); > + if (name && tables) { > + if (version_json) { > + version = json_string(version_json); > + if (!ovsdb_is_valid_version(version)) { > + return ovsdb_syntax_error(json, NULL, > + "schema version \"%s\" not " > + "in format x.y.z", version); > + } > + } else { > + /* Backward compatibility with old databases. */ > + version = ""; > } > - } else { > - /* Backward compatibility with old databases. */ > - version = ""; > - } > > - schema = ovsdb_schema_create(json_string(name), version, > - cksum ? json_string(cksum) : ""); > - SHASH_FOR_EACH (node, json_object(tables)) { > - struct ovsdb_table_schema *table; > + schema = ovsdb_schema_create(json_string(name), version, > + cksum ? json_string(cksum) : ""); > + SHASH_FOR_EACH (node, json_object(tables)) { > + struct ovsdb_table_schema *table; > + > + if (node->name[0] == '_') { > + error = ovsdb_syntax_error(json, NULL, "names beginning with " > + "\"_\" are reserved"); > + } else if (!ovsdb_parser_is_id(node->name)) { > + error = ovsdb_syntax_error(json, NULL, > + "name must be a valid id"); > + } else { > + error = ovsdb_table_schema_from_json(node->data, node->name, > + &table); > + } > + if (error) { > + ovsdb_schema_destroy(schema); > + return error; > + } > > - if (node->name[0] == '_') { > - error = ovsdb_syntax_error(json, NULL, "names beginning with " > - "\"_\" are reserved"); > - } else if (!ovsdb_parser_is_id(node->name)) { > - error = ovsdb_syntax_error(json, NULL, "name must be a valid id"); > - } else { > - error = ovsdb_table_schema_from_json(node->data, node->name, > - &table); > - } > - if (error) { > - ovsdb_schema_destroy(schema); > - return error; > + shash_add(&schema->tables, table->name, table); > } > - > - shash_add(&schema->tables, table->name, table); > } > > - /* "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 > - * in the root set. */ > - if (root_set_size(schema) == 0) { > - SHASH_FOR_EACH (node, &schema->tables) { > - struct ovsdb_table_schema *table = node->data; > - > - table->is_root = true; > + if (schema) { > + /* "isRoot" was not part of the original schema definition. Before it > + * was added, there was no support for garbage collection. So, for > + * backward compatibility, if the root set is empty then assume that > + * every table is in the root set. */ > + if (root_set_size(schema) == 0) { > + SHASH_FOR_EACH (node, &schema->tables) { > + struct ovsdb_table_schema *table = node->data; > + > + table->is_root = true; > + } > } > - } > > - /* Validate that all refTables refer to the names of tables that exist. > - * > - * Also force certain columns to be persistent, as explained in > - * ovsdb_schema_check_ref_table(). This requires 'is_root' to be known, so > - * this must follow the loop updating 'is_root' above. */ > - SHASH_FOR_EACH (node, &schema->tables) { > - struct ovsdb_table_schema *table = node->data; > - struct shash_node *node2; > + /* Validate that all refTables refer to the names of tables that exist. > + * > + * Also force certain columns to be persistent, as explained in > + * ovsdb_schema_check_ref_table(). This requires 'is_root' to be > + * known, so this must follow the loop updating 'is_root' above. */ > + SHASH_FOR_EACH (node, &schema->tables) { > + struct ovsdb_table_schema *table = node->data; > + struct shash_node *node2; > > - SHASH_FOR_EACH (node2, &table->columns) { > - struct ovsdb_column *column = node2->data; > + SHASH_FOR_EACH (node2, &table->columns) { > + struct ovsdb_column *column = node2->data; > > - error = ovsdb_schema_check_ref_table(column, &schema->tables, > - &column->type.key, "key"); > - if (!error) { > error = ovsdb_schema_check_ref_table(column, &schema->tables, > - &column->type.value, > - "value"); > - } > - if (error) { > - ovsdb_schema_destroy(schema); > - return error; > + &column->type.key, "key"); > + if (!error) { > + error = ovsdb_schema_check_ref_table(column, > + &schema->tables, > + &column->type.value, > + "value"); > + } > + if (error) { > + ovsdb_schema_destroy(schema); > + return error; > + } > } > } > + > + *schemap = schema; > } > > - *schemap = schema; > return NULL; > } > > diff --git a/ovsdb/query.c b/ovsdb/query.c > index eebe56412..0129eb038 100644 > --- a/ovsdb/query.c > +++ b/ovsdb/query.c > @@ -91,7 +91,9 @@ ovsdb_query_distinct(struct ovsdb_table *table, > struct ovsdb_row_hash hash; > > ovsdb_row_hash_init(&hash, columns); > - ovsdb_query(table, condition, query_distinct_cb, &hash); > + if (condition) { > + ovsdb_query(table, condition, query_distinct_cb, &hash); > + } > HMAP_FOR_EACH (node, hmap_node, &hash.rows) { > ovsdb_row_set_add_row(results, node->row); > } > diff --git a/ovsdb/replication.c b/ovsdb/replication.c > index 477c69d70..f5e150c33 100644 > --- a/ovsdb/replication.c > +++ b/ovsdb/replication.c > @@ -575,15 +575,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..43b104f31 100644 > --- a/ovsdb/transaction.c > +++ b/ovsdb/transaction.c > @@ -576,9 +576,11 @@ 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(!ovsdb_row_find_weak_ref(dst_row, weak)); > - hmap_insert(&dst_row->dst_refs, &weak->dst_node, > - ovsdb_weak_ref_hash(weak)); > + if (dst_row) { > + ovs_assert(!ovsdb_row_find_weak_ref(dst_row, weak)); > + hmap_insert(&dst_row->dst_refs, &weak->dst_node, > + ovsdb_weak_ref_hash(weak)); > + } > ovs_list_remove(&weak->src_node); > ovs_list_init(&weak->src_node); > } > diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c > index 2f5ac1a26..991b426b9 100644 > --- a/utilities/ovs-vsctl.c > +++ b/utilities/ovs-vsctl.c > @@ -334,8 +334,11 @@ parse_options(int argc, char *argv[], struct shash *local_options) > exit(EXIT_SUCCESS); > > case 't': > - if (!str_to_uint(optarg, 10, &timeout) || !timeout) { > - ctl_fatal("value %s on -t or --timeout is invalid", optarg); > + if (optarg) { > + if (!str_to_uint(optarg, 10, &timeout) || !timeout) { > + ctl_fatal("value %s on -t or --timeout is invalid", > + optarg); > + } > } > break; > > @@ -349,11 +352,15 @@ parse_options(int argc, char *argv[], struct shash *local_options) > STREAM_SSL_OPTION_HANDLERS > > case OPT_PEER_CA_CERT: > - stream_ssl_set_peer_ca_cert_file(optarg); > + if (optarg) { > + stream_ssl_set_peer_ca_cert_file(optarg); > + } > break; > > case OPT_BOOTSTRAP_CA_CERT: > - stream_ssl_set_ca_cert_file(optarg, true); > + if (optarg) { > + stream_ssl_set_ca_cert_file(optarg, true); > + } > break; > > case '?': > @@ -575,7 +582,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 +666,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,11 +678,13 @@ add_port_to_cache(struct vsctl_context *vsctl_ctx, struct vsctl_bridge *parent, > } > } > > - port = xmalloc(sizeof *port); > - ovs_list_push_back(&parent->ports, &port->ports_node); > + if (parent) { > + ovs_list_push_back(&parent->ports, &port->ports_node); > + port->bridge = parent; > + } > + > ovs_list_init(&port->ifaces); > port->port_cfg = port_cfg; > - port->bridge = parent; > shash_add(&vsctl_ctx->ports, port_cfg->name, port); > > return port; > @@ -818,55 +827,63 @@ vsctl_context_populate_cache(struct ctl_context *ctx) > continue; > } > br = shash_find_data(&vsctl_ctx->bridges, br_cfg->name); > - for (j = 0; j < br_cfg->n_ports; j++) { > - struct ovsrec_port *port_cfg = br_cfg->ports[j]; > - struct vsctl_port *port; > - size_t k; > - > - port = shash_find_data(&vsctl_ctx->ports, port_cfg->name); > - if (port) { > - if (port_cfg == port->port_cfg) { > - VLOG_WARN("%s: port is in multiple bridges (%s and %s)", > - port_cfg->name, br->name, port->bridge->name); > - } else { > - /* Log as an error because this violates the database's > - * uniqueness constraints, so the database server shouldn't > - * have allowed it. */ > - VLOG_ERR("%s: database contains duplicate port name", > - port_cfg->name); > - } > - continue; > - } > - > - if (port_is_fake_bridge(port_cfg) > - && !sset_add(&bridges, port_cfg->name)) { > - continue; > - } > - > - port = add_port_to_cache(vsctl_ctx, br, port_cfg); > - for (k = 0; k < port_cfg->n_interfaces; k++) { > - struct ovsrec_interface *iface_cfg = port_cfg->interfaces[k]; > - struct vsctl_iface *iface; > - > - iface = shash_find_data(&vsctl_ctx->ifaces, iface_cfg->name); > - if (iface) { > - if (iface_cfg == iface->iface_cfg) { > - VLOG_WARN("%s: interface is in multiple ports " > - "(%s and %s)", > - iface_cfg->name, > - iface->port->port_cfg->name, > - port->port_cfg->name); > + if (br) { > + for (j = 0; j < br_cfg->n_ports; j++) { > + struct ovsrec_port *port_cfg = br_cfg->ports[j]; > + struct vsctl_port *port; > + size_t k; > + > + port = shash_find_data(&vsctl_ctx->ports, port_cfg->name); > + if (port) { > + if (port_cfg == port->port_cfg) { > + VLOG_WARN("%s: port is in multiple bridges " > + "(%s and %s)", port_cfg->name, br->name, > + port->bridge->name); > } else { > /* Log as an error because this violates the database's > - * uniqueness constraints, so the database server > - * shouldn't have allowed it. */ > - VLOG_ERR("%s: database contains duplicate interface " > - "name", iface_cfg->name); > + * uniqueness constraints, so the database server > + * shouldn't have allowed it. */ > + VLOG_ERR("%s: database contains duplicate port name", > + port_cfg->name); > } > continue; > } > > - add_iface_to_cache(vsctl_ctx, port, iface_cfg); > + if (port_is_fake_bridge(port_cfg) > + && !sset_add(&bridges, port_cfg->name)) { > + continue; > + } > + > + port = add_port_to_cache(vsctl_ctx, br, port_cfg); > + if (port) { > + for (k = 0; k < port_cfg->n_interfaces; k++) { > + struct ovsrec_interface *iface_cfg = > + port_cfg->interfaces[k]; > + struct vsctl_iface *iface; > + > + iface = shash_find_data(&vsctl_ctx->ifaces, > + iface_cfg->name); > + if (iface) { > + if (iface_cfg == iface->iface_cfg) { > + VLOG_WARN("%s: interface is in multiple ports " > + "(%s and %s)", > + iface_cfg->name, > + iface->port->port_cfg->name, > + port->port_cfg->name); > + } else { > + /* Log as an error because this violates the > + * database's uniqueness constraints, so the > + * database server shouldn't have allowed it. > + */ > + VLOG_ERR("%s: database contains duplicate " > + "interface name", iface_cfg->name); > + } > + continue; > + } > + > + add_iface_to_cache(vsctl_ctx, port, iface_cfg); > + } > + } > } > } > } > @@ -889,14 +906,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 +962,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 +980,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 +1706,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..356840089 100644 > --- a/vtep/vtep-ctl.c > +++ b/vtep/vtep-ctl.c > @@ -250,8 +250,11 @@ parse_options(int argc, char *argv[], struct shash *local_options) > exit(EXIT_SUCCESS); > > case 't': > - if (!str_to_uint(optarg, 10, &timeout) || !timeout) { > - ctl_fatal("value %s on -t or --timeout is invalid", optarg); > + if (optarg) { > + if (!str_to_uint(optarg, 10, &timeout) || !timeout) { > + ctl_fatal("value %s on -t or --timeout is invalid", > + optarg); > + } > } > break; > > @@ -1065,42 +1068,46 @@ vtep_ctl_context_populate_cache(struct ctl_context *ctx) > continue; > } > ps = shash_find_data(&vtepctl_ctx->pswitches, ps_cfg->name); > - for (j = 0; j < ps_cfg->n_ports; j++) { > - struct vteprec_physical_port *port_cfg = ps_cfg->ports[j]; > - struct vtep_ctl_port *port; > - size_t k; > - > - port = shash_find_data(&vtepctl_ctx->ports, port_cfg->name); > - if (port) { > - if (port_cfg == port->port_cfg) { > - VLOG_WARN("%s: port is in multiple physical switches " > - "(%s and %s)", > - port_cfg->name, ps->name, port->ps->name); > - } else { > - /* Log as an error because this violates the database's > - * uniqueness constraints, so the database server shouldn't > - * have allowed it. */ > - VLOG_ERR("%s: database contains duplicate port name", > - port_cfg->name); > + if (ps) { > + for (j = 0; j < ps_cfg->n_ports; j++) { > + struct vteprec_physical_port *port_cfg = ps_cfg->ports[j]; > + struct vtep_ctl_port *port; > + size_t k; > + > + port = shash_find_data(&vtepctl_ctx->ports, port_cfg->name); > + if (port) { > + if (port_cfg == port->port_cfg) { > + VLOG_WARN("%s: port is in multiple physical switches " > + "(%s and %s)", > + port_cfg->name, ps->name, port->ps->name); > + } else { > + /* Log as an error because this violates the database's > + * uniqueness constraints, so the database server > + * shouldn't have allowed it. */ > + VLOG_ERR("%s: database contains duplicate port name", > + port_cfg->name); > + } > + continue; > } > - continue; > - } > > - port = add_port_to_cache(vtepctl_ctx, ps, port_cfg); > + port = add_port_to_cache(vtepctl_ctx, ps, port_cfg); > + if (port) { > + for (k = 0; k < port_cfg->n_vlan_bindings; k++) { > + struct vtep_ctl_lswitch *ls; > + char *vlan; > > - for (k = 0; k < port_cfg->n_vlan_bindings; k++) { > - struct vtep_ctl_lswitch *ls; > - char *vlan; > + vlan = xasprintf("%"PRId64, > + port_cfg->key_vlan_bindings[k]); > + if (shash_find(&port->bindings, vlan)) { > + ctl_fatal("multiple bindings for vlan %s", vlan); > + } > > - vlan = xasprintf("%"PRId64, port_cfg->key_vlan_bindings[k]); > - if (shash_find(&port->bindings, vlan)) { > - ctl_fatal("multiple bindings for vlan %s", vlan); > - } > - > - ls_cfg = port_cfg->value_vlan_bindings[k]; > - ls = find_lswitch(vtepctl_ctx, ls_cfg->name, true); > + ls_cfg = port_cfg->value_vlan_bindings[k]; > + ls = find_lswitch(vtepctl_ctx, ls_cfg->name, true); > > - shash_add_nocopy(&port->bindings, vlan, ls); > + shash_add_nocopy(&port->bindings, vlan, ls); > + } > + } > } > } > } > @@ -1892,8 +1899,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, > -- > 2.39.2 >
On 16 Mar 2023, at 17:36, James Raphael Tiovalen wrote: > 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. 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. I did not review this, but I noticed you do null checks for memset() and memcpy(). But there are functions for this in OVS like nullable_memset() and nullable_memcpy(), maybe some of the changes might benefit from using this. In addition, there is also a nullable_string_is_equal() which might be useful in some cases. //Eelco > Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com> > --- > 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 | 69 ++++++++++++------ > lib/dpctl.c | 66 +++++++++-------- > lib/json.c | 32 +++++--- > lib/latch-unix.c | 2 +- > lib/memory.c | 10 ++- > lib/netdev-native-tnl.c | 48 +++++++----- > lib/odp-execute.c | 91 ++++++++++++----------- > lib/pcap-file.c | 5 +- > lib/rtnetlink.c | 11 ++- > lib/sflow_agent.c | 91 +++++++++++++++-------- > lib/shash.c | 4 +- > lib/sset.c | 7 +- > ovsdb/condition.c | 10 ++- > ovsdb/file.c | 26 +++++-- > ovsdb/jsonrpc-server.c | 5 +- > ovsdb/monitor.c | 49 ++++++++++--- > ovsdb/ovsdb-client.c | 45 ++++++++---- > ovsdb/ovsdb-server.c | 15 ++-- > ovsdb/ovsdb-util.c | 9 +++ > ovsdb/ovsdb.c | 124 ++++++++++++++++--------------- > ovsdb/query.c | 4 +- > ovsdb/replication.c | 14 ++-- > ovsdb/row.c | 4 +- > ovsdb/table.c | 2 +- > ovsdb/transaction.c | 8 +- > utilities/ovs-vsctl.c | 157 ++++++++++++++++++++++++---------------- > vtep/vtep-ctl.c | 79 +++++++++++--------- > 27 files changed, 607 insertions(+), 380 deletions(-) > > diff --git a/lib/dp-packet.c b/lib/dp-packet.c > index ae8ab5800..50cb30681 100644 > --- a/lib/dp-packet.c > +++ b/lib/dp-packet.c > @@ -171,7 +171,11 @@ dp_packet_new_with_headroom(size_t size, size_t headroom) > struct dp_packet * > dp_packet_clone(const struct dp_packet *buffer) > { > - return dp_packet_clone_with_headroom(buffer, 0); > + if (buffer) { > + return dp_packet_clone_with_headroom(buffer, 0); > + } else { > + return NULL; > + } > } > > /* Creates and returns a new dp_packet whose data are copied from 'buffer'. > @@ -183,26 +187,32 @@ 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); > - /* 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, > - sizeof(struct dp_packet) - > - offsetof(struct dp_packet, l2_pad_size)); > + const void *data_dp = dp_packet_data(buffer); > > - *dp_packet_ol_flags_ptr(new_buffer) = *dp_packet_ol_flags_ptr(buffer); > - *dp_packet_ol_flags_ptr(new_buffer) &= DP_PACKET_OL_SUPPORTED_MASK; > + if (data_dp) { > + 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, > + sizeof(struct dp_packet) - > + offsetof(struct dp_packet, l2_pad_size)); > > - if (dp_packet_rss_valid(buffer)) { > - dp_packet_set_rss_hash(new_buffer, dp_packet_get_rss_hash(buffer)); > - } > - if (dp_packet_has_flow_mark(buffer, &mark)) { > - dp_packet_set_flow_mark(new_buffer, mark); > - } > + *dp_packet_ol_flags_ptr(new_buffer) = *dp_packet_ol_flags_ptr(buffer); > + *dp_packet_ol_flags_ptr(new_buffer) &= DP_PACKET_OL_SUPPORTED_MASK; > + > + if (dp_packet_rss_valid(buffer)) { > + dp_packet_set_rss_hash(new_buffer, dp_packet_get_rss_hash(buffer)); > + } > + if (dp_packet_has_flow_mark(buffer, &mark)) { > + dp_packet_set_flow_mark(new_buffer, mark); > + } > > - return new_buffer; > + return new_buffer; > + } else { > + return NULL; > + } > } > > /* Creates and returns a new dp_packet that initially contains a copy of the > @@ -323,8 +333,11 @@ 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)); > - dp_packet_set_data(b, dst); > + const void *data_dp = dp_packet_data(b); > + if (data_dp) { > + memmove(dst, data_dp, dp_packet_size(b)); > + dp_packet_set_data(b, dst); > + } > } > } > > @@ -348,7 +361,9 @@ void * > dp_packet_put_zeros(struct dp_packet *b, size_t size) > { > void *dst = dp_packet_put_uninit(b, size); > - memset(dst, 0, size); > + if (dst) { > + memset(dst, 0, size); > + } > return dst; > } > > @@ -359,7 +374,9 @@ 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); > + if (dst) { > + memcpy(dst, p, size); > + } > return dst; > } > > @@ -431,7 +448,9 @@ void * > dp_packet_push_zeros(struct dp_packet *b, size_t size) > { > void *dst = dp_packet_push_uninit(b, size); > - memset(dst, 0, size); > + if (dst) { > + memset(dst, 0, size); > + } > return dst; > } > > @@ -442,7 +461,9 @@ 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); > + if (dst) { > + memcpy(dst, p, size); > + } > return dst; > } > > diff --git a/lib/dpctl.c b/lib/dpctl.c > index c501a0cd7..6472990cc 100644 > --- a/lib/dpctl.c > +++ b/lib/dpctl.c > @@ -336,12 +336,14 @@ dpctl_add_if(int argc OVS_UNUSED, const char *argv[], > value = ""; > } > > - if (!strcmp(key, "type")) { > - type = value; > - } else if (!strcmp(key, "port_no")) { > - port_no = u32_to_odp(atoi(value)); > - } else if (!smap_add_once(&args, key, value)) { > - dpctl_error(dpctl_p, 0, "duplicate \"%s\" option", key); > + if (key) { > + if (!strcmp(key, "type")) { > + type = value; > + } else if (!strcmp(key, "port_no")) { > + port_no = u32_to_odp(atoi(value)); > + } else if (!smap_add_once(&args, key, value)) { > + dpctl_error(dpctl_p, 0, "duplicate \"%s\" option", key); > + } > } > } > > @@ -454,25 +456,29 @@ dpctl_set_if(int argc, const char *argv[], struct dpctl_params *dpctl_p) > value = ""; > } > > - if (!strcmp(key, "type")) { > - if (strcmp(value, type)) { > - dpctl_error(dpctl_p, 0, > - "%s: can't change type from %s to %s", > - name, type, value); > - error = EINVAL; > - goto next_destroy_args; > - } > - } else if (!strcmp(key, "port_no")) { > - if (port_no != u32_to_odp(atoi(value))) { > - dpctl_error(dpctl_p, 0, "%s: can't change port number from" > - " %"PRIu32" to %d", name, port_no, atoi(value)); > - error = EINVAL; > - goto next_destroy_args; > + if (key) { > + if (!strcmp(key, "type")) { > + if (strcmp(value, type)) { > + dpctl_error(dpctl_p, 0, > + "%s: can't change type from %s to %s", > + name, type, value); > + error = EINVAL; > + goto next_destroy_args; > + } > + } else if (!strcmp(key, "port_no")) { > + if (port_no != u32_to_odp(atoi(value))) { > + dpctl_error(dpctl_p, 0, > + "%s: can't change port number from" > + " %"PRIu32" to %d", name, port_no, > + atoi(value)); > + error = EINVAL; > + goto next_destroy_args; > + } > + } else if (value[0] == '\0') { > + smap_remove(&args, key); > + } else { > + smap_replace(&args, key, value); > } > - } else if (value[0] == '\0') { > - smap_remove(&args, key); > - } else { > - smap_replace(&args, key, value); > } > } > > @@ -693,12 +699,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..09683b54f 100644 > --- a/lib/json.c > +++ b/lib/json.c > @@ -342,8 +342,12 @@ json_object(const struct json *json) > bool > json_boolean(const struct json *json) > { > - ovs_assert(json->type == JSON_TRUE || json->type == JSON_FALSE); > - return json->type == JSON_TRUE; > + if (json) { > + ovs_assert(json->type == JSON_TRUE || json->type == JSON_FALSE); > + return json->type == JSON_TRUE; > + } else { > + return false; > + } > } > > double > @@ -497,13 +501,15 @@ json_hash_object(const struct shash *object, size_t basis) > size_t n, i; > > 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) { > + 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); > + } > + free(nodes); > } > - free(nodes); > return basis; > } > > @@ -1654,11 +1660,13 @@ json_serialize_object(const struct shash *object, struct json_serializer *s) > size_t n, i; > > nodes = shash_sort(object); > - n = shash_count(object); > - for (i = 0; i < n; i++) { > - json_serialize_object_member(i, nodes[i], s); > + if (nodes) { > + n = shash_count(object); > + 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..5ece04807 100644 > --- a/lib/memory.c > +++ b/lib/memory.c > @@ -116,13 +116,15 @@ 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]; > + 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); > + 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..e2dd61880 100644 > --- a/lib/netdev-native-tnl.c > +++ b/lib/netdev-native-tnl.c > @@ -221,16 +221,20 @@ 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))); > - } else { > - csum = packet_csum_pseudoheader(netdev_tnl_ip_hdr( > - dp_packet_data(packet))); > - } > + void *data_dp = dp_packet_data(packet); > + > + if (data_dp) { > + 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( > + data_dp)); > + } > > - csum = csum_continue(csum, udp, ip_tot_size); > - udp->udp_csum = csum_finish(csum); > + csum = csum_continue(csum, udp, ip_tot_size); > + udp->udp_csum = csum_finish(csum); > + } > > if (!udp->udp_csum) { > udp->udp_csum = htons(0xffff); > @@ -425,20 +429,24 @@ 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)) ? > - IPV6_HEADER_LEN : IP_HEADER_LEN; > + const void *data_dp = dp_packet_data(packet); > > - pkt_metadata_init_tnl(md); > - if (hlen > dp_packet_size(packet)) { > - goto err; > - } > + if (data_dp) { > + hlen += netdev_tnl_is_header_ipv6(data_dp) ? > + IPV6_HEADER_LEN : IP_HEADER_LEN; > > - hlen = parse_gre_header(packet, tnl); > - if (hlen < 0) { > - goto err; > - } > + pkt_metadata_init_tnl(md); > + if (hlen > dp_packet_size(packet)) { > + goto err; > + } > > - dp_packet_reset_packet(packet, hlen); > + hlen = parse_gre_header(packet, tnl); > + if (hlen < 0) { > + goto err; > + } > + > + dp_packet_reset_packet(packet, hlen); > + } > > return packet; > err: > diff --git a/lib/odp-execute.c b/lib/odp-execute.c > index 5cf6fbec0..b8cf3db55 100644 > --- a/lib/odp-execute.c > +++ b/lib/odp-execute.c > @@ -147,42 +147,45 @@ odp_set_ipv4(struct dp_packet *packet, const struct ovs_key_ipv4 *key, > uint8_t new_tos; > uint8_t new_ttl; > > - 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); > + if (nh) { > + if (mask->ipv4_src) { > + ip_src_nh = get_16aligned_be32(&nh->ip_src); > + new_ip_src = key->ipv4_src | (ip_src_nh & ~mask->ipv4_src); > > - if (ip_src_nh != new_ip_src) { > - packet_set_ipv4_addr(packet, &nh->ip_src, new_ip_src); > + if (ip_src_nh != new_ip_src) { > + packet_set_ipv4_addr(packet, &nh->ip_src, new_ip_src); > + } > } > - } > > - if (mask->ipv4_dst) { > - ip_dst_nh = get_16aligned_be32(&nh->ip_dst); > - new_ip_dst = key->ipv4_dst | (ip_dst_nh & ~mask->ipv4_dst); > + if (mask->ipv4_dst) { > + ip_dst_nh = get_16aligned_be32(&nh->ip_dst); > + new_ip_dst = key->ipv4_dst | (ip_dst_nh & ~mask->ipv4_dst); > > - if (ip_dst_nh != new_ip_dst) { > - packet_set_ipv4_addr(packet, &nh->ip_dst, new_ip_dst); > + if (ip_dst_nh != new_ip_dst) { > + packet_set_ipv4_addr(packet, &nh->ip_dst, new_ip_dst); > + } > } > - } > > - if (mask->ipv4_tos) { > - new_tos = key->ipv4_tos | (nh->ip_tos & ~mask->ipv4_tos); > + if (mask->ipv4_tos) { > + new_tos = key->ipv4_tos | (nh->ip_tos & ~mask->ipv4_tos); > > - if (nh->ip_tos != new_tos) { > - nh->ip_csum = recalc_csum16(nh->ip_csum, > - htons((uint16_t) nh->ip_tos), > - htons((uint16_t) new_tos)); > - nh->ip_tos = new_tos; > + if (nh->ip_tos != new_tos) { > + nh->ip_csum = recalc_csum16(nh->ip_csum, > + htons((uint16_t) nh->ip_tos), > + htons((uint16_t) new_tos)); > + nh->ip_tos = new_tos; > + } > } > - } > > - if (OVS_LIKELY(mask->ipv4_ttl)) { > - new_ttl = key->ipv4_ttl | (nh->ip_ttl & ~mask->ipv4_ttl); > + if (OVS_LIKELY(mask->ipv4_ttl)) { > + new_ttl = key->ipv4_ttl | (nh->ip_ttl & ~mask->ipv4_ttl); > > - if (OVS_LIKELY(nh->ip_ttl != new_ttl)) { > - nh->ip_csum = recalc_csum16(nh->ip_csum, htons(nh->ip_ttl << 8), > - htons(new_ttl << 8)); > - nh->ip_ttl = new_ttl; > + if (OVS_LIKELY(nh->ip_ttl != new_ttl)) { > + nh->ip_csum = recalc_csum16(nh->ip_csum, > + htons(nh->ip_ttl << 8), > + htons(new_ttl << 8)); > + nh->ip_ttl = new_ttl; > + } > } > } > } > @@ -276,23 +279,25 @@ set_arp(struct dp_packet *packet, const struct ovs_key_arp *key, > { > struct arp_eth_header *arp = dp_packet_l3(packet); > > - if (!mask) { > - arp->ar_op = key->arp_op; > - arp->ar_sha = key->arp_sha; > - put_16aligned_be32(&arp->ar_spa, key->arp_sip); > - arp->ar_tha = key->arp_tha; > - put_16aligned_be32(&arp->ar_tpa, key->arp_tip); > - } else { > - ovs_be32 ar_spa = get_16aligned_be32(&arp->ar_spa); > - ovs_be32 ar_tpa = get_16aligned_be32(&arp->ar_tpa); > - > - arp->ar_op = key->arp_op | (arp->ar_op & ~mask->arp_op); > - ether_addr_copy_masked(&arp->ar_sha, key->arp_sha, mask->arp_sha); > - put_16aligned_be32(&arp->ar_spa, > - key->arp_sip | (ar_spa & ~mask->arp_sip)); > - ether_addr_copy_masked(&arp->ar_tha, key->arp_tha, mask->arp_tha); > - put_16aligned_be32(&arp->ar_tpa, > - key->arp_tip | (ar_tpa & ~mask->arp_tip)); > + if (arp) { > + if (!mask) { > + arp->ar_op = key->arp_op; > + arp->ar_sha = key->arp_sha; > + put_16aligned_be32(&arp->ar_spa, key->arp_sip); > + arp->ar_tha = key->arp_tha; > + put_16aligned_be32(&arp->ar_tpa, key->arp_tip); > + } else { > + ovs_be32 ar_spa = get_16aligned_be32(&arp->ar_spa); > + ovs_be32 ar_tpa = get_16aligned_be32(&arp->ar_tpa); > + > + arp->ar_op = key->arp_op | (arp->ar_op & ~mask->arp_op); > + ether_addr_copy_masked(&arp->ar_sha, key->arp_sha, mask->arp_sha); > + put_16aligned_be32(&arp->ar_spa, > + key->arp_sip | (ar_spa & ~mask->arp_sip)); > + ether_addr_copy_masked(&arp->ar_tha, key->arp_tha, mask->arp_tha); > + put_16aligned_be32(&arp->ar_tpa, > + key->arp_tip | (ar_tpa & ~mask->arp_tip)); > + } > } > } > > diff --git a/lib/pcap-file.c b/lib/pcap-file.c > index 3ed7ea488..7cb6f5b2a 100644 > --- a/lib/pcap-file.c > +++ b/lib/pcap-file.c > @@ -291,7 +291,10 @@ 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); > + if (data_dp) { > + 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..67a0134ed 100644 > --- a/lib/rtnetlink.c > +++ b/lib/rtnetlink.c > @@ -131,10 +131,13 @@ rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change) > change->irrelevant = true; > } > > + if (ifinfo) { > + change->if_index = ifinfo->ifi_index; > + change->ifi_flags = ifinfo->ifi_flags; > + } > + > change->nlmsg_type = nlmsg->nlmsg_type; > - change->if_index = ifinfo->ifi_index; > change->ifname = nl_attr_get_string(attrs[IFLA_IFNAME]); > - change->ifi_flags = ifinfo->ifi_flags; > change->master_ifindex = (attrs[IFLA_MASTER] > ? nl_attr_get_u32(attrs[IFLA_MASTER]) > : 0); > @@ -178,7 +181,9 @@ rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change) > ifaddr = ofpbuf_at(buf, NLMSG_HDRLEN, sizeof *ifaddr); > > change->nlmsg_type = nlmsg->nlmsg_type; > - change->if_index = ifaddr->ifa_index; > + if (ifaddr) { > + change->if_index = ifaddr->ifa_index; > + } > change->ifname = (attrs[IFA_LABEL] > ? nl_attr_get_string(attrs[IFA_LABEL]) > : NULL); > diff --git a/lib/sflow_agent.c b/lib/sflow_agent.c > index c95f654a5..c19434d6c 100644 > --- a/lib/sflow_agent.c > +++ b/lib/sflow_agent.c > @@ -152,15 +152,24 @@ void sfl_agent_tick(SFLAgent *agent, time_t now) > > SFLReceiver *sfl_agent_addReceiver(SFLAgent *agent) > { > - SFLReceiver *rcv = (SFLReceiver *)sflAlloc(agent, sizeof(SFLReceiver)); > - sfl_receiver_init(rcv, agent); > - /* add to end of list - to preserve the receiver index numbers for existing receivers */ > - { > - SFLReceiver *r, *prev = NULL; > - for(r = agent->receivers; r != NULL; prev = r, r = r->nxt); > - if(prev) prev->nxt = rcv; > - else agent->receivers = rcv; > - rcv->nxt = NULL; > + SFLReceiver *rcv = (SFLReceiver *) sflAlloc(agent, sizeof(SFLReceiver)); > + if (rcv) { > + sfl_receiver_init(rcv, agent); > + /* add to end of list - to preserve the receiver index numbers for > + * existing receivers > + */ > + { > + SFLReceiver *r, *prev = NULL; > + for (r = agent->receivers; r != NULL; prev = r, r = r->nxt) { > + /* nothing to do */ > + } > + if (prev) { > + prev->nxt = rcv; > + } else { > + agent->receivers = rcv; > + } > + rcv->nxt = NULL; > + } > } > return rcv; > } > @@ -202,23 +211,35 @@ SFLSampler *sfl_agent_addSampler(SFLAgent *agent, SFLDataSource_instance *pdsi) > /* either we found the insert point, or reached the end of the list...*/ > > { > - SFLSampler *newsm = (SFLSampler *)sflAlloc(agent, sizeof(SFLSampler)); > - sfl_sampler_init(newsm, agent, pdsi); > - if(prev) prev->nxt = newsm; > - else agent->samplers = newsm; > - newsm->nxt = sm; > - > - /* see if we should go in the ifIndex jumpTable */ > - if(SFL_DS_CLASS(newsm->dsi) == 0) { > - SFLSampler *test = sfl_agent_getSamplerByIfIndex(agent, SFL_DS_INDEX(newsm->dsi)); > - if(test && (SFL_DS_INSTANCE(newsm->dsi) < SFL_DS_INSTANCE(test->dsi))) { > - /* replace with this new one because it has a lower ds_instance number */ > - sfl_agent_jumpTableRemove(agent, test); > - test = NULL; > - } > - if(test == NULL) sfl_agent_jumpTableAdd(agent, newsm); > - } > - return newsm; > + SFLSampler *newsm = (SFLSampler *) sflAlloc(agent, sizeof(SFLSampler)); > + if (newsm) { > + memset(newsm, 0, sizeof(*newsm)); > + sfl_sampler_init(newsm, agent, pdsi); > + if (prev) { > + prev->nxt = newsm; > + } else { > + agent->samplers = newsm; > + } > + newsm->nxt = sm; > + > + /* see if we should go in the ifIndex jumpTable */ > + if (SFL_DS_CLASS(newsm->dsi) == 0) { > + SFLSampler *test = sfl_agent_getSamplerByIfIndex(agent, > + SFL_DS_INDEX(newsm->dsi)); > + if (test && (SFL_DS_INSTANCE(newsm->dsi) > + < SFL_DS_INSTANCE(test->dsi))) { > + /* replace with this new one because it has a lower > + * ds_instance number > + */ > + sfl_agent_jumpTableRemove(agent, test); > + test = NULL; > + } > + if (test == NULL) { > + sfl_agent_jumpTableAdd(agent, newsm); > + } > + } > + } > + return newsm; > } > } > > @@ -241,12 +262,18 @@ 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)); > - sfl_poller_init(newpl, agent, pdsi, magic, getCountersFn); > - if(prev) prev->nxt = newpl; > - else agent->pollers = newpl; > - newpl->nxt = pl; > - return newpl; > + SFLPoller *newpl = (SFLPoller *) sflAlloc(agent, sizeof(SFLPoller)); > + if (newpl) { > + memset(newpl, 0, sizeof(*newpl)); > + sfl_poller_init(newpl, agent, pdsi, magic, getCountersFn); > + if (prev) { > + prev->nxt = newpl; > + } else { > + agent->pollers = newpl; > + } > + newpl->nxt = pl; > + } > + return newpl; > } > } > > diff --git a/lib/shash.c b/lib/shash.c > index a7b2c6458..adb388cf7 100644 > --- a/lib/shash.c > +++ b/lib/shash.c > @@ -194,7 +194,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..50f6676df 100644 > --- a/lib/sset.c > +++ b/lib/sset.c > @@ -261,8 +261,11 @@ char * > sset_pop(struct sset *set) > { > const char *name = SSET_FIRST(set); > - char *copy = xstrdup(name); > - sset_delete(set, SSET_NODE_FROM_NAME(name)); > + char *copy = NULL; > + if (name) { > + 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..f36531669 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; > + 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..bccb76098 100644 > --- a/ovsdb/file.c > +++ b/ovsdb/file.c > @@ -522,7 +522,12 @@ 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]; > > if (table != ftxn->table) { > @@ -533,14 +538,23 @@ ovsdb_file_txn_add_row(struct ovsdb_file_txn *ftxn, > > /* Create JSON object for transaction on this table. */ > ftxn->table_json = json_object_create(); > - ftxn->table = table; > - json_object_put(ftxn->json, table->schema->name, ftxn->table_json); > + if (table) { > + ftxn->table = table; > + json_object_put(ftxn->json, table->schema->name, > + ftxn->table_json); > + } > } > > /* 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..b1cbaa227 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,7 @@ 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..1ea92cc0f 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,7 +1285,9 @@ ovsdb_monitor_table_add_select(struct ovsdb_monitor *dbmon, > struct ovsdb_monitor_table * mt; > > mt = shash_find_data(&dbmon->tables, table->schema->name); > - mt->select |= select; > + if (mt) { > + mt->select |= select; > + } > } > > /* > @@ -1329,8 +1337,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,15 +1680,21 @@ 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; > > - basis = hash_pointer(mt->table, basis); > - basis = hash_3words(mt->select, mt->n_columns, basis); > + if (mt) { > + basis = hash_pointer(mt->table, basis); > + basis = hash_3words(mt->select, mt->n_columns, basis); > > - for (j = 0; j < mt->n_columns; j++) { > - basis = hash_pointer(mt->columns[j].column, basis); > - basis = hash_2words(mt->columns[j].select, basis); > + for (j = 0; j < mt->n_columns; j++) { > + basis = hash_pointer(mt->columns[j].column, basis); > + basis = hash_2words(mt->columns[j].select, basis); > + } > } > } > free(nodes); > diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c > index f1b8d6491..ca2becd8d 100644 > --- a/ovsdb/ovsdb-client.c > +++ b/ovsdb/ovsdb-client.c > @@ -1223,17 +1223,25 @@ 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 +1447,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 +1880,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 +1943,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..02e024de4 100644 > --- a/ovsdb/ovsdb-server.c > +++ b/ovsdb/ovsdb-server.c > @@ -1784,14 +1784,16 @@ 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 +2193,7 @@ 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..6dcfc5751 100644 > --- a/ovsdb/ovsdb.c > +++ b/ovsdb/ovsdb.c > @@ -195,7 +195,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,78 +215,86 @@ ovsdb_schema_from_json(const struct json *json, struct ovsdb_schema **schemap) > return error; > } > > - if (version_json) { > - version = json_string(version_json); > - if (!ovsdb_is_valid_version(version)) { > - return ovsdb_syntax_error(json, NULL, "schema version \"%s\" not " > - "in format x.y.z", version); > + if (name && tables) { > + if (version_json) { > + version = json_string(version_json); > + if (!ovsdb_is_valid_version(version)) { > + return ovsdb_syntax_error(json, NULL, > + "schema version \"%s\" not " > + "in format x.y.z", version); > + } > + } else { > + /* Backward compatibility with old databases. */ > + version = ""; > } > - } else { > - /* Backward compatibility with old databases. */ > - version = ""; > - } > > - schema = ovsdb_schema_create(json_string(name), version, > - cksum ? json_string(cksum) : ""); > - SHASH_FOR_EACH (node, json_object(tables)) { > - struct ovsdb_table_schema *table; > + schema = ovsdb_schema_create(json_string(name), version, > + cksum ? json_string(cksum) : ""); > + SHASH_FOR_EACH (node, json_object(tables)) { > + struct ovsdb_table_schema *table; > + > + if (node->name[0] == '_') { > + error = ovsdb_syntax_error(json, NULL, "names beginning with " > + "\"_\" are reserved"); > + } else if (!ovsdb_parser_is_id(node->name)) { > + error = ovsdb_syntax_error(json, NULL, > + "name must be a valid id"); > + } else { > + error = ovsdb_table_schema_from_json(node->data, node->name, > + &table); > + } > + if (error) { > + ovsdb_schema_destroy(schema); > + return error; > + } > > - if (node->name[0] == '_') { > - error = ovsdb_syntax_error(json, NULL, "names beginning with " > - "\"_\" are reserved"); > - } else if (!ovsdb_parser_is_id(node->name)) { > - error = ovsdb_syntax_error(json, NULL, "name must be a valid id"); > - } else { > - error = ovsdb_table_schema_from_json(node->data, node->name, > - &table); > - } > - if (error) { > - ovsdb_schema_destroy(schema); > - return error; > + shash_add(&schema->tables, table->name, table); > } > - > - shash_add(&schema->tables, table->name, table); > } > > - /* "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 > - * in the root set. */ > - if (root_set_size(schema) == 0) { > - SHASH_FOR_EACH (node, &schema->tables) { > - struct ovsdb_table_schema *table = node->data; > - > - table->is_root = true; > + if (schema) { > + /* "isRoot" was not part of the original schema definition. Before it > + * was added, there was no support for garbage collection. So, for > + * backward compatibility, if the root set is empty then assume that > + * every table is in the root set. */ > + if (root_set_size(schema) == 0) { > + SHASH_FOR_EACH (node, &schema->tables) { > + struct ovsdb_table_schema *table = node->data; > + > + table->is_root = true; > + } > } > - } > > - /* Validate that all refTables refer to the names of tables that exist. > - * > - * Also force certain columns to be persistent, as explained in > - * ovsdb_schema_check_ref_table(). This requires 'is_root' to be known, so > - * this must follow the loop updating 'is_root' above. */ > - SHASH_FOR_EACH (node, &schema->tables) { > - struct ovsdb_table_schema *table = node->data; > - struct shash_node *node2; > + /* Validate that all refTables refer to the names of tables that exist. > + * > + * Also force certain columns to be persistent, as explained in > + * ovsdb_schema_check_ref_table(). This requires 'is_root' to be > + * known, so this must follow the loop updating 'is_root' above. */ > + SHASH_FOR_EACH (node, &schema->tables) { > + struct ovsdb_table_schema *table = node->data; > + struct shash_node *node2; > > - SHASH_FOR_EACH (node2, &table->columns) { > - struct ovsdb_column *column = node2->data; > + SHASH_FOR_EACH (node2, &table->columns) { > + struct ovsdb_column *column = node2->data; > > - error = ovsdb_schema_check_ref_table(column, &schema->tables, > - &column->type.key, "key"); > - if (!error) { > error = ovsdb_schema_check_ref_table(column, &schema->tables, > - &column->type.value, > - "value"); > - } > - if (error) { > - ovsdb_schema_destroy(schema); > - return error; > + &column->type.key, "key"); > + if (!error) { > + error = ovsdb_schema_check_ref_table(column, > + &schema->tables, > + &column->type.value, > + "value"); > + } > + if (error) { > + ovsdb_schema_destroy(schema); > + return error; > + } > } > } > + > + *schemap = schema; > } > > - *schemap = schema; > return NULL; > } > > diff --git a/ovsdb/query.c b/ovsdb/query.c > index eebe56412..0129eb038 100644 > --- a/ovsdb/query.c > +++ b/ovsdb/query.c > @@ -91,7 +91,9 @@ ovsdb_query_distinct(struct ovsdb_table *table, > struct ovsdb_row_hash hash; > > ovsdb_row_hash_init(&hash, columns); > - ovsdb_query(table, condition, query_distinct_cb, &hash); > + if (condition) { > + ovsdb_query(table, condition, query_distinct_cb, &hash); > + } > HMAP_FOR_EACH (node, hmap_node, &hash.rows) { > ovsdb_row_set_add_row(results, node->row); > } > diff --git a/ovsdb/replication.c b/ovsdb/replication.c > index 477c69d70..f5e150c33 100644 > --- a/ovsdb/replication.c > +++ b/ovsdb/replication.c > @@ -575,15 +575,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..43b104f31 100644 > --- a/ovsdb/transaction.c > +++ b/ovsdb/transaction.c > @@ -576,9 +576,11 @@ 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(!ovsdb_row_find_weak_ref(dst_row, weak)); > - hmap_insert(&dst_row->dst_refs, &weak->dst_node, > - ovsdb_weak_ref_hash(weak)); > + if (dst_row) { > + ovs_assert(!ovsdb_row_find_weak_ref(dst_row, weak)); > + hmap_insert(&dst_row->dst_refs, &weak->dst_node, > + ovsdb_weak_ref_hash(weak)); > + } > ovs_list_remove(&weak->src_node); > ovs_list_init(&weak->src_node); > } > diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c > index 2f5ac1a26..991b426b9 100644 > --- a/utilities/ovs-vsctl.c > +++ b/utilities/ovs-vsctl.c > @@ -334,8 +334,11 @@ parse_options(int argc, char *argv[], struct shash *local_options) > exit(EXIT_SUCCESS); > > case 't': > - if (!str_to_uint(optarg, 10, &timeout) || !timeout) { > - ctl_fatal("value %s on -t or --timeout is invalid", optarg); > + if (optarg) { > + if (!str_to_uint(optarg, 10, &timeout) || !timeout) { > + ctl_fatal("value %s on -t or --timeout is invalid", > + optarg); > + } > } > break; > > @@ -349,11 +352,15 @@ parse_options(int argc, char *argv[], struct shash *local_options) > STREAM_SSL_OPTION_HANDLERS > > case OPT_PEER_CA_CERT: > - stream_ssl_set_peer_ca_cert_file(optarg); > + if (optarg) { > + stream_ssl_set_peer_ca_cert_file(optarg); > + } > break; > > case OPT_BOOTSTRAP_CA_CERT: > - stream_ssl_set_ca_cert_file(optarg, true); > + if (optarg) { > + stream_ssl_set_ca_cert_file(optarg, true); > + } > break; > > case '?': > @@ -575,7 +582,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 +666,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,11 +678,13 @@ add_port_to_cache(struct vsctl_context *vsctl_ctx, struct vsctl_bridge *parent, > } > } > > - port = xmalloc(sizeof *port); > - ovs_list_push_back(&parent->ports, &port->ports_node); > + if (parent) { > + ovs_list_push_back(&parent->ports, &port->ports_node); > + port->bridge = parent; > + } > + > ovs_list_init(&port->ifaces); > port->port_cfg = port_cfg; > - port->bridge = parent; > shash_add(&vsctl_ctx->ports, port_cfg->name, port); > > return port; > @@ -818,55 +827,63 @@ vsctl_context_populate_cache(struct ctl_context *ctx) > continue; > } > br = shash_find_data(&vsctl_ctx->bridges, br_cfg->name); > - for (j = 0; j < br_cfg->n_ports; j++) { > - struct ovsrec_port *port_cfg = br_cfg->ports[j]; > - struct vsctl_port *port; > - size_t k; > - > - port = shash_find_data(&vsctl_ctx->ports, port_cfg->name); > - if (port) { > - if (port_cfg == port->port_cfg) { > - VLOG_WARN("%s: port is in multiple bridges (%s and %s)", > - port_cfg->name, br->name, port->bridge->name); > - } else { > - /* Log as an error because this violates the database's > - * uniqueness constraints, so the database server shouldn't > - * have allowed it. */ > - VLOG_ERR("%s: database contains duplicate port name", > - port_cfg->name); > - } > - continue; > - } > - > - if (port_is_fake_bridge(port_cfg) > - && !sset_add(&bridges, port_cfg->name)) { > - continue; > - } > - > - port = add_port_to_cache(vsctl_ctx, br, port_cfg); > - for (k = 0; k < port_cfg->n_interfaces; k++) { > - struct ovsrec_interface *iface_cfg = port_cfg->interfaces[k]; > - struct vsctl_iface *iface; > - > - iface = shash_find_data(&vsctl_ctx->ifaces, iface_cfg->name); > - if (iface) { > - if (iface_cfg == iface->iface_cfg) { > - VLOG_WARN("%s: interface is in multiple ports " > - "(%s and %s)", > - iface_cfg->name, > - iface->port->port_cfg->name, > - port->port_cfg->name); > + if (br) { > + for (j = 0; j < br_cfg->n_ports; j++) { > + struct ovsrec_port *port_cfg = br_cfg->ports[j]; > + struct vsctl_port *port; > + size_t k; > + > + port = shash_find_data(&vsctl_ctx->ports, port_cfg->name); > + if (port) { > + if (port_cfg == port->port_cfg) { > + VLOG_WARN("%s: port is in multiple bridges " > + "(%s and %s)", port_cfg->name, br->name, > + port->bridge->name); > } else { > /* Log as an error because this violates the database's > - * uniqueness constraints, so the database server > - * shouldn't have allowed it. */ > - VLOG_ERR("%s: database contains duplicate interface " > - "name", iface_cfg->name); > + * uniqueness constraints, so the database server > + * shouldn't have allowed it. */ > + VLOG_ERR("%s: database contains duplicate port name", > + port_cfg->name); > } > continue; > } > > - add_iface_to_cache(vsctl_ctx, port, iface_cfg); > + if (port_is_fake_bridge(port_cfg) > + && !sset_add(&bridges, port_cfg->name)) { > + continue; > + } > + > + port = add_port_to_cache(vsctl_ctx, br, port_cfg); > + if (port) { > + for (k = 0; k < port_cfg->n_interfaces; k++) { > + struct ovsrec_interface *iface_cfg = > + port_cfg->interfaces[k]; > + struct vsctl_iface *iface; > + > + iface = shash_find_data(&vsctl_ctx->ifaces, > + iface_cfg->name); > + if (iface) { > + if (iface_cfg == iface->iface_cfg) { > + VLOG_WARN("%s: interface is in multiple ports " > + "(%s and %s)", > + iface_cfg->name, > + iface->port->port_cfg->name, > + port->port_cfg->name); > + } else { > + /* Log as an error because this violates the > + * database's uniqueness constraints, so the > + * database server shouldn't have allowed it. > + */ > + VLOG_ERR("%s: database contains duplicate " > + "interface name", iface_cfg->name); > + } > + continue; > + } > + > + add_iface_to_cache(vsctl_ctx, port, iface_cfg); > + } > + } > } > } > } > @@ -889,14 +906,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 +962,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 +980,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 +1706,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..356840089 100644 > --- a/vtep/vtep-ctl.c > +++ b/vtep/vtep-ctl.c > @@ -250,8 +250,11 @@ parse_options(int argc, char *argv[], struct shash *local_options) > exit(EXIT_SUCCESS); > > case 't': > - if (!str_to_uint(optarg, 10, &timeout) || !timeout) { > - ctl_fatal("value %s on -t or --timeout is invalid", optarg); > + if (optarg) { > + if (!str_to_uint(optarg, 10, &timeout) || !timeout) { > + ctl_fatal("value %s on -t or --timeout is invalid", > + optarg); > + } > } > break; > > @@ -1065,42 +1068,46 @@ vtep_ctl_context_populate_cache(struct ctl_context *ctx) > continue; > } > ps = shash_find_data(&vtepctl_ctx->pswitches, ps_cfg->name); > - for (j = 0; j < ps_cfg->n_ports; j++) { > - struct vteprec_physical_port *port_cfg = ps_cfg->ports[j]; > - struct vtep_ctl_port *port; > - size_t k; > - > - port = shash_find_data(&vtepctl_ctx->ports, port_cfg->name); > - if (port) { > - if (port_cfg == port->port_cfg) { > - VLOG_WARN("%s: port is in multiple physical switches " > - "(%s and %s)", > - port_cfg->name, ps->name, port->ps->name); > - } else { > - /* Log as an error because this violates the database's > - * uniqueness constraints, so the database server shouldn't > - * have allowed it. */ > - VLOG_ERR("%s: database contains duplicate port name", > - port_cfg->name); > + if (ps) { > + for (j = 0; j < ps_cfg->n_ports; j++) { > + struct vteprec_physical_port *port_cfg = ps_cfg->ports[j]; > + struct vtep_ctl_port *port; > + size_t k; > + > + port = shash_find_data(&vtepctl_ctx->ports, port_cfg->name); > + if (port) { > + if (port_cfg == port->port_cfg) { > + VLOG_WARN("%s: port is in multiple physical switches " > + "(%s and %s)", > + port_cfg->name, ps->name, port->ps->name); > + } else { > + /* Log as an error because this violates the database's > + * uniqueness constraints, so the database server > + * shouldn't have allowed it. */ > + VLOG_ERR("%s: database contains duplicate port name", > + port_cfg->name); > + } > + continue; > } > - continue; > - } > > - port = add_port_to_cache(vtepctl_ctx, ps, port_cfg); > + port = add_port_to_cache(vtepctl_ctx, ps, port_cfg); > + if (port) { > + for (k = 0; k < port_cfg->n_vlan_bindings; k++) { > + struct vtep_ctl_lswitch *ls; > + char *vlan; > > - for (k = 0; k < port_cfg->n_vlan_bindings; k++) { > - struct vtep_ctl_lswitch *ls; > - char *vlan; > + vlan = xasprintf("%"PRId64, > + port_cfg->key_vlan_bindings[k]); > + if (shash_find(&port->bindings, vlan)) { > + ctl_fatal("multiple bindings for vlan %s", vlan); > + } > > - vlan = xasprintf("%"PRId64, port_cfg->key_vlan_bindings[k]); > - if (shash_find(&port->bindings, vlan)) { > - ctl_fatal("multiple bindings for vlan %s", vlan); > - } > - > - ls_cfg = port_cfg->value_vlan_bindings[k]; > - ls = find_lswitch(vtepctl_ctx, ls_cfg->name, true); > + ls_cfg = port_cfg->value_vlan_bindings[k]; > + ls = find_lswitch(vtepctl_ctx, ls_cfg->name, true); > > - shash_add_nocopy(&port->bindings, vlan, ls); > + shash_add_nocopy(&port->bindings, vlan, ls); > + } > + } > } > } > } > @@ -1892,8 +1899,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, > -- > 2.39.2 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 3/27/23 11:03, Eelco Chaudron wrote: > > > On 16 Mar 2023, at 17:36, James Raphael Tiovalen wrote: > >> 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. 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. > > I did not review this, but I noticed you do null checks for memset() and memcpy(). But there are functions for this in OVS like nullable_memset() and nullable_memcpy(), maybe some of the changes might benefit from using this. > > In addition, there is also a nullable_string_is_equal() which might be useful in some cases. I'd also say that the vast majority of changes here should be replaced with assertions, because they are checking for impossible conditions. For example, there should be no way the name and tables are not set after successful parser run in ovsdb_schema_from_json(), because they are not optional. Explicit handling of impossible cases is confusing for readers. Use of assertions will also reduce the patch size significantly. Best regards, Ilya Maximets.
On Mon, Mar 27, 2023 at 5:03 PM Eelco Chaudron <echaudro@redhat.com> wrote: > > I did not review this, but I noticed you do null checks for memset() and memcpy(). But there are functions for this in OVS like nullable_memset() and nullable_memcpy(), maybe some of the changes might benefit from using this. > > In addition, there is also a nullable_string_is_equal() which might be useful in some cases. Ah okay sure, thank you for the suggestions Eelco. I was not aware of the `nullable_` functions. I will look into them when improving this patch. On Tue, Mar 28, 2023 at 6:36 PM Ilya Maximets <i.maximets@ovn.org> wrote: > > I'd also say that the vast majority of changes here should > be replaced with assertions, because they are checking for > impossible conditions. For example, there should be no way > the name and tables are not set after successful parser run > in ovsdb_schema_from_json(), because they are not optional. > Explicit handling of impossible cases is confusing for readers. > Use of assertions will also reduce the patch size significantly. Got it, thank you for the suggestions Ilya. I will incorporate them into the next patch version as soon as I find time to work on it. Best regards, James Raphael Tiovalen
diff --git a/lib/dp-packet.c b/lib/dp-packet.c index ae8ab5800..50cb30681 100644 --- a/lib/dp-packet.c +++ b/lib/dp-packet.c @@ -171,7 +171,11 @@ dp_packet_new_with_headroom(size_t size, size_t headroom) struct dp_packet * dp_packet_clone(const struct dp_packet *buffer) { - return dp_packet_clone_with_headroom(buffer, 0); + if (buffer) { + return dp_packet_clone_with_headroom(buffer, 0); + } else { + return NULL; + } } /* Creates and returns a new dp_packet whose data are copied from 'buffer'. @@ -183,26 +187,32 @@ 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); - /* 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, - sizeof(struct dp_packet) - - offsetof(struct dp_packet, l2_pad_size)); + const void *data_dp = dp_packet_data(buffer); - *dp_packet_ol_flags_ptr(new_buffer) = *dp_packet_ol_flags_ptr(buffer); - *dp_packet_ol_flags_ptr(new_buffer) &= DP_PACKET_OL_SUPPORTED_MASK; + if (data_dp) { + 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, + sizeof(struct dp_packet) - + offsetof(struct dp_packet, l2_pad_size)); - if (dp_packet_rss_valid(buffer)) { - dp_packet_set_rss_hash(new_buffer, dp_packet_get_rss_hash(buffer)); - } - if (dp_packet_has_flow_mark(buffer, &mark)) { - dp_packet_set_flow_mark(new_buffer, mark); - } + *dp_packet_ol_flags_ptr(new_buffer) = *dp_packet_ol_flags_ptr(buffer); + *dp_packet_ol_flags_ptr(new_buffer) &= DP_PACKET_OL_SUPPORTED_MASK; + + if (dp_packet_rss_valid(buffer)) { + dp_packet_set_rss_hash(new_buffer, dp_packet_get_rss_hash(buffer)); + } + if (dp_packet_has_flow_mark(buffer, &mark)) { + dp_packet_set_flow_mark(new_buffer, mark); + } - return new_buffer; + return new_buffer; + } else { + return NULL; + } } /* Creates and returns a new dp_packet that initially contains a copy of the @@ -323,8 +333,11 @@ 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)); - dp_packet_set_data(b, dst); + const void *data_dp = dp_packet_data(b); + if (data_dp) { + memmove(dst, data_dp, dp_packet_size(b)); + dp_packet_set_data(b, dst); + } } } @@ -348,7 +361,9 @@ void * dp_packet_put_zeros(struct dp_packet *b, size_t size) { void *dst = dp_packet_put_uninit(b, size); - memset(dst, 0, size); + if (dst) { + memset(dst, 0, size); + } return dst; } @@ -359,7 +374,9 @@ 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); + if (dst) { + memcpy(dst, p, size); + } return dst; } @@ -431,7 +448,9 @@ void * dp_packet_push_zeros(struct dp_packet *b, size_t size) { void *dst = dp_packet_push_uninit(b, size); - memset(dst, 0, size); + if (dst) { + memset(dst, 0, size); + } return dst; } @@ -442,7 +461,9 @@ 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); + if (dst) { + memcpy(dst, p, size); + } return dst; } diff --git a/lib/dpctl.c b/lib/dpctl.c index c501a0cd7..6472990cc 100644 --- a/lib/dpctl.c +++ b/lib/dpctl.c @@ -336,12 +336,14 @@ dpctl_add_if(int argc OVS_UNUSED, const char *argv[], value = ""; } - if (!strcmp(key, "type")) { - type = value; - } else if (!strcmp(key, "port_no")) { - port_no = u32_to_odp(atoi(value)); - } else if (!smap_add_once(&args, key, value)) { - dpctl_error(dpctl_p, 0, "duplicate \"%s\" option", key); + if (key) { + if (!strcmp(key, "type")) { + type = value; + } else if (!strcmp(key, "port_no")) { + port_no = u32_to_odp(atoi(value)); + } else if (!smap_add_once(&args, key, value)) { + dpctl_error(dpctl_p, 0, "duplicate \"%s\" option", key); + } } } @@ -454,25 +456,29 @@ dpctl_set_if(int argc, const char *argv[], struct dpctl_params *dpctl_p) value = ""; } - if (!strcmp(key, "type")) { - if (strcmp(value, type)) { - dpctl_error(dpctl_p, 0, - "%s: can't change type from %s to %s", - name, type, value); - error = EINVAL; - goto next_destroy_args; - } - } else if (!strcmp(key, "port_no")) { - if (port_no != u32_to_odp(atoi(value))) { - dpctl_error(dpctl_p, 0, "%s: can't change port number from" - " %"PRIu32" to %d", name, port_no, atoi(value)); - error = EINVAL; - goto next_destroy_args; + if (key) { + if (!strcmp(key, "type")) { + if (strcmp(value, type)) { + dpctl_error(dpctl_p, 0, + "%s: can't change type from %s to %s", + name, type, value); + error = EINVAL; + goto next_destroy_args; + } + } else if (!strcmp(key, "port_no")) { + if (port_no != u32_to_odp(atoi(value))) { + dpctl_error(dpctl_p, 0, + "%s: can't change port number from" + " %"PRIu32" to %d", name, port_no, + atoi(value)); + error = EINVAL; + goto next_destroy_args; + } + } else if (value[0] == '\0') { + smap_remove(&args, key); + } else { + smap_replace(&args, key, value); } - } else if (value[0] == '\0') { - smap_remove(&args, key); - } else { - smap_replace(&args, key, value); } } @@ -693,12 +699,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..09683b54f 100644 --- a/lib/json.c +++ b/lib/json.c @@ -342,8 +342,12 @@ json_object(const struct json *json) bool json_boolean(const struct json *json) { - ovs_assert(json->type == JSON_TRUE || json->type == JSON_FALSE); - return json->type == JSON_TRUE; + if (json) { + ovs_assert(json->type == JSON_TRUE || json->type == JSON_FALSE); + return json->type == JSON_TRUE; + } else { + return false; + } } double @@ -497,13 +501,15 @@ json_hash_object(const struct shash *object, size_t basis) size_t n, i; 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) { + 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); + } + free(nodes); } - free(nodes); return basis; } @@ -1654,11 +1660,13 @@ json_serialize_object(const struct shash *object, struct json_serializer *s) size_t n, i; nodes = shash_sort(object); - n = shash_count(object); - for (i = 0; i < n; i++) { - json_serialize_object_member(i, nodes[i], s); + if (nodes) { + n = shash_count(object); + 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..5ece04807 100644 --- a/lib/memory.c +++ b/lib/memory.c @@ -116,13 +116,15 @@ 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]; + 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); + 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..e2dd61880 100644 --- a/lib/netdev-native-tnl.c +++ b/lib/netdev-native-tnl.c @@ -221,16 +221,20 @@ 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))); - } else { - csum = packet_csum_pseudoheader(netdev_tnl_ip_hdr( - dp_packet_data(packet))); - } + void *data_dp = dp_packet_data(packet); + + if (data_dp) { + 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( + data_dp)); + } - csum = csum_continue(csum, udp, ip_tot_size); - udp->udp_csum = csum_finish(csum); + csum = csum_continue(csum, udp, ip_tot_size); + udp->udp_csum = csum_finish(csum); + } if (!udp->udp_csum) { udp->udp_csum = htons(0xffff); @@ -425,20 +429,24 @@ 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)) ? - IPV6_HEADER_LEN : IP_HEADER_LEN; + const void *data_dp = dp_packet_data(packet); - pkt_metadata_init_tnl(md); - if (hlen > dp_packet_size(packet)) { - goto err; - } + if (data_dp) { + hlen += netdev_tnl_is_header_ipv6(data_dp) ? + IPV6_HEADER_LEN : IP_HEADER_LEN; - hlen = parse_gre_header(packet, tnl); - if (hlen < 0) { - goto err; - } + pkt_metadata_init_tnl(md); + if (hlen > dp_packet_size(packet)) { + goto err; + } - dp_packet_reset_packet(packet, hlen); + hlen = parse_gre_header(packet, tnl); + if (hlen < 0) { + goto err; + } + + dp_packet_reset_packet(packet, hlen); + } return packet; err: diff --git a/lib/odp-execute.c b/lib/odp-execute.c index 5cf6fbec0..b8cf3db55 100644 --- a/lib/odp-execute.c +++ b/lib/odp-execute.c @@ -147,42 +147,45 @@ odp_set_ipv4(struct dp_packet *packet, const struct ovs_key_ipv4 *key, uint8_t new_tos; uint8_t new_ttl; - 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); + if (nh) { + if (mask->ipv4_src) { + ip_src_nh = get_16aligned_be32(&nh->ip_src); + new_ip_src = key->ipv4_src | (ip_src_nh & ~mask->ipv4_src); - if (ip_src_nh != new_ip_src) { - packet_set_ipv4_addr(packet, &nh->ip_src, new_ip_src); + if (ip_src_nh != new_ip_src) { + packet_set_ipv4_addr(packet, &nh->ip_src, new_ip_src); + } } - } - if (mask->ipv4_dst) { - ip_dst_nh = get_16aligned_be32(&nh->ip_dst); - new_ip_dst = key->ipv4_dst | (ip_dst_nh & ~mask->ipv4_dst); + if (mask->ipv4_dst) { + ip_dst_nh = get_16aligned_be32(&nh->ip_dst); + new_ip_dst = key->ipv4_dst | (ip_dst_nh & ~mask->ipv4_dst); - if (ip_dst_nh != new_ip_dst) { - packet_set_ipv4_addr(packet, &nh->ip_dst, new_ip_dst); + if (ip_dst_nh != new_ip_dst) { + packet_set_ipv4_addr(packet, &nh->ip_dst, new_ip_dst); + } } - } - if (mask->ipv4_tos) { - new_tos = key->ipv4_tos | (nh->ip_tos & ~mask->ipv4_tos); + if (mask->ipv4_tos) { + new_tos = key->ipv4_tos | (nh->ip_tos & ~mask->ipv4_tos); - if (nh->ip_tos != new_tos) { - nh->ip_csum = recalc_csum16(nh->ip_csum, - htons((uint16_t) nh->ip_tos), - htons((uint16_t) new_tos)); - nh->ip_tos = new_tos; + if (nh->ip_tos != new_tos) { + nh->ip_csum = recalc_csum16(nh->ip_csum, + htons((uint16_t) nh->ip_tos), + htons((uint16_t) new_tos)); + nh->ip_tos = new_tos; + } } - } - if (OVS_LIKELY(mask->ipv4_ttl)) { - new_ttl = key->ipv4_ttl | (nh->ip_ttl & ~mask->ipv4_ttl); + if (OVS_LIKELY(mask->ipv4_ttl)) { + new_ttl = key->ipv4_ttl | (nh->ip_ttl & ~mask->ipv4_ttl); - if (OVS_LIKELY(nh->ip_ttl != new_ttl)) { - nh->ip_csum = recalc_csum16(nh->ip_csum, htons(nh->ip_ttl << 8), - htons(new_ttl << 8)); - nh->ip_ttl = new_ttl; + if (OVS_LIKELY(nh->ip_ttl != new_ttl)) { + nh->ip_csum = recalc_csum16(nh->ip_csum, + htons(nh->ip_ttl << 8), + htons(new_ttl << 8)); + nh->ip_ttl = new_ttl; + } } } } @@ -276,23 +279,25 @@ set_arp(struct dp_packet *packet, const struct ovs_key_arp *key, { struct arp_eth_header *arp = dp_packet_l3(packet); - if (!mask) { - arp->ar_op = key->arp_op; - arp->ar_sha = key->arp_sha; - put_16aligned_be32(&arp->ar_spa, key->arp_sip); - arp->ar_tha = key->arp_tha; - put_16aligned_be32(&arp->ar_tpa, key->arp_tip); - } else { - ovs_be32 ar_spa = get_16aligned_be32(&arp->ar_spa); - ovs_be32 ar_tpa = get_16aligned_be32(&arp->ar_tpa); - - arp->ar_op = key->arp_op | (arp->ar_op & ~mask->arp_op); - ether_addr_copy_masked(&arp->ar_sha, key->arp_sha, mask->arp_sha); - put_16aligned_be32(&arp->ar_spa, - key->arp_sip | (ar_spa & ~mask->arp_sip)); - ether_addr_copy_masked(&arp->ar_tha, key->arp_tha, mask->arp_tha); - put_16aligned_be32(&arp->ar_tpa, - key->arp_tip | (ar_tpa & ~mask->arp_tip)); + if (arp) { + if (!mask) { + arp->ar_op = key->arp_op; + arp->ar_sha = key->arp_sha; + put_16aligned_be32(&arp->ar_spa, key->arp_sip); + arp->ar_tha = key->arp_tha; + put_16aligned_be32(&arp->ar_tpa, key->arp_tip); + } else { + ovs_be32 ar_spa = get_16aligned_be32(&arp->ar_spa); + ovs_be32 ar_tpa = get_16aligned_be32(&arp->ar_tpa); + + arp->ar_op = key->arp_op | (arp->ar_op & ~mask->arp_op); + ether_addr_copy_masked(&arp->ar_sha, key->arp_sha, mask->arp_sha); + put_16aligned_be32(&arp->ar_spa, + key->arp_sip | (ar_spa & ~mask->arp_sip)); + ether_addr_copy_masked(&arp->ar_tha, key->arp_tha, mask->arp_tha); + put_16aligned_be32(&arp->ar_tpa, + key->arp_tip | (ar_tpa & ~mask->arp_tip)); + } } } diff --git a/lib/pcap-file.c b/lib/pcap-file.c index 3ed7ea488..7cb6f5b2a 100644 --- a/lib/pcap-file.c +++ b/lib/pcap-file.c @@ -291,7 +291,10 @@ 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); + if (data_dp) { + 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..67a0134ed 100644 --- a/lib/rtnetlink.c +++ b/lib/rtnetlink.c @@ -131,10 +131,13 @@ rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change) change->irrelevant = true; } + if (ifinfo) { + change->if_index = ifinfo->ifi_index; + change->ifi_flags = ifinfo->ifi_flags; + } + change->nlmsg_type = nlmsg->nlmsg_type; - change->if_index = ifinfo->ifi_index; change->ifname = nl_attr_get_string(attrs[IFLA_IFNAME]); - change->ifi_flags = ifinfo->ifi_flags; change->master_ifindex = (attrs[IFLA_MASTER] ? nl_attr_get_u32(attrs[IFLA_MASTER]) : 0); @@ -178,7 +181,9 @@ rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change) ifaddr = ofpbuf_at(buf, NLMSG_HDRLEN, sizeof *ifaddr); change->nlmsg_type = nlmsg->nlmsg_type; - change->if_index = ifaddr->ifa_index; + if (ifaddr) { + change->if_index = ifaddr->ifa_index; + } change->ifname = (attrs[IFA_LABEL] ? nl_attr_get_string(attrs[IFA_LABEL]) : NULL); diff --git a/lib/sflow_agent.c b/lib/sflow_agent.c index c95f654a5..c19434d6c 100644 --- a/lib/sflow_agent.c +++ b/lib/sflow_agent.c @@ -152,15 +152,24 @@ void sfl_agent_tick(SFLAgent *agent, time_t now) SFLReceiver *sfl_agent_addReceiver(SFLAgent *agent) { - SFLReceiver *rcv = (SFLReceiver *)sflAlloc(agent, sizeof(SFLReceiver)); - sfl_receiver_init(rcv, agent); - /* add to end of list - to preserve the receiver index numbers for existing receivers */ - { - SFLReceiver *r, *prev = NULL; - for(r = agent->receivers; r != NULL; prev = r, r = r->nxt); - if(prev) prev->nxt = rcv; - else agent->receivers = rcv; - rcv->nxt = NULL; + SFLReceiver *rcv = (SFLReceiver *) sflAlloc(agent, sizeof(SFLReceiver)); + if (rcv) { + sfl_receiver_init(rcv, agent); + /* add to end of list - to preserve the receiver index numbers for + * existing receivers + */ + { + SFLReceiver *r, *prev = NULL; + for (r = agent->receivers; r != NULL; prev = r, r = r->nxt) { + /* nothing to do */ + } + if (prev) { + prev->nxt = rcv; + } else { + agent->receivers = rcv; + } + rcv->nxt = NULL; + } } return rcv; } @@ -202,23 +211,35 @@ SFLSampler *sfl_agent_addSampler(SFLAgent *agent, SFLDataSource_instance *pdsi) /* either we found the insert point, or reached the end of the list...*/ { - SFLSampler *newsm = (SFLSampler *)sflAlloc(agent, sizeof(SFLSampler)); - sfl_sampler_init(newsm, agent, pdsi); - if(prev) prev->nxt = newsm; - else agent->samplers = newsm; - newsm->nxt = sm; - - /* see if we should go in the ifIndex jumpTable */ - if(SFL_DS_CLASS(newsm->dsi) == 0) { - SFLSampler *test = sfl_agent_getSamplerByIfIndex(agent, SFL_DS_INDEX(newsm->dsi)); - if(test && (SFL_DS_INSTANCE(newsm->dsi) < SFL_DS_INSTANCE(test->dsi))) { - /* replace with this new one because it has a lower ds_instance number */ - sfl_agent_jumpTableRemove(agent, test); - test = NULL; - } - if(test == NULL) sfl_agent_jumpTableAdd(agent, newsm); - } - return newsm; + SFLSampler *newsm = (SFLSampler *) sflAlloc(agent, sizeof(SFLSampler)); + if (newsm) { + memset(newsm, 0, sizeof(*newsm)); + sfl_sampler_init(newsm, agent, pdsi); + if (prev) { + prev->nxt = newsm; + } else { + agent->samplers = newsm; + } + newsm->nxt = sm; + + /* see if we should go in the ifIndex jumpTable */ + if (SFL_DS_CLASS(newsm->dsi) == 0) { + SFLSampler *test = sfl_agent_getSamplerByIfIndex(agent, + SFL_DS_INDEX(newsm->dsi)); + if (test && (SFL_DS_INSTANCE(newsm->dsi) + < SFL_DS_INSTANCE(test->dsi))) { + /* replace with this new one because it has a lower + * ds_instance number + */ + sfl_agent_jumpTableRemove(agent, test); + test = NULL; + } + if (test == NULL) { + sfl_agent_jumpTableAdd(agent, newsm); + } + } + } + return newsm; } } @@ -241,12 +262,18 @@ 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)); - sfl_poller_init(newpl, agent, pdsi, magic, getCountersFn); - if(prev) prev->nxt = newpl; - else agent->pollers = newpl; - newpl->nxt = pl; - return newpl; + SFLPoller *newpl = (SFLPoller *) sflAlloc(agent, sizeof(SFLPoller)); + if (newpl) { + memset(newpl, 0, sizeof(*newpl)); + sfl_poller_init(newpl, agent, pdsi, magic, getCountersFn); + if (prev) { + prev->nxt = newpl; + } else { + agent->pollers = newpl; + } + newpl->nxt = pl; + } + return newpl; } } diff --git a/lib/shash.c b/lib/shash.c index a7b2c6458..adb388cf7 100644 --- a/lib/shash.c +++ b/lib/shash.c @@ -194,7 +194,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..50f6676df 100644 --- a/lib/sset.c +++ b/lib/sset.c @@ -261,8 +261,11 @@ char * sset_pop(struct sset *set) { const char *name = SSET_FIRST(set); - char *copy = xstrdup(name); - sset_delete(set, SSET_NODE_FROM_NAME(name)); + char *copy = NULL; + if (name) { + 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..f36531669 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; + 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..bccb76098 100644 --- a/ovsdb/file.c +++ b/ovsdb/file.c @@ -522,7 +522,12 @@ 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]; if (table != ftxn->table) { @@ -533,14 +538,23 @@ ovsdb_file_txn_add_row(struct ovsdb_file_txn *ftxn, /* Create JSON object for transaction on this table. */ ftxn->table_json = json_object_create(); - ftxn->table = table; - json_object_put(ftxn->json, table->schema->name, ftxn->table_json); + if (table) { + ftxn->table = table; + json_object_put(ftxn->json, table->schema->name, + ftxn->table_json); + } } /* 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..b1cbaa227 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,7 @@ 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..1ea92cc0f 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,7 +1285,9 @@ ovsdb_monitor_table_add_select(struct ovsdb_monitor *dbmon, struct ovsdb_monitor_table * mt; mt = shash_find_data(&dbmon->tables, table->schema->name); - mt->select |= select; + if (mt) { + mt->select |= select; + } } /* @@ -1329,8 +1337,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,15 +1680,21 @@ 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; - basis = hash_pointer(mt->table, basis); - basis = hash_3words(mt->select, mt->n_columns, basis); + if (mt) { + basis = hash_pointer(mt->table, basis); + basis = hash_3words(mt->select, mt->n_columns, basis); - for (j = 0; j < mt->n_columns; j++) { - basis = hash_pointer(mt->columns[j].column, basis); - basis = hash_2words(mt->columns[j].select, basis); + for (j = 0; j < mt->n_columns; j++) { + basis = hash_pointer(mt->columns[j].column, basis); + basis = hash_2words(mt->columns[j].select, basis); + } } } free(nodes); diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c index f1b8d6491..ca2becd8d 100644 --- a/ovsdb/ovsdb-client.c +++ b/ovsdb/ovsdb-client.c @@ -1223,17 +1223,25 @@ 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 +1447,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 +1880,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 +1943,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..02e024de4 100644 --- a/ovsdb/ovsdb-server.c +++ b/ovsdb/ovsdb-server.c @@ -1784,14 +1784,16 @@ 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 +2193,7 @@ 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..6dcfc5751 100644 --- a/ovsdb/ovsdb.c +++ b/ovsdb/ovsdb.c @@ -195,7 +195,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,78 +215,86 @@ ovsdb_schema_from_json(const struct json *json, struct ovsdb_schema **schemap) return error; } - if (version_json) { - version = json_string(version_json); - if (!ovsdb_is_valid_version(version)) { - return ovsdb_syntax_error(json, NULL, "schema version \"%s\" not " - "in format x.y.z", version); + if (name && tables) { + if (version_json) { + version = json_string(version_json); + if (!ovsdb_is_valid_version(version)) { + return ovsdb_syntax_error(json, NULL, + "schema version \"%s\" not " + "in format x.y.z", version); + } + } else { + /* Backward compatibility with old databases. */ + version = ""; } - } else { - /* Backward compatibility with old databases. */ - version = ""; - } - schema = ovsdb_schema_create(json_string(name), version, - cksum ? json_string(cksum) : ""); - SHASH_FOR_EACH (node, json_object(tables)) { - struct ovsdb_table_schema *table; + schema = ovsdb_schema_create(json_string(name), version, + cksum ? json_string(cksum) : ""); + SHASH_FOR_EACH (node, json_object(tables)) { + struct ovsdb_table_schema *table; + + if (node->name[0] == '_') { + error = ovsdb_syntax_error(json, NULL, "names beginning with " + "\"_\" are reserved"); + } else if (!ovsdb_parser_is_id(node->name)) { + error = ovsdb_syntax_error(json, NULL, + "name must be a valid id"); + } else { + error = ovsdb_table_schema_from_json(node->data, node->name, + &table); + } + if (error) { + ovsdb_schema_destroy(schema); + return error; + } - if (node->name[0] == '_') { - error = ovsdb_syntax_error(json, NULL, "names beginning with " - "\"_\" are reserved"); - } else if (!ovsdb_parser_is_id(node->name)) { - error = ovsdb_syntax_error(json, NULL, "name must be a valid id"); - } else { - error = ovsdb_table_schema_from_json(node->data, node->name, - &table); - } - if (error) { - ovsdb_schema_destroy(schema); - return error; + shash_add(&schema->tables, table->name, table); } - - shash_add(&schema->tables, table->name, table); } - /* "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 - * in the root set. */ - if (root_set_size(schema) == 0) { - SHASH_FOR_EACH (node, &schema->tables) { - struct ovsdb_table_schema *table = node->data; - - table->is_root = true; + if (schema) { + /* "isRoot" was not part of the original schema definition. Before it + * was added, there was no support for garbage collection. So, for + * backward compatibility, if the root set is empty then assume that + * every table is in the root set. */ + if (root_set_size(schema) == 0) { + SHASH_FOR_EACH (node, &schema->tables) { + struct ovsdb_table_schema *table = node->data; + + table->is_root = true; + } } - } - /* Validate that all refTables refer to the names of tables that exist. - * - * Also force certain columns to be persistent, as explained in - * ovsdb_schema_check_ref_table(). This requires 'is_root' to be known, so - * this must follow the loop updating 'is_root' above. */ - SHASH_FOR_EACH (node, &schema->tables) { - struct ovsdb_table_schema *table = node->data; - struct shash_node *node2; + /* Validate that all refTables refer to the names of tables that exist. + * + * Also force certain columns to be persistent, as explained in + * ovsdb_schema_check_ref_table(). This requires 'is_root' to be + * known, so this must follow the loop updating 'is_root' above. */ + SHASH_FOR_EACH (node, &schema->tables) { + struct ovsdb_table_schema *table = node->data; + struct shash_node *node2; - SHASH_FOR_EACH (node2, &table->columns) { - struct ovsdb_column *column = node2->data; + SHASH_FOR_EACH (node2, &table->columns) { + struct ovsdb_column *column = node2->data; - error = ovsdb_schema_check_ref_table(column, &schema->tables, - &column->type.key, "key"); - if (!error) { error = ovsdb_schema_check_ref_table(column, &schema->tables, - &column->type.value, - "value"); - } - if (error) { - ovsdb_schema_destroy(schema); - return error; + &column->type.key, "key"); + if (!error) { + error = ovsdb_schema_check_ref_table(column, + &schema->tables, + &column->type.value, + "value"); + } + if (error) { + ovsdb_schema_destroy(schema); + return error; + } } } + + *schemap = schema; } - *schemap = schema; return NULL; } diff --git a/ovsdb/query.c b/ovsdb/query.c index eebe56412..0129eb038 100644 --- a/ovsdb/query.c +++ b/ovsdb/query.c @@ -91,7 +91,9 @@ ovsdb_query_distinct(struct ovsdb_table *table, struct ovsdb_row_hash hash; ovsdb_row_hash_init(&hash, columns); - ovsdb_query(table, condition, query_distinct_cb, &hash); + if (condition) { + ovsdb_query(table, condition, query_distinct_cb, &hash); + } HMAP_FOR_EACH (node, hmap_node, &hash.rows) { ovsdb_row_set_add_row(results, node->row); } diff --git a/ovsdb/replication.c b/ovsdb/replication.c index 477c69d70..f5e150c33 100644 --- a/ovsdb/replication.c +++ b/ovsdb/replication.c @@ -575,15 +575,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..43b104f31 100644 --- a/ovsdb/transaction.c +++ b/ovsdb/transaction.c @@ -576,9 +576,11 @@ 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(!ovsdb_row_find_weak_ref(dst_row, weak)); - hmap_insert(&dst_row->dst_refs, &weak->dst_node, - ovsdb_weak_ref_hash(weak)); + if (dst_row) { + ovs_assert(!ovsdb_row_find_weak_ref(dst_row, weak)); + hmap_insert(&dst_row->dst_refs, &weak->dst_node, + ovsdb_weak_ref_hash(weak)); + } ovs_list_remove(&weak->src_node); ovs_list_init(&weak->src_node); } diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c index 2f5ac1a26..991b426b9 100644 --- a/utilities/ovs-vsctl.c +++ b/utilities/ovs-vsctl.c @@ -334,8 +334,11 @@ parse_options(int argc, char *argv[], struct shash *local_options) exit(EXIT_SUCCESS); case 't': - if (!str_to_uint(optarg, 10, &timeout) || !timeout) { - ctl_fatal("value %s on -t or --timeout is invalid", optarg); + if (optarg) { + if (!str_to_uint(optarg, 10, &timeout) || !timeout) { + ctl_fatal("value %s on -t or --timeout is invalid", + optarg); + } } break; @@ -349,11 +352,15 @@ parse_options(int argc, char *argv[], struct shash *local_options) STREAM_SSL_OPTION_HANDLERS case OPT_PEER_CA_CERT: - stream_ssl_set_peer_ca_cert_file(optarg); + if (optarg) { + stream_ssl_set_peer_ca_cert_file(optarg); + } break; case OPT_BOOTSTRAP_CA_CERT: - stream_ssl_set_ca_cert_file(optarg, true); + if (optarg) { + stream_ssl_set_ca_cert_file(optarg, true); + } break; case '?': @@ -575,7 +582,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 +666,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,11 +678,13 @@ add_port_to_cache(struct vsctl_context *vsctl_ctx, struct vsctl_bridge *parent, } } - port = xmalloc(sizeof *port); - ovs_list_push_back(&parent->ports, &port->ports_node); + if (parent) { + ovs_list_push_back(&parent->ports, &port->ports_node); + port->bridge = parent; + } + ovs_list_init(&port->ifaces); port->port_cfg = port_cfg; - port->bridge = parent; shash_add(&vsctl_ctx->ports, port_cfg->name, port); return port; @@ -818,55 +827,63 @@ vsctl_context_populate_cache(struct ctl_context *ctx) continue; } br = shash_find_data(&vsctl_ctx->bridges, br_cfg->name); - for (j = 0; j < br_cfg->n_ports; j++) { - struct ovsrec_port *port_cfg = br_cfg->ports[j]; - struct vsctl_port *port; - size_t k; - - port = shash_find_data(&vsctl_ctx->ports, port_cfg->name); - if (port) { - if (port_cfg == port->port_cfg) { - VLOG_WARN("%s: port is in multiple bridges (%s and %s)", - port_cfg->name, br->name, port->bridge->name); - } else { - /* Log as an error because this violates the database's - * uniqueness constraints, so the database server shouldn't - * have allowed it. */ - VLOG_ERR("%s: database contains duplicate port name", - port_cfg->name); - } - continue; - } - - if (port_is_fake_bridge(port_cfg) - && !sset_add(&bridges, port_cfg->name)) { - continue; - } - - port = add_port_to_cache(vsctl_ctx, br, port_cfg); - for (k = 0; k < port_cfg->n_interfaces; k++) { - struct ovsrec_interface *iface_cfg = port_cfg->interfaces[k]; - struct vsctl_iface *iface; - - iface = shash_find_data(&vsctl_ctx->ifaces, iface_cfg->name); - if (iface) { - if (iface_cfg == iface->iface_cfg) { - VLOG_WARN("%s: interface is in multiple ports " - "(%s and %s)", - iface_cfg->name, - iface->port->port_cfg->name, - port->port_cfg->name); + if (br) { + for (j = 0; j < br_cfg->n_ports; j++) { + struct ovsrec_port *port_cfg = br_cfg->ports[j]; + struct vsctl_port *port; + size_t k; + + port = shash_find_data(&vsctl_ctx->ports, port_cfg->name); + if (port) { + if (port_cfg == port->port_cfg) { + VLOG_WARN("%s: port is in multiple bridges " + "(%s and %s)", port_cfg->name, br->name, + port->bridge->name); } else { /* Log as an error because this violates the database's - * uniqueness constraints, so the database server - * shouldn't have allowed it. */ - VLOG_ERR("%s: database contains duplicate interface " - "name", iface_cfg->name); + * uniqueness constraints, so the database server + * shouldn't have allowed it. */ + VLOG_ERR("%s: database contains duplicate port name", + port_cfg->name); } continue; } - add_iface_to_cache(vsctl_ctx, port, iface_cfg); + if (port_is_fake_bridge(port_cfg) + && !sset_add(&bridges, port_cfg->name)) { + continue; + } + + port = add_port_to_cache(vsctl_ctx, br, port_cfg); + if (port) { + for (k = 0; k < port_cfg->n_interfaces; k++) { + struct ovsrec_interface *iface_cfg = + port_cfg->interfaces[k]; + struct vsctl_iface *iface; + + iface = shash_find_data(&vsctl_ctx->ifaces, + iface_cfg->name); + if (iface) { + if (iface_cfg == iface->iface_cfg) { + VLOG_WARN("%s: interface is in multiple ports " + "(%s and %s)", + iface_cfg->name, + iface->port->port_cfg->name, + port->port_cfg->name); + } else { + /* Log as an error because this violates the + * database's uniqueness constraints, so the + * database server shouldn't have allowed it. + */ + VLOG_ERR("%s: database contains duplicate " + "interface name", iface_cfg->name); + } + continue; + } + + add_iface_to_cache(vsctl_ctx, port, iface_cfg); + } + } } } } @@ -889,14 +906,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 +962,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 +980,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 +1706,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..356840089 100644 --- a/vtep/vtep-ctl.c +++ b/vtep/vtep-ctl.c @@ -250,8 +250,11 @@ parse_options(int argc, char *argv[], struct shash *local_options) exit(EXIT_SUCCESS); case 't': - if (!str_to_uint(optarg, 10, &timeout) || !timeout) { - ctl_fatal("value %s on -t or --timeout is invalid", optarg); + if (optarg) { + if (!str_to_uint(optarg, 10, &timeout) || !timeout) { + ctl_fatal("value %s on -t or --timeout is invalid", + optarg); + } } break; @@ -1065,42 +1068,46 @@ vtep_ctl_context_populate_cache(struct ctl_context *ctx) continue; } ps = shash_find_data(&vtepctl_ctx->pswitches, ps_cfg->name); - for (j = 0; j < ps_cfg->n_ports; j++) { - struct vteprec_physical_port *port_cfg = ps_cfg->ports[j]; - struct vtep_ctl_port *port; - size_t k; - - port = shash_find_data(&vtepctl_ctx->ports, port_cfg->name); - if (port) { - if (port_cfg == port->port_cfg) { - VLOG_WARN("%s: port is in multiple physical switches " - "(%s and %s)", - port_cfg->name, ps->name, port->ps->name); - } else { - /* Log as an error because this violates the database's - * uniqueness constraints, so the database server shouldn't - * have allowed it. */ - VLOG_ERR("%s: database contains duplicate port name", - port_cfg->name); + if (ps) { + for (j = 0; j < ps_cfg->n_ports; j++) { + struct vteprec_physical_port *port_cfg = ps_cfg->ports[j]; + struct vtep_ctl_port *port; + size_t k; + + port = shash_find_data(&vtepctl_ctx->ports, port_cfg->name); + if (port) { + if (port_cfg == port->port_cfg) { + VLOG_WARN("%s: port is in multiple physical switches " + "(%s and %s)", + port_cfg->name, ps->name, port->ps->name); + } else { + /* Log as an error because this violates the database's + * uniqueness constraints, so the database server + * shouldn't have allowed it. */ + VLOG_ERR("%s: database contains duplicate port name", + port_cfg->name); + } + continue; } - continue; - } - port = add_port_to_cache(vtepctl_ctx, ps, port_cfg); + port = add_port_to_cache(vtepctl_ctx, ps, port_cfg); + if (port) { + for (k = 0; k < port_cfg->n_vlan_bindings; k++) { + struct vtep_ctl_lswitch *ls; + char *vlan; - for (k = 0; k < port_cfg->n_vlan_bindings; k++) { - struct vtep_ctl_lswitch *ls; - char *vlan; + vlan = xasprintf("%"PRId64, + port_cfg->key_vlan_bindings[k]); + if (shash_find(&port->bindings, vlan)) { + ctl_fatal("multiple bindings for vlan %s", vlan); + } - vlan = xasprintf("%"PRId64, port_cfg->key_vlan_bindings[k]); - if (shash_find(&port->bindings, vlan)) { - ctl_fatal("multiple bindings for vlan %s", vlan); - } - - ls_cfg = port_cfg->value_vlan_bindings[k]; - ls = find_lswitch(vtepctl_ctx, ls_cfg->name, true); + ls_cfg = port_cfg->value_vlan_bindings[k]; + ls = find_lswitch(vtepctl_ctx, ls_cfg->name, true); - shash_add_nocopy(&port->bindings, vlan, ls); + shash_add_nocopy(&port->bindings, vlan, ls); + } + } } } } @@ -1892,8 +1899,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. 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> --- 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 | 69 ++++++++++++------ lib/dpctl.c | 66 +++++++++-------- lib/json.c | 32 +++++--- lib/latch-unix.c | 2 +- lib/memory.c | 10 ++- lib/netdev-native-tnl.c | 48 +++++++----- lib/odp-execute.c | 91 ++++++++++++----------- lib/pcap-file.c | 5 +- lib/rtnetlink.c | 11 ++- lib/sflow_agent.c | 91 +++++++++++++++-------- lib/shash.c | 4 +- lib/sset.c | 7 +- ovsdb/condition.c | 10 ++- ovsdb/file.c | 26 +++++-- ovsdb/jsonrpc-server.c | 5 +- ovsdb/monitor.c | 49 ++++++++++--- ovsdb/ovsdb-client.c | 45 ++++++++---- ovsdb/ovsdb-server.c | 15 ++-- ovsdb/ovsdb-util.c | 9 +++ ovsdb/ovsdb.c | 124 ++++++++++++++++--------------- ovsdb/query.c | 4 +- ovsdb/replication.c | 14 ++-- ovsdb/row.c | 4 +- ovsdb/table.c | 2 +- ovsdb/transaction.c | 8 +- utilities/ovs-vsctl.c | 157 ++++++++++++++++++++++++---------------- vtep/vtep-ctl.c | 79 +++++++++++--------- 27 files changed, 607 insertions(+), 380 deletions(-)