Dumitru Ceara <dce...@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 <dce...@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)) { _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev