diff mbox series

[ovs-dev,v5,2/6] treewide: Avoid offsetting NULL pointers.

Message ID 20220405124245.16152.81993.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
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Dumitru Ceara April 5, 2022, 12:42 p.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>
---
v5:
- Rebase.
v4:
- Addressed Ilya's comments.
---
 include/openvswitch/ofp-actions.h |    4 +++-
 include/openvswitch/ofpbuf.h      |   17 ++++++++++++-----
 lib/dynamic-string.c              |    8 ++++++--
 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, 62 insertions(+), 32 deletions(-)

Comments

Aaron Conole April 5, 2022, 2:41 p.m. UTC | #1 
Dumitru Ceara <dceara@redhat.com> writes:

> 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>
> ---
> v5:
> - Rebase.
> v4:
> - Addressed Ilya's comments.
> ---

Glad to see that the undefined behavior got removed, BUT this
can introduce some different undefined behavior - places where we
have a calls to ofpbuf_at_...() always assume a valid pointer is
returned.

I think it makes sense to abort if b->data is NULL in these cases.
Maybe something like:

  ovs_abort(0, "invalid buffer data pointer");

WDYT?

>  include/openvswitch/ofp-actions.h |    4 +++-
>  include/openvswitch/ofpbuf.h      |   17 ++++++++++++-----
>  lib/dynamic-string.c              |    8 ++++++--
>  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, 62 insertions(+), 32 deletions(-)
>
> diff --git a/include/openvswitch/ofp-actions.h b/include/openvswitch/ofp-actions.h
> index 41bcb55d2056..b7231c7bb334 100644
> --- a/include/openvswitch/ofp-actions.h
> +++ b/include/openvswitch/ofp-actions.h
> @@ -218,7 +218,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..7b6aba9dc29c 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..6940e1fd63bd 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,8 @@ 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->length],
> +                           available, format, args);
>          va_end(args);
>  
>          ovs_assert(needed < available);
> diff --git a/lib/meta-flow.c b/lib/meta-flow.c
> index e03cd8d0c5cd..c576ae6202a4 100644
> --- a/lib/meta-flow.c
> +++ b/lib/meta-flow.c
> @@ -3442,7 +3442,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 a0b70a89d780..4ada0f4a3c49 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -3204,14 +3204,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..61ad7679a6c5 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_from_index_unsafe(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_from_index_unsafe(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_from_index_unsafe(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_from_index_unsafe(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_from_index_unsafe(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_from_index_unsafe(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_from_index_unsafe(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_from_index_unsafe(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_from_index_unsafe(diff, new, ni, type);
>      }
>  }
>  
> diff --git a/lib/ovsdb-data.h b/lib/ovsdb-data.h
> index 47115a7b85b7..ba5d179a6509 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_from_index_unsafe(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 c3197e305fff..6fbaa9d60d85 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 f9fee3793992..050eafa6b8c3 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 2ed107800748..933f7de2dc56 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -8963,7 +8963,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)) {
Dumitru Ceara April 5, 2022, 3:23 p.m. UTC | #2 
On 4/5/22 16:41, Aaron Conole wrote:
> Dumitru Ceara <dceara@redhat.com> writes:
> 
>> 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>
>> ---
>> v5:
>> - Rebase.
>> v4:
>> - Addressed Ilya's comments.
>> ---
> 
> Glad to see that the undefined behavior got removed, BUT this
> can introduce some different undefined behavior - places where we
> have a calls to ofpbuf_at_...() always assume a valid pointer is
> returned.
> 

Thanks for the review!

> I think it makes sense to abort if b->data is NULL in these cases.
> Maybe something like:
> 
>   ovs_abort(0, "invalid buffer data pointer");
> 
> WDYT?
> 

Calling ovs_abort() directly from openvswitch/util.h will be a challenge
because it's an internal function and the openvswitch/util.h header is
public.  Worst case we just call ovs_assert() like we already do in
ofpbuf_at_assert().

But, just to make sure I understood properly, you'd like to assert that
b->data is not NULL only in ofpbuf_at() and ofpbuf_at_assert(), right?

Because the other ofpact_...() functions are also called in valid
scenarios on ofpbufs that have b->data = NULL.

>>  include/openvswitch/ofp-actions.h |    4 +++-
>>  include/openvswitch/ofpbuf.h      |   17 ++++++++++++-----
>>  lib/dynamic-string.c              |    8 ++++++--
>>  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, 62 insertions(+), 32 deletions(-)
>>
>> diff --git a/include/openvswitch/ofp-actions.h b/include/openvswitch/ofp-actions.h
>> index 41bcb55d2056..b7231c7bb334 100644
>> --- a/include/openvswitch/ofp-actions.h
>> +++ b/include/openvswitch/ofp-actions.h
>> @@ -218,7 +218,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..7b6aba9dc29c 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..6940e1fd63bd 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,8 @@ 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->length],
>> +                           available, format, args);
>>          va_end(args);
>>  
>>          ovs_assert(needed < available);
>> diff --git a/lib/meta-flow.c b/lib/meta-flow.c
>> index e03cd8d0c5cd..c576ae6202a4 100644
>> --- a/lib/meta-flow.c
>> +++ b/lib/meta-flow.c
>> @@ -3442,7 +3442,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 a0b70a89d780..4ada0f4a3c49 100644
>> --- a/lib/ofp-actions.c
>> +++ b/lib/ofp-actions.c
>> @@ -3204,14 +3204,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..61ad7679a6c5 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_from_index_unsafe(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_from_index_unsafe(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_from_index_unsafe(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_from_index_unsafe(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_from_index_unsafe(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_from_index_unsafe(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_from_index_unsafe(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_from_index_unsafe(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_from_index_unsafe(diff, new, ni, type);
>>      }
>>  }
>>  
>> diff --git a/lib/ovsdb-data.h b/lib/ovsdb-data.h
>> index 47115a7b85b7..ba5d179a6509 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_from_index_unsafe(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 c3197e305fff..6fbaa9d60d85 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 f9fee3793992..050eafa6b8c3 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 2ed107800748..933f7de2dc56 100644
>> --- a/ofproto/ofproto.c
>> +++ b/ofproto/ofproto.c
>> @@ -8963,7 +8963,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)) {
>
Aaron Conole April 5, 2022, 7:20 p.m. UTC | #3 
Dumitru Ceara <dceara@redhat.com> writes:

> On 4/5/22 16:41, Aaron Conole wrote:
>> Dumitru Ceara <dceara@redhat.com> writes:
>> 
>>> 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>
>>> ---
>>> v5:
>>> - Rebase.
>>> v4:
>>> - Addressed Ilya's comments.
>>> ---
>> 
>> Glad to see that the undefined behavior got removed, BUT this
>> can introduce some different undefined behavior - places where we
>> have a calls to ofpbuf_at_...() always assume a valid pointer is
>> returned.
>> 
>
> Thanks for the review!
>
>> I think it makes sense to abort if b->data is NULL in these cases.
>> Maybe something like:
>> 
>>   ovs_abort(0, "invalid buffer data pointer");
>> 
>> WDYT?
>> 
>
> Calling ovs_abort() directly from openvswitch/util.h will be a challenge
> because it's an internal function and the openvswitch/util.h header is
> public.  Worst case we just call ovs_assert() like we already do in
> ofpbuf_at_assert().

Maybe we can expose ovs_abort as well?

> But, just to make sure I understood properly, you'd like to assert that
> b->data is not NULL only in ofpbuf_at() and ofpbuf_at_assert(), right?

right - only for those places where we have the assumption that the
return must be !NULL

> Because the other ofpact_...() functions are also called in valid
> scenarios on ofpbufs that have b->data = NULL.
>
>>>  include/openvswitch/ofp-actions.h |    4 +++-
>>>  include/openvswitch/ofpbuf.h      |   17 ++++++++++++-----
>>>  lib/dynamic-string.c              |    8 ++++++--
>>>  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, 62 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/include/openvswitch/ofp-actions.h b/include/openvswitch/ofp-actions.h
>>> index 41bcb55d2056..b7231c7bb334 100644
>>> --- a/include/openvswitch/ofp-actions.h
>>> +++ b/include/openvswitch/ofp-actions.h
>>> @@ -218,7 +218,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..7b6aba9dc29c 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..6940e1fd63bd 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,8 @@ 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->length],
>>> +                           available, format, args);
>>>          va_end(args);
>>>  
>>>          ovs_assert(needed < available);
>>> diff --git a/lib/meta-flow.c b/lib/meta-flow.c
>>> index e03cd8d0c5cd..c576ae6202a4 100644
>>> --- a/lib/meta-flow.c
>>> +++ b/lib/meta-flow.c
>>> @@ -3442,7 +3442,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 a0b70a89d780..4ada0f4a3c49 100644
>>> --- a/lib/ofp-actions.c
>>> +++ b/lib/ofp-actions.c
>>> @@ -3204,14 +3204,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..61ad7679a6c5 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_from_index_unsafe(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_from_index_unsafe(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_from_index_unsafe(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_from_index_unsafe(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_from_index_unsafe(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_from_index_unsafe(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_from_index_unsafe(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_from_index_unsafe(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_from_index_unsafe(diff, new, ni, type);
>>>      }
>>>  }
>>>  
>>> diff --git a/lib/ovsdb-data.h b/lib/ovsdb-data.h
>>> index 47115a7b85b7..ba5d179a6509 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_from_index_unsafe(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 c3197e305fff..6fbaa9d60d85 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 f9fee3793992..050eafa6b8c3 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 2ed107800748..933f7de2dc56 100644
>>> --- a/ofproto/ofproto.c
>>> +++ b/ofproto/ofproto.c
>>> @@ -8963,7 +8963,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)) {
>>
Dumitru Ceara April 6, 2022, 9:10 a.m. UTC | #4 
On 4/5/22 21:20, Aaron Conole wrote:
> Dumitru Ceara <dceara@redhat.com> writes:
> 
>> On 4/5/22 16:41, Aaron Conole wrote:
>>> Dumitru Ceara <dceara@redhat.com> writes:
>>>
>>>> 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>
>>>> ---
>>>> v5:
>>>> - Rebase.
>>>> v4:
>>>> - Addressed Ilya's comments.
>>>> ---
>>>
>>> Glad to see that the undefined behavior got removed, BUT this
>>> can introduce some different undefined behavior - places where we
>>> have a calls to ofpbuf_at_...() always assume a valid pointer is
>>> returned.
>>>
>>
>> Thanks for the review!
>>
>>> I think it makes sense to abort if b->data is NULL in these cases.
>>> Maybe something like:
>>>
>>>   ovs_abort(0, "invalid buffer data pointer");
>>>
>>> WDYT?
>>>
>>
>> Calling ovs_abort() directly from openvswitch/util.h will be a challenge
>> because it's an internal function and the openvswitch/util.h header is
>> public.  Worst case we just call ovs_assert() like we already do in
>> ofpbuf_at_assert().
> 
> Maybe we can expose ovs_abort as well?
> 

We can, but should we then expose all of the following, for consistency?

OVS_NO_RETURN void ovs_abort(int err_no, const char *format, ...)
    OVS_PRINTF_FORMAT(2, 3);
OVS_NO_RETURN void ovs_abort_valist(int err_no, const char *format, va_list)
    OVS_PRINTF_FORMAT(2, 0);
OVS_NO_RETURN void ovs_fatal(int err_no, const char *format, ...)
    OVS_PRINTF_FORMAT(2, 3);
OVS_NO_RETURN void ovs_fatal_valist(int err_no, const char *format, va_list)
    OVS_PRINTF_FORMAT(2, 0);

>> But, just to make sure I understood properly, you'd like to assert that
>> b->data is not NULL only in ofpbuf_at() and ofpbuf_at_assert(), right?
> 
> right - only for those places where we have the assumption that the
> return must be !NULL
> 

Ok.

>> Because the other ofpact_...() functions are also called in valid
>> scenarios on ofpbufs that have b->data = NULL.
>>

[...]
Aaron Conole April 6, 2022, 2:53 p.m. UTC | #5 
Dumitru Ceara <dceara@redhat.com> writes:

> On 4/5/22 21:20, Aaron Conole wrote:
>> Dumitru Ceara <dceara@redhat.com> writes:
>> 
>>> On 4/5/22 16:41, Aaron Conole wrote:
>>>> Dumitru Ceara <dceara@redhat.com> writes:
>>>>
>>>>> 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>
>>>>> ---
>>>>> v5:
>>>>> - Rebase.
>>>>> v4:
>>>>> - Addressed Ilya's comments.
>>>>> ---
>>>>
>>>> Glad to see that the undefined behavior got removed, BUT this
>>>> can introduce some different undefined behavior - places where we
>>>> have a calls to ofpbuf_at_...() always assume a valid pointer is
>>>> returned.
>>>>
>>>
>>> Thanks for the review!
>>>
>>>> I think it makes sense to abort if b->data is NULL in these cases.
>>>> Maybe something like:
>>>>
>>>>   ovs_abort(0, "invalid buffer data pointer");
>>>>
>>>> WDYT?
>>>>
>>>
>>> Calling ovs_abort() directly from openvswitch/util.h will be a challenge
>>> because it's an internal function and the openvswitch/util.h header is
>>> public.  Worst case we just call ovs_assert() like we already do in
>>> ofpbuf_at_assert().
>> 
>> Maybe we can expose ovs_abort as well?
>> 
>
> We can, but should we then expose all of the following, for consistency?
>
> OVS_NO_RETURN void ovs_abort(int err_no, const char *format, ...)
>     OVS_PRINTF_FORMAT(2, 3);
> OVS_NO_RETURN void ovs_abort_valist(int err_no, const char *format, va_list)
>     OVS_PRINTF_FORMAT(2, 0);
> OVS_NO_RETURN void ovs_fatal(int err_no, const char *format, ...)
>     OVS_PRINTF_FORMAT(2, 3);
> OVS_NO_RETURN void ovs_fatal_valist(int err_no, const char *format, va_list)
>     OVS_PRINTF_FORMAT(2, 0);

I think it makes sense.  Maybe Ilya/Ian disagrees

>>> But, just to make sure I understood properly, you'd like to assert that
>>> b->data is not NULL only in ofpbuf_at() and ofpbuf_at_assert(), right?
>> 
>> right - only for those places where we have the assumption that the
>> return must be !NULL
>> 
>
> Ok.
>
>>> Because the other ofpact_...() functions are also called in valid
>>> scenarios on ofpbufs that have b->data = NULL.
>>>
>
> [...]
Ilya Maximets April 6, 2022, 3 p.m. UTC | #6 
On 4/6/22 16:53, Aaron Conole wrote:
> Dumitru Ceara <dceara@redhat.com> writes:
> 
>> On 4/5/22 21:20, Aaron Conole wrote:
>>> Dumitru Ceara <dceara@redhat.com> writes:
>>>
>>>> On 4/5/22 16:41, Aaron Conole wrote:
>>>>> Dumitru Ceara <dceara@redhat.com> writes:
>>>>>
>>>>>> 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>
>>>>>> ---
>>>>>> v5:
>>>>>> - Rebase.
>>>>>> v4:
>>>>>> - Addressed Ilya's comments.
>>>>>> ---
>>>>>
>>>>> Glad to see that the undefined behavior got removed, BUT this
>>>>> can introduce some different undefined behavior - places where we
>>>>> have a calls to ofpbuf_at_...() always assume a valid pointer is
>>>>> returned.
>>>>>
>>>>
>>>> Thanks for the review!
>>>>
>>>>> I think it makes sense to abort if b->data is NULL in these cases.
>>>>> Maybe something like:
>>>>>
>>>>>   ovs_abort(0, "invalid buffer data pointer");
>>>>>
>>>>> WDYT?
>>>>>
>>>>
>>>> Calling ovs_abort() directly from openvswitch/util.h will be a challenge
>>>> because it's an internal function and the openvswitch/util.h header is
>>>> public.  Worst case we just call ovs_assert() like we already do in
>>>> ofpbuf_at_assert().
>>>
>>> Maybe we can expose ovs_abort as well?
>>>
>>
>> We can, but should we then expose all of the following, for consistency?
>>
>> OVS_NO_RETURN void ovs_abort(int err_no, const char *format, ...)
>>     OVS_PRINTF_FORMAT(2, 3);
>> OVS_NO_RETURN void ovs_abort_valist(int err_no, const char *format, va_list)
>>     OVS_PRINTF_FORMAT(2, 0);
>> OVS_NO_RETURN void ovs_fatal(int err_no, const char *format, ...)
>>     OVS_PRINTF_FORMAT(2, 3);
>> OVS_NO_RETURN void ovs_fatal_valist(int err_no, const char *format, va_list)
>>     OVS_PRINTF_FORMAT(2, 0);
> 
> I think it makes sense.  Maybe Ilya/Ian disagrees


Hmm.  Can we just use ovs_assert() instead of ovs_abort() ?
This one is defined in the openvswitch/util.h.

> 
>>>> But, just to make sure I understood properly, you'd like to assert that
>>>> b->data is not NULL only in ofpbuf_at() and ofpbuf_at_assert(), right?
>>>
>>> right - only for those places where we have the assumption that the
>>> return must be !NULL
>>>
>>
>> Ok.
>>
>>>> Because the other ofpact_...() functions are also called in valid
>>>> scenarios on ofpbufs that have b->data = NULL.
>>>>
>>
>> [...]
>
Aaron Conole April 6, 2022, 4:31 p.m. UTC | #7 
Ilya Maximets <i.maximets@ovn.org> writes:

> On 4/6/22 16:53, Aaron Conole wrote:
>> Dumitru Ceara <dceara@redhat.com> writes:
>> 
>>> On 4/5/22 21:20, Aaron Conole wrote:
>>>> Dumitru Ceara <dceara@redhat.com> writes:
>>>>
>>>>> On 4/5/22 16:41, Aaron Conole wrote:
>>>>>> Dumitru Ceara <dceara@redhat.com> writes:
>>>>>>
>>>>>>> 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>
>>>>>>> ---
>>>>>>> v5:
>>>>>>> - Rebase.
>>>>>>> v4:
>>>>>>> - Addressed Ilya's comments.
>>>>>>> ---
>>>>>>
>>>>>> Glad to see that the undefined behavior got removed, BUT this
>>>>>> can introduce some different undefined behavior - places where we
>>>>>> have a calls to ofpbuf_at_...() always assume a valid pointer is
>>>>>> returned.
>>>>>>
>>>>>
>>>>> Thanks for the review!
>>>>>
>>>>>> I think it makes sense to abort if b->data is NULL in these cases.
>>>>>> Maybe something like:
>>>>>>
>>>>>>   ovs_abort(0, "invalid buffer data pointer");
>>>>>>
>>>>>> WDYT?
>>>>>>
>>>>>
>>>>> Calling ovs_abort() directly from openvswitch/util.h will be a challenge
>>>>> because it's an internal function and the openvswitch/util.h header is
>>>>> public.  Worst case we just call ovs_assert() like we already do in
>>>>> ofpbuf_at_assert().
>>>>
>>>> Maybe we can expose ovs_abort as well?
>>>>
>>>
>>> We can, but should we then expose all of the following, for consistency?
>>>
>>> OVS_NO_RETURN void ovs_abort(int err_no, const char *format, ...)
>>>     OVS_PRINTF_FORMAT(2, 3);
>>> OVS_NO_RETURN void ovs_abort_valist(int err_no, const char *format, va_list)
>>>     OVS_PRINTF_FORMAT(2, 0);
>>> OVS_NO_RETURN void ovs_fatal(int err_no, const char *format, ...)
>>>     OVS_PRINTF_FORMAT(2, 3);
>>> OVS_NO_RETURN void ovs_fatal_valist(int err_no, const char *format, va_list)
>>>     OVS_PRINTF_FORMAT(2, 0);
>> 
>> I think it makes sense.  Maybe Ilya/Ian disagrees
>
>
> Hmm.  Can we just use ovs_assert() instead of ovs_abort() ?
> This one is defined in the openvswitch/util.h.

ovs_assert will only evaluate (CONDITION) but not take any action if
compiled with -DNDEBUG.  So we will have a SIGSEGV instead of abort() in
production compiled code.

I guess it's 6-of-one, 1/2 dozen of the other - either is fine by me.

>> 
>>>>> But, just to make sure I understood properly, you'd like to assert that
>>>>> b->data is not NULL only in ofpbuf_at() and ofpbuf_at_assert(), right?
>>>>
>>>> right - only for those places where we have the assumption that the
>>>> return must be !NULL
>>>>
>>>
>>> Ok.
>>>
>>>>> Because the other ofpact_...() functions are also called in valid
>>>>> scenarios on ofpbufs that have b->data = NULL.
>>>>>
>>>
>>> [...]
>>
Ilya Maximets April 6, 2022, 4:36 p.m. UTC | #8 
On 4/6/22 18:31, Aaron Conole wrote:
> Ilya Maximets <i.maximets@ovn.org> writes:
> 
>> On 4/6/22 16:53, Aaron Conole wrote:
>>> Dumitru Ceara <dceara@redhat.com> writes:
>>>
>>>> On 4/5/22 21:20, Aaron Conole wrote:
>>>>> Dumitru Ceara <dceara@redhat.com> writes:
>>>>>
>>>>>> On 4/5/22 16:41, Aaron Conole wrote:
>>>>>>> Dumitru Ceara <dceara@redhat.com> writes:
>>>>>>>
>>>>>>>> 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>
>>>>>>>> ---
>>>>>>>> v5:
>>>>>>>> - Rebase.
>>>>>>>> v4:
>>>>>>>> - Addressed Ilya's comments.
>>>>>>>> ---
>>>>>>>
>>>>>>> Glad to see that the undefined behavior got removed, BUT this
>>>>>>> can introduce some different undefined behavior - places where we
>>>>>>> have a calls to ofpbuf_at_...() always assume a valid pointer is
>>>>>>> returned.
>>>>>>>
>>>>>>
>>>>>> Thanks for the review!
>>>>>>
>>>>>>> I think it makes sense to abort if b->data is NULL in these cases.
>>>>>>> Maybe something like:
>>>>>>>
>>>>>>>   ovs_abort(0, "invalid buffer data pointer");
>>>>>>>
>>>>>>> WDYT?
>>>>>>>
>>>>>>
>>>>>> Calling ovs_abort() directly from openvswitch/util.h will be a challenge
>>>>>> because it's an internal function and the openvswitch/util.h header is
>>>>>> public.  Worst case we just call ovs_assert() like we already do in
>>>>>> ofpbuf_at_assert().
>>>>>
>>>>> Maybe we can expose ovs_abort as well?
>>>>>
>>>>
>>>> We can, but should we then expose all of the following, for consistency?
>>>>
>>>> OVS_NO_RETURN void ovs_abort(int err_no, const char *format, ...)
>>>>     OVS_PRINTF_FORMAT(2, 3);
>>>> OVS_NO_RETURN void ovs_abort_valist(int err_no, const char *format, va_list)
>>>>     OVS_PRINTF_FORMAT(2, 0);
>>>> OVS_NO_RETURN void ovs_fatal(int err_no, const char *format, ...)
>>>>     OVS_PRINTF_FORMAT(2, 3);
>>>> OVS_NO_RETURN void ovs_fatal_valist(int err_no, const char *format, va_list)
>>>>     OVS_PRINTF_FORMAT(2, 0);
>>>
>>> I think it makes sense.  Maybe Ilya/Ian disagrees
>>
>>
>> Hmm.  Can we just use ovs_assert() instead of ovs_abort() ?
>> This one is defined in the openvswitch/util.h.
> 
> ovs_assert will only evaluate (CONDITION) but not take any action if
> compiled with -DNDEBUG.  So we will have a SIGSEGV instead of abort() in
> production compiled code.

But our unit tests are perfect and will catch any such condition, right? :)

> 
> I guess it's 6-of-one, 1/2 dozen of the other - either is fine by me.
> 
>>>
>>>>>> But, just to make sure I understood properly, you'd like to assert that
>>>>>> b->data is not NULL only in ofpbuf_at() and ofpbuf_at_assert(), right?
>>>>>
>>>>> right - only for those places where we have the assumption that the
>>>>> return must be !NULL
>>>>>
>>>>
>>>> Ok.
>>>>
>>>>>> Because the other ofpact_...() functions are also called in valid
>>>>>> scenarios on ofpbufs that have b->data = NULL.
>>>>>>
>>>>
>>>> [...]
>>>
>
Dumitru Ceara April 11, 2022, 11:08 a.m. UTC | #9 
On 4/6/22 18:36, Ilya Maximets wrote:
> On 4/6/22 18:31, Aaron Conole wrote:
>> Ilya Maximets <i.maximets@ovn.org> writes:
>>
>>> On 4/6/22 16:53, Aaron Conole wrote:
>>>> Dumitru Ceara <dceara@redhat.com> writes:
>>>>
>>>>> On 4/5/22 21:20, Aaron Conole wrote:
>>>>>> Dumitru Ceara <dceara@redhat.com> writes:
>>>>>>
>>>>>>> On 4/5/22 16:41, Aaron Conole wrote:
>>>>>>>> Dumitru Ceara <dceara@redhat.com> writes:
>>>>>>>>
>>>>>>>>> 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>
>>>>>>>>> ---
>>>>>>>>> v5:
>>>>>>>>> - Rebase.
>>>>>>>>> v4:
>>>>>>>>> - Addressed Ilya's comments.
>>>>>>>>> ---
>>>>>>>>
>>>>>>>> Glad to see that the undefined behavior got removed, BUT this
>>>>>>>> can introduce some different undefined behavior - places where we
>>>>>>>> have a calls to ofpbuf_at_...() always assume a valid pointer is
>>>>>>>> returned.
>>>>>>>>
>>>>>>>
>>>>>>> Thanks for the review!
>>>>>>>
>>>>>>>> I think it makes sense to abort if b->data is NULL in these cases.
>>>>>>>> Maybe something like:
>>>>>>>>
>>>>>>>>   ovs_abort(0, "invalid buffer data pointer");
>>>>>>>>
>>>>>>>> WDYT?
>>>>>>>>
>>>>>>>
>>>>>>> Calling ovs_abort() directly from openvswitch/util.h will be a challenge
>>>>>>> because it's an internal function and the openvswitch/util.h header is
>>>>>>> public.  Worst case we just call ovs_assert() like we already do in
>>>>>>> ofpbuf_at_assert().
>>>>>>
>>>>>> Maybe we can expose ovs_abort as well?
>>>>>>
>>>>>
>>>>> We can, but should we then expose all of the following, for consistency?
>>>>>
>>>>> OVS_NO_RETURN void ovs_abort(int err_no, const char *format, ...)
>>>>>     OVS_PRINTF_FORMAT(2, 3);
>>>>> OVS_NO_RETURN void ovs_abort_valist(int err_no, const char *format, va_list)
>>>>>     OVS_PRINTF_FORMAT(2, 0);
>>>>> OVS_NO_RETURN void ovs_fatal(int err_no, const char *format, ...)
>>>>>     OVS_PRINTF_FORMAT(2, 3);
>>>>> OVS_NO_RETURN void ovs_fatal_valist(int err_no, const char *format, va_list)
>>>>>     OVS_PRINTF_FORMAT(2, 0);
>>>>
>>>> I think it makes sense.  Maybe Ilya/Ian disagrees
>>>
>>>
>>> Hmm.  Can we just use ovs_assert() instead of ovs_abort() ?
>>> This one is defined in the openvswitch/util.h.
>>
>> ovs_assert will only evaluate (CONDITION) but not take any action if
>> compiled with -DNDEBUG.  So we will have a SIGSEGV instead of abort() in
>> production compiled code.
> 
> But our unit tests are perfect and will catch any such condition, right? :)
> 

I went with ovs_assert() in v6.  I'll be posting it shortly, thanks!

>>
>> I guess it's 6-of-one, 1/2 dozen of the other - either is fine by me.
>>
>>>>
>>>>>>> But, just to make sure I understood properly, you'd like to assert that
>>>>>>> b->data is not NULL only in ofpbuf_at() and ofpbuf_at_assert(), right?
>>>>>>
>>>>>> right - only for those places where we have the assumption that the
>>>>>> return must be !NULL
>>>>>>
>>>>>
>>>>> Ok.
>>>>>
>>>>>>> Because the other ofpact_...() functions are also called in valid
>>>>>>> scenarios on ofpbufs that have b->data = NULL.
>>>>>>>
>>>>>
>>>>> [...]
>>>>
>>
>
diff mbox series

Patch

diff --git a/include/openvswitch/ofp-actions.h b/include/openvswitch/ofp-actions.h
index 41bcb55d2056..b7231c7bb334 100644
--- a/include/openvswitch/ofp-actions.h
+++ b/include/openvswitch/ofp-actions.h
@@ -218,7 +218,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..7b6aba9dc29c 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..6940e1fd63bd 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,8 @@  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->length],
+                           available, format, args);
         va_end(args);
 
         ovs_assert(needed < available);
diff --git a/lib/meta-flow.c b/lib/meta-flow.c
index e03cd8d0c5cd..c576ae6202a4 100644
--- a/lib/meta-flow.c
+++ b/lib/meta-flow.c
@@ -3442,7 +3442,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 a0b70a89d780..4ada0f4a3c49 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -3204,14 +3204,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..61ad7679a6c5 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_from_index_unsafe(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_from_index_unsafe(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_from_index_unsafe(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_from_index_unsafe(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_from_index_unsafe(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_from_index_unsafe(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_from_index_unsafe(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_from_index_unsafe(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_from_index_unsafe(diff, new, ni, type);
     }
 }
 
diff --git a/lib/ovsdb-data.h b/lib/ovsdb-data.h
index 47115a7b85b7..ba5d179a6509 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_from_index_unsafe(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 c3197e305fff..6fbaa9d60d85 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 f9fee3793992..050eafa6b8c3 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 2ed107800748..933f7de2dc56 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -8963,7 +8963,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)) {