diff mbox series

[ovs-dev,v2,10/11] treewide: Avoid offsetting NULL pointers.

Message ID 20211221110334.14345.77207.stgit@dceara.remote.csb
State Changes Requested
Headers show
Series Fix UndefinedBehaviorSanitizer reported issues and enable it in CI. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed

Commit Message

Dumitru Ceara Dec. 21, 2021, 11:03 a.m. UTC
This is undefined behavior and was reported by UB Sanitizer:
  lib/meta-flow.c:3445:16: runtime error: member access within null pointer of type 'struct vl_mf_field'
      #0 0x6aad0f in mf_get_vl_mff lib/meta-flow.c:3445
      #1 0x6d96d7 in mf_from_oxm_header lib/nx-match.c:260
      #2 0x6d9e2e in nx_pull_header__ lib/nx-match.c:341
      #3 0x6daafa in nx_pull_header lib/nx-match.c:488
      #4 0x6abcb6 in mf_vl_mff_nx_pull_header lib/meta-flow.c:3605
      #5 0x73b9be in decode_NXAST_RAW_REG_MOVE lib/ofp-actions.c:2652
      #6 0x764ccd in ofpact_decode lib/ofp-actions.inc2:4681
      [...]
  lib/sset.c:315:12: runtime error: applying zero offset to null pointer
      #0 0xcc2e6a in sset_at_position /root/ovs/lib/sset.c:315:12
      #1 0x5734b3 in port_dump_next /root/ovs/ofproto/ofproto-dpif.c:4083:20
      [...]
  lib/ovsdb-data.c:2194:56: runtime error: applying zero offset to null pointer
      #0 0x5e9530 in ovsdb_datum_added_removed /root/ovs/lib/ovsdb-data.c:2194:56
      #1 0x4d6258 in update_row_ref_count /root/ovs/ovsdb/transaction.c:335:17
      #2 0x4c360b in for_each_txn_row /root/ovs/ovsdb/transaction.c:1572:33
      [...]
  lib/ofpbuf.c:440:30: runtime error: applying zero offset to null pointer
      #0 0x75066d in ofpbuf_push_uninit lib/ofpbuf.c:440
      #1 0x46ac8a in ovnacts_parse lib/actions.c:4190
      #2 0x46ad91 in ovnacts_parse_string lib/actions.c:4208
      #3 0x4106d1 in test_parse_actions tests/test-ovn.c:1324
      [...]
  lib/ofp-actions.c:3205:22: runtime error: applying non-zero offset 2 to null pointer
      #0 0x6e1641 in set_field_split_str /root/ovs/lib/ofp-actions.c:3205:22
      [...]
  lib/tnl-ports.c:74:12: runtime error: applying zero offset to null pointer
      #0 0xceffe7 in tnl_port_cast /root/ovs/lib/tnl-ports.c:74:12
      #1 0xcf14c3 in map_insert /root/ovs/lib/tnl-ports.c:116:13
      [...]
  ofproto/ofproto.c:8905:16: runtime error: applying zero offset to null pointer
      #0 0x556795 in eviction_group_hash_rule /root/ovs/ofproto/ofproto.c:8905:16
      #1 0x503f8d in eviction_group_add_rule /root/ovs/ofproto/ofproto.c:9022:42
      [...]

Also, it's valid to have an empty ofpact list and we should be able to
try to iterate through it.

UB Sanitizer report:
  include/openvswitch/ofp-actions.h:222:12: runtime error: applying zero offset to null pointer
      #0 0x665d69 in ofpact_end /root/ovs/./include/openvswitch/ofp-actions.h:222:12
      #1 0x66b2cf in ofpacts_put_openflow_actions /root/ovs/lib/ofp-actions.c:8861:5
      #2 0x6ffdd1 in ofputil_encode_flow_mod /root/ovs/lib/ofp-flow.c:447:9
      [...]

Signed-off-by: Dumitru Ceara <dceara@redhat.com>
---
 include/openvswitch/ofp-actions.h |    4 +++-
 include/openvswitch/ofpbuf.h      |   17 ++++++++++++-----
 lib/dynamic-string.c              |   10 ++++++++--
 lib/meta-flow.c                   |    4 +++-
 lib/ofp-actions.c                 |    8 ++++----
 lib/ofpbuf.c                      |    4 ++++
 lib/ovsdb-data.c                  |   37 +++++++++++++++++++++----------------
 lib/ovsdb-data.h                  |    4 ++++
 lib/sset.c                        |    4 +++-
 lib/tnl-ports.c                   |    2 +-
 ofproto/ofproto.c                 |    2 +-
 11 files changed, 64 insertions(+), 32 deletions(-)
diff mbox series

Patch

diff --git a/include/openvswitch/ofp-actions.h b/include/openvswitch/ofp-actions.h
index 3b67675fe02a..0991a0177411 100644
--- a/include/openvswitch/ofp-actions.h
+++ b/include/openvswitch/ofp-actions.h
@@ -219,7 +219,9 @@  struct ofpact *ofpact_next_flattened(const struct ofpact *);
 static inline struct ofpact *
 ofpact_end(const struct ofpact *ofpacts, size_t ofpacts_len)
 {
-    return ALIGNED_CAST(struct ofpact *, (uint8_t *) ofpacts + ofpacts_len);
+    return ofpacts
+           ? ALIGNED_CAST(struct ofpact *, (uint8_t *) ofpacts + ofpacts_len)
+           : NULL;
 }
 
 static inline bool
diff --git a/include/openvswitch/ofpbuf.h b/include/openvswitch/ofpbuf.h
index 1136ba04c84e..a9ff8a56e81c 100644
--- a/include/openvswitch/ofpbuf.h
+++ b/include/openvswitch/ofpbuf.h
@@ -179,7 +179,9 @@  static inline void ofpbuf_delete(struct ofpbuf *b)
 static inline void *ofpbuf_at(const struct ofpbuf *b, size_t offset,
                               size_t size)
 {
-    return offset + size <= b->size ? (char *) b->data + offset : NULL;
+    return offset + size <= b->size && b->data
+           ? (char *) b->data + offset
+           : NULL;
 }
 
 /* Returns a pointer to byte 'offset' in 'b', which must contain at least
@@ -188,20 +190,20 @@  static inline void *ofpbuf_at_assert(const struct ofpbuf *b, size_t offset,
                                      size_t size)
 {
     ovs_assert(offset + size <= b->size);
-    return ((char *) b->data) + offset;
+    return b->data ? (char *) b->data + offset : NULL;
 }
 
 /* Returns a pointer to byte following the last byte of data in use in 'b'. */
 static inline void *ofpbuf_tail(const struct ofpbuf *b)
 {
-    return (char *) b->data + b->size;
+    return b->data ? (char *) b->data + b->size : NULL;
 }
 
 /* Returns a pointer to byte following the last byte allocated for use (but
  * not necessarily in use) in 'b'. */
 static inline void *ofpbuf_end(const struct ofpbuf *b)
 {
-    return (char *) b->base + b->allocated;
+    return b->base ? (char *) b->base + b->allocated : NULL;
 }
 
 /* Returns the number of bytes of headroom in 'b', that is, the number of bytes
@@ -249,6 +251,11 @@  static inline void *ofpbuf_pull(struct ofpbuf *b, size_t size)
 {
     ovs_assert(b->size >= size);
     void *data = b->data;
+
+    if (!size) {
+        return data;
+    }
+
     b->data = (char*)b->data + size;
     b->size = b->size - size;
     return data;
@@ -270,7 +277,7 @@  static inline struct ofpbuf *ofpbuf_from_list(const struct ovs_list *list)
 static inline bool ofpbuf_equal(const struct ofpbuf *a, const struct ofpbuf *b)
 {
     return a->size == b->size &&
-           memcmp(a->data, b->data, a->size) == 0;
+           (a->size == 0 || memcmp(a->data, b->data, a->size) == 0);
 }
 
 static inline bool ofpbuf_oversized(const struct ofpbuf *ofpacts)
diff --git a/lib/dynamic-string.c b/lib/dynamic-string.c
index fd0127ed1740..9b9a8f50e562 100644
--- a/lib/dynamic-string.c
+++ b/lib/dynamic-string.c
@@ -152,7 +152,10 @@  ds_put_format_valist(struct ds *ds, const char *format, va_list args_)
 
     va_copy(args, args_);
     available = ds->string ? ds->allocated - ds->length + 1 : 0;
-    needed = vsnprintf(&ds->string[ds->length], available, format, args);
+    needed = vsnprintf(ds->string
+                       ? &ds->string[ds->length]
+                       : NULL,
+                       available, format, args);
     va_end(args);
 
     if (needed < available) {
@@ -162,7 +165,10 @@  ds_put_format_valist(struct ds *ds, const char *format, va_list args_)
 
         va_copy(args, args_);
         available = ds->allocated - ds->length + 1;
-        needed = vsnprintf(&ds->string[ds->length], available, format, args);
+        needed = vsnprintf(ds->string
+                           ? &ds->string[ds->length]
+                           : NULL,
+                           available, format, args);
         va_end(args);
 
         ovs_assert(needed < available);
diff --git a/lib/meta-flow.c b/lib/meta-flow.c
index 92bfd350a92b..2e64fed5f8f2 100644
--- a/lib/meta-flow.c
+++ b/lib/meta-flow.c
@@ -3448,7 +3448,9 @@  mf_get_vl_mff(const struct mf_field *mff,
               const struct vl_mff_map *vl_mff_map)
 {
     if (mff && mff->variable_len && vl_mff_map) {
-        return &mf_get_vl_mff__(mff->id, vl_mff_map)->mf;
+        struct vl_mf_field *vl_mff = mf_get_vl_mff__(mff->id, vl_mff_map);
+
+        return vl_mff ? &vl_mff->mf : NULL;
     }
 
     return NULL;
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 040456ad532d..cf7240873dde 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -3202,14 +3202,14 @@  set_field_split_str(char *arg, char **key, char **value, char **delim)
 
     *value = arg;
     value_end = strstr(arg, "->");
+    if (!value_end) {
+        return xasprintf("%s: missing `->'", arg);
+    }
+
     *key = value_end + strlen("->");
     if (delim) {
         *delim = value_end;
     }
-
-    if (!value_end) {
-        return xasprintf("%s: missing `->'", arg);
-    }
     if (strlen(value_end) <= strlen("->")) {
         return xasprintf("%s: missing field name following `->'", arg);
     }
diff --git a/lib/ofpbuf.c b/lib/ofpbuf.c
index 271105bdea17..0330681b2518 100644
--- a/lib/ofpbuf.c
+++ b/lib/ofpbuf.c
@@ -436,6 +436,10 @@  ofpbuf_reserve(struct ofpbuf *b, size_t size)
 void *
 ofpbuf_push_uninit(struct ofpbuf *b, size_t size)
 {
+    if (!size) {
+        return b->data;
+    }
+
     ofpbuf_prealloc_headroom(b, size);
     b->data = (char*)b->data - size;
     b->size += size;
diff --git a/lib/ovsdb-data.c b/lib/ovsdb-data.c
index 6b1c20ff85a0..306b8d0d946a 100644
--- a/lib/ovsdb-data.c
+++ b/lib/ovsdb-data.c
@@ -1957,6 +1957,19 @@  ovsdb_datum_add_unsafe(struct ovsdb_datum *datum,
     }
 }
 
+void
+ovsdb_datum_add_unsafe_index(struct ovsdb_datum *dst,
+                             const struct ovsdb_datum *src,
+                             size_t idx,
+                             const struct ovsdb_type *type)
+{
+    const union ovsdb_atom *key = &src->keys[idx];
+    const union ovsdb_atom *value = type->value.type != OVSDB_TYPE_VOID
+                                    ? &src->values[idx]
+                                    : NULL;
+    ovsdb_datum_add_unsafe(dst, key, value, type, NULL);
+}
+
 /* Adds 'n' atoms starting from index 'start_idx' from 'src' to the end of
  * 'dst'.  'dst' should have enough memory allocated to hold the additional
  * 'n' atoms.  Atoms are not cloned, i.e. 'dst' will reference the same data.
@@ -2165,12 +2178,10 @@  ovsdb_datum_added_removed(struct ovsdb_datum *added,
         int c = ovsdb_atom_compare_3way(&old->keys[oi], &new->keys[ni],
                                         type->key.type);
         if (c < 0) {
-            ovsdb_datum_add_unsafe(removed, &old->keys[oi], &old->values[oi],
-                                   type, NULL);
+            ovsdb_datum_add_unsafe_index(removed, old, oi, type);
             oi++;
         } else if (c > 0) {
-            ovsdb_datum_add_unsafe(added, &new->keys[ni], &new->values[ni],
-                                   type, NULL);
+            ovsdb_datum_add_unsafe_index(added, new, ni, type);
             ni++;
         } else {
             if (type->value.type != OVSDB_TYPE_VOID &&
@@ -2186,13 +2197,11 @@  ovsdb_datum_added_removed(struct ovsdb_datum *added,
     }
 
     for (; oi < old->n; oi++) {
-        ovsdb_datum_add_unsafe(removed, &old->keys[oi], &old->values[oi],
-                               type, NULL);
+        ovsdb_datum_add_unsafe_index(removed, old, oi, type);
     }
 
     for (; ni < new->n; ni++) {
-        ovsdb_datum_add_unsafe(added, &new->keys[ni], &new->values[ni],
-                               type, NULL);
+        ovsdb_datum_add_unsafe_index(added, new, ni, type);
     }
 }
 
@@ -2228,12 +2237,10 @@  ovsdb_datum_diff(struct ovsdb_datum *diff,
         int c = ovsdb_atom_compare_3way(&old->keys[oi], &new->keys[ni],
                                         type->key.type);
         if (c < 0) {
-            ovsdb_datum_add_unsafe(diff, &old->keys[oi], &old->values[oi],
-                                   type, NULL);
+            ovsdb_datum_add_unsafe_index(diff, old, oi, type);
             oi++;
         } else if (c > 0) {
-            ovsdb_datum_add_unsafe(diff, &new->keys[ni], &new->values[ni],
-                                   type, NULL);
+            ovsdb_datum_add_unsafe_index(diff, new, ni, type);
             ni++;
         } else {
             if (type->value.type != OVSDB_TYPE_VOID &&
@@ -2247,13 +2254,11 @@  ovsdb_datum_diff(struct ovsdb_datum *diff,
     }
 
     for (; oi < old->n; oi++) {
-        ovsdb_datum_add_unsafe(diff, &old->keys[oi], &old->values[oi],
-                               type, NULL);
+        ovsdb_datum_add_unsafe_index(diff, old, oi, type);
     }
 
     for (; ni < new->n; ni++) {
-        ovsdb_datum_add_unsafe(diff, &new->keys[ni], &new->values[ni],
-                               type, NULL);
+        ovsdb_datum_add_unsafe_index(diff, new, ni, type);
     }
 }
 
diff --git a/lib/ovsdb-data.h b/lib/ovsdb-data.h
index 47115a7b85b7..31c7c27e6b93 100644
--- a/lib/ovsdb-data.h
+++ b/lib/ovsdb-data.h
@@ -280,6 +280,10 @@  void ovsdb_datum_add_unsafe(struct ovsdb_datum *,
                             const union ovsdb_atom *value,
                             const struct ovsdb_type *,
                             const union ovsdb_atom *range_end_atom);
+void ovsdb_datum_add_unsafe_index(struct ovsdb_datum *dst,
+                                  const struct ovsdb_datum *src,
+                                  size_t idx,
+                                  const struct ovsdb_type *type);
 
 /* Transactions with named-uuid row names. */
 struct json *ovsdb_datum_to_json_with_row_names(const struct ovsdb_datum *,
diff --git a/lib/sset.c b/lib/sset.c
index b2e3f43ec91b..b18b948fa4e7 100644
--- a/lib/sset.c
+++ b/lib/sset.c
@@ -312,7 +312,9 @@  sset_at_position(const struct sset *set, struct sset_position *pos)
     struct hmap_node *hmap_node;
 
     hmap_node = hmap_at_position(&set->map, &pos->pos);
-    return SSET_NODE_FROM_HMAP_NODE(hmap_node);
+    return hmap_node
+           ? SSET_NODE_FROM_HMAP_NODE(hmap_node)
+           : NULL;
 }
 
 /* Replaces 'a' by the intersection of 'a' and 'b'.  That is, removes from 'a'
diff --git a/lib/tnl-ports.c b/lib/tnl-ports.c
index 58269d3b1631..dd3320ccbb96 100644
--- a/lib/tnl-ports.c
+++ b/lib/tnl-ports.c
@@ -71,7 +71,7 @@  tnl_port_cast(const struct cls_rule *cr)
 {
     BUILD_ASSERT_DECL(offsetof(struct tnl_port_in, cr) == 0);
 
-    return CONTAINER_OF(cr, struct tnl_port_in, cr);
+    return cr ? CONTAINER_OF(cr, struct tnl_port_in, cr) : NULL;
 }
 
 static void
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 56aeac720935..c3ffbbb13b5b 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -8902,7 +8902,7 @@  eviction_group_hash_rule(struct rule *rule)
     hash = table->eviction_group_id_basis;
     miniflow_expand(rule->cr.match.flow, &flow);
     for (sf = table->eviction_fields;
-         sf < &table->eviction_fields[table->n_eviction_fields];
+         sf && sf < &table->eviction_fields[table->n_eviction_fields];
          sf++)
     {
         if (mf_are_prereqs_ok(sf->field, &flow, NULL)) {