| Message ID | 20220124141859.11777.68499.stgit@dceara.remote.csb |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Fix UndefinedBehaviorSanitizer reported issues and enable it in CI. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 1/24/22 15:19, Dumitru Ceara wrote: > This is undefined behavior and was reported by UB Sanitizer: > lib/meta-flow.c:3445:16: runtime error: member access within null pointer of type 'struct vl_mf_field' > #0 0x6aad0f in mf_get_vl_mff lib/meta-flow.c:3445 > #1 0x6d96d7 in mf_from_oxm_header lib/nx-match.c:260 > #2 0x6d9e2e in nx_pull_header__ lib/nx-match.c:341 > #3 0x6daafa in nx_pull_header lib/nx-match.c:488 > #4 0x6abcb6 in mf_vl_mff_nx_pull_header lib/meta-flow.c:3605 > #5 0x73b9be in decode_NXAST_RAW_REG_MOVE lib/ofp-actions.c:2652 > #6 0x764ccd in ofpact_decode lib/ofp-actions.inc2:4681 > [...] > lib/sset.c:315:12: runtime error: applying zero offset to null pointer > #0 0xcc2e6a in sset_at_position /root/ovs/lib/sset.c:315:12 > #1 0x5734b3 in port_dump_next /root/ovs/ofproto/ofproto-dpif.c:4083:20 > [...] > lib/ovsdb-data.c:2194:56: runtime error: applying zero offset to null pointer > #0 0x5e9530 in ovsdb_datum_added_removed /root/ovs/lib/ovsdb-data.c:2194:56 > #1 0x4d6258 in update_row_ref_count /root/ovs/ovsdb/transaction.c:335:17 > #2 0x4c360b in for_each_txn_row /root/ovs/ovsdb/transaction.c:1572:33 > [...] > lib/ofpbuf.c:440:30: runtime error: applying zero offset to null pointer > #0 0x75066d in ofpbuf_push_uninit lib/ofpbuf.c:440 > #1 0x46ac8a in ovnacts_parse lib/actions.c:4190 > #2 0x46ad91 in ovnacts_parse_string lib/actions.c:4208 > #3 0x4106d1 in test_parse_actions tests/test-ovn.c:1324 > [...] > lib/ofp-actions.c:3205:22: runtime error: applying non-zero offset 2 to null pointer > #0 0x6e1641 in set_field_split_str /root/ovs/lib/ofp-actions.c:3205:22 > [...] > lib/tnl-ports.c:74:12: runtime error: applying zero offset to null pointer > #0 0xceffe7 in tnl_port_cast /root/ovs/lib/tnl-ports.c:74:12 > #1 0xcf14c3 in map_insert /root/ovs/lib/tnl-ports.c:116:13 > [...] > ofproto/ofproto.c:8905:16: runtime error: applying zero offset to null pointer > #0 0x556795 in eviction_group_hash_rule /root/ovs/ofproto/ofproto.c:8905:16 > #1 0x503f8d in eviction_group_add_rule /root/ovs/ofproto/ofproto.c:9022:42 > [...] > > Also, it's valid to have an empty ofpact list and we should be able to > try to iterate through it. > > UB Sanitizer report: > include/openvswitch/ofp-actions.h:222:12: runtime error: applying zero offset to null pointer > #0 0x665d69 in ofpact_end /root/ovs/./include/openvswitch/ofp-actions.h:222:12 > #1 0x66b2cf in ofpacts_put_openflow_actions /root/ovs/lib/ofp-actions.c:8861:5 > #2 0x6ffdd1 in ofputil_encode_flow_mod /root/ovs/lib/ofp-flow.c:447:9 > [...] > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > include/openvswitch/ofp-actions.h | 4 +++- > include/openvswitch/ofpbuf.h | 17 ++++++++++++----- > lib/dynamic-string.c | 10 ++++++++-- > lib/meta-flow.c | 4 +++- > lib/ofp-actions.c | 8 ++++---- > lib/ofpbuf.c | 4 ++++ > lib/ovsdb-data.c | 37 +++++++++++++++++++++---------------- > lib/ovsdb-data.h | 4 ++++ > lib/sset.c | 4 +++- > lib/tnl-ports.c | 2 +- > ofproto/ofproto.c | 2 +- > 11 files changed, 64 insertions(+), 32 deletions(-) > > diff --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..a9ff8a56e81c 100644 > --- a/include/openvswitch/ofpbuf.h > +++ b/include/openvswitch/ofpbuf.h > @@ -179,7 +179,9 @@ static inline void ofpbuf_delete(struct ofpbuf *b) > static inline void *ofpbuf_at(const struct ofpbuf *b, size_t offset, > size_t size) > { > - return offset + size <= b->size ? (char *) b->data + offset : NULL; > + return offset + size <= b->size && b->data It's better to parenthesize the condition. > + ? (char *) b->data + offset > + : NULL; > } > > /* Returns a pointer to byte 'offset' in 'b', which must contain at least > @@ -188,20 +190,20 @@ static inline void *ofpbuf_at_assert(const struct ofpbuf *b, size_t offset, > size_t size) > { > ovs_assert(offset + size <= b->size); > - return ((char *) b->data) + offset; > + return b->data ? (char *) b->data + offset : NULL; > } > > /* Returns a pointer to byte following the last byte of data in use in 'b'. */ > static inline void *ofpbuf_tail(const struct ofpbuf *b) > { > - return (char *) b->data + b->size; > + return b->data ? (char *) b->data + b->size : NULL; > } > > /* Returns a pointer to byte following the last byte allocated for use (but > * not necessarily in use) in 'b'. */ > static inline void *ofpbuf_end(const struct ofpbuf *b) > { > - return (char *) b->base + b->allocated; > + return b->base ? (char *) b->base + b->allocated : NULL; > } > > /* Returns the number of bytes of headroom in 'b', that is, the number of bytes > @@ -249,6 +251,11 @@ static inline void *ofpbuf_pull(struct ofpbuf *b, size_t size) > { > ovs_assert(b->size >= size); > void *data = b->data; > + > + if (!size) { > + return data; > + } > + > b->data = (char*)b->data + size; > b->size = b->size - size; > return data; > @@ -270,7 +277,7 @@ static inline struct ofpbuf *ofpbuf_from_list(const struct ovs_list *list) > static inline bool ofpbuf_equal(const struct ofpbuf *a, const struct ofpbuf *b) > { > return a->size == b->size && > - memcmp(a->data, b->data, a->size) == 0; > + (a->size == 0 || memcmp(a->data, b->data, a->size) == 0); > } > > static inline bool ofpbuf_oversized(const struct ofpbuf *ofpacts) > diff --git a/lib/dynamic-string.c b/lib/dynamic-string.c > index fd0127ed1740..9b9a8f50e562 100644 > --- a/lib/dynamic-string.c > +++ b/lib/dynamic-string.c > @@ -152,7 +152,10 @@ ds_put_format_valist(struct ds *ds, const char *format, va_list args_) > > va_copy(args, args_); > available = ds->string ? ds->allocated - ds->length + 1 : 0; > - needed = vsnprintf(&ds->string[ds->length], available, format, args); > + needed = vsnprintf(ds->string > + ? &ds->string[ds->length] > + : NULL, > + available, format, args); > va_end(args); > > if (needed < available) { > @@ -162,7 +165,10 @@ ds_put_format_valist(struct ds *ds, const char *format, va_list args_) > > va_copy(args, args_); > available = ds->allocated - ds->length + 1; > - needed = vsnprintf(&ds->string[ds->length], available, format, args); > + needed = vsnprintf(ds->string This pointer can't be NULL here. We already called ds_reserve(). > + ? &ds->string[ds->length] > + : NULL, > + available, format, args); > va_end(args); > > ovs_assert(needed < available); > diff --git a/lib/meta-flow.c b/lib/meta-flow.c > index 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 006837c2e1f5..b24b46d2196b 100644 > --- a/lib/ofp-actions.c > +++ b/lib/ofp-actions.c > @@ -3202,14 +3202,14 @@ set_field_split_str(char *arg, char **key, char **value, char **delim) > > *value = arg; > value_end = strstr(arg, "->"); > + if (!value_end) { > + return xasprintf("%s: missing `->'", arg); > + } > + > *key = value_end + strlen("->"); > if (delim) { > *delim = value_end; > } > - > - if (!value_end) { > - return xasprintf("%s: missing `->'", arg); > - } > if (strlen(value_end) <= strlen("->")) { > return xasprintf("%s: missing field name following `->'", arg); > } > diff --git a/lib/ofpbuf.c b/lib/ofpbuf.c > index 271105bdea17..0330681b2518 100644 > --- a/lib/ofpbuf.c > +++ b/lib/ofpbuf.c > @@ -436,6 +436,10 @@ ofpbuf_reserve(struct ofpbuf *b, size_t size) > void * > ofpbuf_push_uninit(struct ofpbuf *b, size_t size) > { > + if (!size) { > + return b->data; > + } > + > ofpbuf_prealloc_headroom(b, size); > b->data = (char*)b->data - size; > b->size += size; > diff --git a/lib/ovsdb-data.c b/lib/ovsdb-data.c > index 6b1c20ff85a0..306b8d0d946a 100644 > --- a/lib/ovsdb-data.c > +++ b/lib/ovsdb-data.c > @@ -1957,6 +1957,19 @@ ovsdb_datum_add_unsafe(struct ovsdb_datum *datum, > } > } > > +void > +ovsdb_datum_add_unsafe_index(struct ovsdb_datum *dst, Maybe ovsdb_datum_add_from_index_unsafe() or something like that? > + const struct ovsdb_datum *src, > + size_t idx, > + const struct ovsdb_type *type) > +{ > + const union ovsdb_atom *key = &src->keys[idx]; > + const union ovsdb_atom *value = type->value.type != OVSDB_TYPE_VOID > + ? &src->values[idx] > + : NULL; > + ovsdb_datum_add_unsafe(dst, key, value, type, NULL); > +} > + > /* Adds 'n' atoms starting from index 'start_idx' from 'src' to the end of > * 'dst'. 'dst' should have enough memory allocated to hold the additional > * 'n' atoms. Atoms are not cloned, i.e. 'dst' will reference the same data. > @@ -2165,12 +2178,10 @@ ovsdb_datum_added_removed(struct ovsdb_datum *added, > int c = ovsdb_atom_compare_3way(&old->keys[oi], &new->keys[ni], > type->key.type); > if (c < 0) { > - ovsdb_datum_add_unsafe(removed, &old->keys[oi], &old->values[oi], > - type, NULL); > + ovsdb_datum_add_unsafe_index(removed, old, oi, type); > oi++; > } else if (c > 0) { > - ovsdb_datum_add_unsafe(added, &new->keys[ni], &new->values[ni], > - type, NULL); > + ovsdb_datum_add_unsafe_index(added, new, ni, type); > ni++; > } else { > if (type->value.type != OVSDB_TYPE_VOID && > @@ -2186,13 +2197,11 @@ ovsdb_datum_added_removed(struct ovsdb_datum *added, > } > > for (; oi < old->n; oi++) { > - ovsdb_datum_add_unsafe(removed, &old->keys[oi], &old->values[oi], > - type, NULL); > + ovsdb_datum_add_unsafe_index(removed, old, oi, type); > } > > for (; ni < new->n; ni++) { > - ovsdb_datum_add_unsafe(added, &new->keys[ni], &new->values[ni], > - type, NULL); > + ovsdb_datum_add_unsafe_index(added, new, ni, type); > } > } > > @@ -2228,12 +2237,10 @@ ovsdb_datum_diff(struct ovsdb_datum *diff, > int c = ovsdb_atom_compare_3way(&old->keys[oi], &new->keys[ni], > type->key.type); > if (c < 0) { > - ovsdb_datum_add_unsafe(diff, &old->keys[oi], &old->values[oi], > - type, NULL); > + ovsdb_datum_add_unsafe_index(diff, old, oi, type); > oi++; > } else if (c > 0) { > - ovsdb_datum_add_unsafe(diff, &new->keys[ni], &new->values[ni], > - type, NULL); > + ovsdb_datum_add_unsafe_index(diff, new, ni, type); > ni++; > } else { > if (type->value.type != OVSDB_TYPE_VOID && > @@ -2247,13 +2254,11 @@ ovsdb_datum_diff(struct ovsdb_datum *diff, > } > > for (; oi < old->n; oi++) { > - ovsdb_datum_add_unsafe(diff, &old->keys[oi], &old->values[oi], > - type, NULL); > + ovsdb_datum_add_unsafe_index(diff, old, oi, type); > } > > for (; ni < new->n; ni++) { > - ovsdb_datum_add_unsafe(diff, &new->keys[ni], &new->values[ni], > - type, NULL); > + ovsdb_datum_add_unsafe_index(diff, new, ni, type); > } > } > > diff --git a/lib/ovsdb-data.h b/lib/ovsdb-data.h > index 47115a7b85b7..31c7c27e6b93 100644 > --- a/lib/ovsdb-data.h > +++ b/lib/ovsdb-data.h > @@ -280,6 +280,10 @@ void ovsdb_datum_add_unsafe(struct ovsdb_datum *, > const union ovsdb_atom *value, > const struct ovsdb_type *, > const union ovsdb_atom *range_end_atom); > +void ovsdb_datum_add_unsafe_index(struct ovsdb_datum *dst, > + const struct ovsdb_datum *src, > + size_t idx, > + const struct ovsdb_type *type); > > /* Transactions with named-uuid row names. */ > struct json *ovsdb_datum_to_json_with_row_names(const struct ovsdb_datum *, > diff --git a/lib/sset.c b/lib/sset.c > index b2e3f43ec91b..b18b948fa4e7 100644 > --- a/lib/sset.c > +++ b/lib/sset.c > @@ -312,7 +312,9 @@ sset_at_position(const struct sset *set, struct sset_position *pos) > struct hmap_node *hmap_node; > > hmap_node = hmap_at_position(&set->map, &pos->pos); > - return SSET_NODE_FROM_HMAP_NODE(hmap_node); > + return hmap_node > + ? SSET_NODE_FROM_HMAP_NODE(hmap_node) > + : NULL; > } > > /* Replaces 'a' by the intersection of 'a' and 'b'. That is, removes from 'a' > diff --git a/lib/tnl-ports.c b/lib/tnl-ports.c > index 58269d3b1631..dd3320ccbb96 100644 > --- a/lib/tnl-ports.c > +++ b/lib/tnl-ports.c > @@ -71,7 +71,7 @@ tnl_port_cast(const struct cls_rule *cr) > { > BUILD_ASSERT_DECL(offsetof(struct tnl_port_in, cr) == 0); > > - return CONTAINER_OF(cr, struct tnl_port_in, cr); > + return cr ? CONTAINER_OF(cr, struct tnl_port_in, cr) : NULL; > } > > static void > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > index 56aeac720935..c3ffbbb13b5b 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -8902,7 +8902,7 @@ eviction_group_hash_rule(struct rule *rule) > hash = table->eviction_group_id_basis; > miniflow_expand(rule->cr.match.flow, &flow); > for (sf = table->eviction_fields; > - sf < &table->eviction_fields[table->n_eviction_fields]; > + sf && sf < &table->eviction_fields[table->n_eviction_fields]; > sf++) > { > if (mf_are_prereqs_ok(sf->field, &flow, NULL)) { >
On 2/15/22 00:04, Ilya Maximets wrote: > On 1/24/22 15:19, Dumitru Ceara wrote: >> This is undefined behavior and was reported by UB Sanitizer: >> lib/meta-flow.c:3445:16: runtime error: member access within null pointer of type 'struct vl_mf_field' >> #0 0x6aad0f in mf_get_vl_mff lib/meta-flow.c:3445 >> #1 0x6d96d7 in mf_from_oxm_header lib/nx-match.c:260 >> #2 0x6d9e2e in nx_pull_header__ lib/nx-match.c:341 >> #3 0x6daafa in nx_pull_header lib/nx-match.c:488 >> #4 0x6abcb6 in mf_vl_mff_nx_pull_header lib/meta-flow.c:3605 >> #5 0x73b9be in decode_NXAST_RAW_REG_MOVE lib/ofp-actions.c:2652 >> #6 0x764ccd in ofpact_decode lib/ofp-actions.inc2:4681 >> [...] >> lib/sset.c:315:12: runtime error: applying zero offset to null pointer >> #0 0xcc2e6a in sset_at_position /root/ovs/lib/sset.c:315:12 >> #1 0x5734b3 in port_dump_next /root/ovs/ofproto/ofproto-dpif.c:4083:20 >> [...] >> lib/ovsdb-data.c:2194:56: runtime error: applying zero offset to null pointer >> #0 0x5e9530 in ovsdb_datum_added_removed /root/ovs/lib/ovsdb-data.c:2194:56 >> #1 0x4d6258 in update_row_ref_count /root/ovs/ovsdb/transaction.c:335:17 >> #2 0x4c360b in for_each_txn_row /root/ovs/ovsdb/transaction.c:1572:33 >> [...] >> lib/ofpbuf.c:440:30: runtime error: applying zero offset to null pointer >> #0 0x75066d in ofpbuf_push_uninit lib/ofpbuf.c:440 >> #1 0x46ac8a in ovnacts_parse lib/actions.c:4190 >> #2 0x46ad91 in ovnacts_parse_string lib/actions.c:4208 >> #3 0x4106d1 in test_parse_actions tests/test-ovn.c:1324 >> [...] >> lib/ofp-actions.c:3205:22: runtime error: applying non-zero offset 2 to null pointer >> #0 0x6e1641 in set_field_split_str /root/ovs/lib/ofp-actions.c:3205:22 >> [...] >> lib/tnl-ports.c:74:12: runtime error: applying zero offset to null pointer >> #0 0xceffe7 in tnl_port_cast /root/ovs/lib/tnl-ports.c:74:12 >> #1 0xcf14c3 in map_insert /root/ovs/lib/tnl-ports.c:116:13 >> [...] >> ofproto/ofproto.c:8905:16: runtime error: applying zero offset to null pointer >> #0 0x556795 in eviction_group_hash_rule /root/ovs/ofproto/ofproto.c:8905:16 >> #1 0x503f8d in eviction_group_add_rule /root/ovs/ofproto/ofproto.c:9022:42 >> [...] >> >> Also, it's valid to have an empty ofpact list and we should be able to >> try to iterate through it. >> >> UB Sanitizer report: >> include/openvswitch/ofp-actions.h:222:12: runtime error: applying zero offset to null pointer >> #0 0x665d69 in ofpact_end /root/ovs/./include/openvswitch/ofp-actions.h:222:12 >> #1 0x66b2cf in ofpacts_put_openflow_actions /root/ovs/lib/ofp-actions.c:8861:5 >> #2 0x6ffdd1 in ofputil_encode_flow_mod /root/ovs/lib/ofp-flow.c:447:9 >> [...] >> >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> --- >> include/openvswitch/ofp-actions.h | 4 +++- >> include/openvswitch/ofpbuf.h | 17 ++++++++++++----- >> lib/dynamic-string.c | 10 ++++++++-- >> lib/meta-flow.c | 4 +++- >> lib/ofp-actions.c | 8 ++++---- >> lib/ofpbuf.c | 4 ++++ >> lib/ovsdb-data.c | 37 +++++++++++++++++++++---------------- >> lib/ovsdb-data.h | 4 ++++ >> lib/sset.c | 4 +++- >> lib/tnl-ports.c | 2 +- >> ofproto/ofproto.c | 2 +- >> 11 files changed, 64 insertions(+), 32 deletions(-) >> >> diff --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..a9ff8a56e81c 100644 >> --- a/include/openvswitch/ofpbuf.h >> +++ b/include/openvswitch/ofpbuf.h >> @@ -179,7 +179,9 @@ static inline void ofpbuf_delete(struct ofpbuf *b) >> static inline void *ofpbuf_at(const struct ofpbuf *b, size_t offset, >> size_t size) >> { >> - return offset + size <= b->size ? (char *) b->data + offset : NULL; >> + return offset + size <= b->size && b->data > > It's better to parenthesize the condition. > Ack. >> + ? (char *) b->data + offset >> + : NULL; >> } >> >> /* Returns a pointer to byte 'offset' in 'b', which must contain at least >> @@ -188,20 +190,20 @@ static inline void *ofpbuf_at_assert(const struct ofpbuf *b, size_t offset, >> size_t size) >> { >> ovs_assert(offset + size <= b->size); >> - return ((char *) b->data) + offset; >> + return b->data ? (char *) b->data + offset : NULL; >> } >> >> /* Returns a pointer to byte following the last byte of data in use in 'b'. */ >> static inline void *ofpbuf_tail(const struct ofpbuf *b) >> { >> - return (char *) b->data + b->size; >> + return b->data ? (char *) b->data + b->size : NULL; >> } >> >> /* Returns a pointer to byte following the last byte allocated for use (but >> * not necessarily in use) in 'b'. */ >> static inline void *ofpbuf_end(const struct ofpbuf *b) >> { >> - return (char *) b->base + b->allocated; >> + return b->base ? (char *) b->base + b->allocated : NULL; >> } >> >> /* Returns the number of bytes of headroom in 'b', that is, the number of bytes >> @@ -249,6 +251,11 @@ static inline void *ofpbuf_pull(struct ofpbuf *b, size_t size) >> { >> ovs_assert(b->size >= size); >> void *data = b->data; >> + >> + if (!size) { >> + return data; >> + } >> + >> b->data = (char*)b->data + size; >> b->size = b->size - size; >> return data; >> @@ -270,7 +277,7 @@ static inline struct ofpbuf *ofpbuf_from_list(const struct ovs_list *list) >> static inline bool ofpbuf_equal(const struct ofpbuf *a, const struct ofpbuf *b) >> { >> return a->size == b->size && >> - memcmp(a->data, b->data, a->size) == 0; >> + (a->size == 0 || memcmp(a->data, b->data, a->size) == 0); >> } >> >> static inline bool ofpbuf_oversized(const struct ofpbuf *ofpacts) >> diff --git a/lib/dynamic-string.c b/lib/dynamic-string.c >> index fd0127ed1740..9b9a8f50e562 100644 >> --- a/lib/dynamic-string.c >> +++ b/lib/dynamic-string.c >> @@ -152,7 +152,10 @@ ds_put_format_valist(struct ds *ds, const char *format, va_list args_) >> >> va_copy(args, args_); >> available = ds->string ? ds->allocated - ds->length + 1 : 0; >> - needed = vsnprintf(&ds->string[ds->length], available, format, args); >> + needed = vsnprintf(ds->string >> + ? &ds->string[ds->length] >> + : NULL, >> + available, format, args); >> va_end(args); >> >> if (needed < available) { >> @@ -162,7 +165,10 @@ ds_put_format_valist(struct ds *ds, const char *format, va_list args_) >> >> va_copy(args, args_); >> available = ds->allocated - ds->length + 1; >> - needed = vsnprintf(&ds->string[ds->length], available, format, args); >> + needed = vsnprintf(ds->string > > This pointer can't be NULL here. We already called ds_reserve(). > Ack. >> + ? &ds->string[ds->length] >> + : NULL, >> + available, format, args); >> va_end(args); >> >> ovs_assert(needed < available); >> diff --git a/lib/meta-flow.c b/lib/meta-flow.c >> index 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 006837c2e1f5..b24b46d2196b 100644 >> --- a/lib/ofp-actions.c >> +++ b/lib/ofp-actions.c >> @@ -3202,14 +3202,14 @@ set_field_split_str(char *arg, char **key, char **value, char **delim) >> >> *value = arg; >> value_end = strstr(arg, "->"); >> + if (!value_end) { >> + return xasprintf("%s: missing `->'", arg); >> + } >> + >> *key = value_end + strlen("->"); >> if (delim) { >> *delim = value_end; >> } >> - >> - if (!value_end) { >> - return xasprintf("%s: missing `->'", arg); >> - } >> if (strlen(value_end) <= strlen("->")) { >> return xasprintf("%s: missing field name following `->'", arg); >> } >> diff --git a/lib/ofpbuf.c b/lib/ofpbuf.c >> index 271105bdea17..0330681b2518 100644 >> --- a/lib/ofpbuf.c >> +++ b/lib/ofpbuf.c >> @@ -436,6 +436,10 @@ ofpbuf_reserve(struct ofpbuf *b, size_t size) >> void * >> ofpbuf_push_uninit(struct ofpbuf *b, size_t size) >> { >> + if (!size) { >> + return b->data; >> + } >> + >> ofpbuf_prealloc_headroom(b, size); >> b->data = (char*)b->data - size; >> b->size += size; >> diff --git a/lib/ovsdb-data.c b/lib/ovsdb-data.c >> index 6b1c20ff85a0..306b8d0d946a 100644 >> --- a/lib/ovsdb-data.c >> +++ b/lib/ovsdb-data.c >> @@ -1957,6 +1957,19 @@ ovsdb_datum_add_unsafe(struct ovsdb_datum *datum, >> } >> } >> >> +void >> +ovsdb_datum_add_unsafe_index(struct ovsdb_datum *dst, > > Maybe ovsdb_datum_add_from_index_unsafe() or something like that? > Sounds good. >> + const struct ovsdb_datum *src, >> + size_t idx, >> + const struct ovsdb_type *type) >> +{ >> + const union ovsdb_atom *key = &src->keys[idx]; >> + const union ovsdb_atom *value = type->value.type != OVSDB_TYPE_VOID >> + ? &src->values[idx] >> + : NULL; >> + ovsdb_datum_add_unsafe(dst, key, value, type, NULL); >> +} >> + >> /* Adds 'n' atoms starting from index 'start_idx' from 'src' to the end of >> * 'dst'. 'dst' should have enough memory allocated to hold the additional >> * 'n' atoms. Atoms are not cloned, i.e. 'dst' will reference the same data. >> @@ -2165,12 +2178,10 @@ ovsdb_datum_added_removed(struct ovsdb_datum *added, >> int c = ovsdb_atom_compare_3way(&old->keys[oi], &new->keys[ni], >> type->key.type); >> if (c < 0) { >> - ovsdb_datum_add_unsafe(removed, &old->keys[oi], &old->values[oi], >> - type, NULL); >> + ovsdb_datum_add_unsafe_index(removed, old, oi, type); >> oi++; >> } else if (c > 0) { >> - ovsdb_datum_add_unsafe(added, &new->keys[ni], &new->values[ni], >> - type, NULL); >> + ovsdb_datum_add_unsafe_index(added, new, ni, type); >> ni++; >> } else { >> if (type->value.type != OVSDB_TYPE_VOID && >> @@ -2186,13 +2197,11 @@ ovsdb_datum_added_removed(struct ovsdb_datum *added, >> } >> >> for (; oi < old->n; oi++) { >> - ovsdb_datum_add_unsafe(removed, &old->keys[oi], &old->values[oi], >> - type, NULL); >> + ovsdb_datum_add_unsafe_index(removed, old, oi, type); >> } >> >> for (; ni < new->n; ni++) { >> - ovsdb_datum_add_unsafe(added, &new->keys[ni], &new->values[ni], >> - type, NULL); >> + ovsdb_datum_add_unsafe_index(added, new, ni, type); >> } >> } >> >> @@ -2228,12 +2237,10 @@ ovsdb_datum_diff(struct ovsdb_datum *diff, >> int c = ovsdb_atom_compare_3way(&old->keys[oi], &new->keys[ni], >> type->key.type); >> if (c < 0) { >> - ovsdb_datum_add_unsafe(diff, &old->keys[oi], &old->values[oi], >> - type, NULL); >> + ovsdb_datum_add_unsafe_index(diff, old, oi, type); >> oi++; >> } else if (c > 0) { >> - ovsdb_datum_add_unsafe(diff, &new->keys[ni], &new->values[ni], >> - type, NULL); >> + ovsdb_datum_add_unsafe_index(diff, new, ni, type); >> ni++; >> } else { >> if (type->value.type != OVSDB_TYPE_VOID && >> @@ -2247,13 +2254,11 @@ ovsdb_datum_diff(struct ovsdb_datum *diff, >> } >> >> for (; oi < old->n; oi++) { >> - ovsdb_datum_add_unsafe(diff, &old->keys[oi], &old->values[oi], >> - type, NULL); >> + ovsdb_datum_add_unsafe_index(diff, old, oi, type); >> } >> >> for (; ni < new->n; ni++) { >> - ovsdb_datum_add_unsafe(diff, &new->keys[ni], &new->values[ni], >> - type, NULL); >> + ovsdb_datum_add_unsafe_index(diff, new, ni, type); >> } >> } >> >> diff --git a/lib/ovsdb-data.h b/lib/ovsdb-data.h >> index 47115a7b85b7..31c7c27e6b93 100644 >> --- a/lib/ovsdb-data.h >> +++ b/lib/ovsdb-data.h >> @@ -280,6 +280,10 @@ void ovsdb_datum_add_unsafe(struct ovsdb_datum *, >> const union ovsdb_atom *value, >> const struct ovsdb_type *, >> const union ovsdb_atom *range_end_atom); >> +void ovsdb_datum_add_unsafe_index(struct ovsdb_datum *dst, >> + const struct ovsdb_datum *src, >> + size_t idx, >> + const struct ovsdb_type *type); >> >> /* Transactions with named-uuid row names. */ >> struct json *ovsdb_datum_to_json_with_row_names(const struct ovsdb_datum *, >> diff --git a/lib/sset.c b/lib/sset.c >> index b2e3f43ec91b..b18b948fa4e7 100644 >> --- a/lib/sset.c >> +++ b/lib/sset.c >> @@ -312,7 +312,9 @@ sset_at_position(const struct sset *set, struct sset_position *pos) >> struct hmap_node *hmap_node; >> >> hmap_node = hmap_at_position(&set->map, &pos->pos); >> - return SSET_NODE_FROM_HMAP_NODE(hmap_node); >> + return hmap_node >> + ? SSET_NODE_FROM_HMAP_NODE(hmap_node) >> + : NULL; >> } >> >> /* Replaces 'a' by the intersection of 'a' and 'b'. That is, removes from 'a' >> diff --git a/lib/tnl-ports.c b/lib/tnl-ports.c >> index 58269d3b1631..dd3320ccbb96 100644 >> --- a/lib/tnl-ports.c >> +++ b/lib/tnl-ports.c >> @@ -71,7 +71,7 @@ tnl_port_cast(const struct cls_rule *cr) >> { >> BUILD_ASSERT_DECL(offsetof(struct tnl_port_in, cr) == 0); >> >> - return CONTAINER_OF(cr, struct tnl_port_in, cr); >> + return cr ? CONTAINER_OF(cr, struct tnl_port_in, cr) : NULL; >> } >> >> static void >> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c >> index 56aeac720935..c3ffbbb13b5b 100644 >> --- a/ofproto/ofproto.c >> +++ b/ofproto/ofproto.c >> @@ -8902,7 +8902,7 @@ eviction_group_hash_rule(struct rule *rule) >> hash = table->eviction_group_id_basis; >> miniflow_expand(rule->cr.match.flow, &flow); >> for (sf = table->eviction_fields; >> - sf < &table->eviction_fields[table->n_eviction_fields]; >> + sf && sf < &table->eviction_fields[table->n_eviction_fields]; >> sf++) >> { >> if (mf_are_prereqs_ok(sf->field, &flow, NULL)) { >> >
On 2/25/22 18:29, Jeffrey Walton wrote: >> @@ -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); >> } > Please forgive my ignorance... a != 0 and b == 0? Or can b never be 0? > a and b can't be zero at this point, otherwise we would've de-referenced NULL in "a->size == b->size". I guess I did it again, I used the fact that if a->size == b->size == 0 then we should assume that the ofpbuf 'a' and ofpbuf 'b' are equal, equivalent to "empty" ofpbufs, regardless of values of 'a->data' and b->data'. But this change doesn't really belong in this patch, it should've been part of 8ed26a8be3be ("treewide: Don't pass NULL to library functions that expect non-NULL."). I can split it out if needed and respin a v5 after v4 gets some reviews. Thanks for pointing it out! Regards, Dumitru
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..a9ff8a56e81c 100644 --- a/include/openvswitch/ofpbuf.h +++ b/include/openvswitch/ofpbuf.h @@ -179,7 +179,9 @@ static inline void ofpbuf_delete(struct ofpbuf *b) static inline void *ofpbuf_at(const struct ofpbuf *b, size_t offset, size_t size) { - return offset + size <= b->size ? (char *) b->data + offset : NULL; + return offset + size <= b->size && b->data + ? (char *) b->data + offset + : NULL; } /* Returns a pointer to byte 'offset' in 'b', which must contain at least @@ -188,20 +190,20 @@ static inline void *ofpbuf_at_assert(const struct ofpbuf *b, size_t offset, size_t size) { ovs_assert(offset + size <= b->size); - return ((char *) b->data) + offset; + return b->data ? (char *) b->data + offset : NULL; } /* Returns a pointer to byte following the last byte of data in use in 'b'. */ static inline void *ofpbuf_tail(const struct ofpbuf *b) { - return (char *) b->data + b->size; + return b->data ? (char *) b->data + b->size : NULL; } /* Returns a pointer to byte following the last byte allocated for use (but * not necessarily in use) in 'b'. */ static inline void *ofpbuf_end(const struct ofpbuf *b) { - return (char *) b->base + b->allocated; + return b->base ? (char *) b->base + b->allocated : NULL; } /* Returns the number of bytes of headroom in 'b', that is, the number of bytes @@ -249,6 +251,11 @@ static inline void *ofpbuf_pull(struct ofpbuf *b, size_t size) { ovs_assert(b->size >= size); void *data = b->data; + + if (!size) { + return data; + } + b->data = (char*)b->data + size; b->size = b->size - size; return data; @@ -270,7 +277,7 @@ static inline struct ofpbuf *ofpbuf_from_list(const struct ovs_list *list) static inline bool ofpbuf_equal(const struct ofpbuf *a, const struct ofpbuf *b) { return a->size == b->size && - memcmp(a->data, b->data, a->size) == 0; + (a->size == 0 || memcmp(a->data, b->data, a->size) == 0); } static inline bool ofpbuf_oversized(const struct ofpbuf *ofpacts) diff --git a/lib/dynamic-string.c b/lib/dynamic-string.c index fd0127ed1740..9b9a8f50e562 100644 --- a/lib/dynamic-string.c +++ b/lib/dynamic-string.c @@ -152,7 +152,10 @@ ds_put_format_valist(struct ds *ds, const char *format, va_list args_) va_copy(args, args_); available = ds->string ? ds->allocated - ds->length + 1 : 0; - needed = vsnprintf(&ds->string[ds->length], available, format, args); + needed = vsnprintf(ds->string + ? &ds->string[ds->length] + : NULL, + available, format, args); va_end(args); if (needed < available) { @@ -162,7 +165,10 @@ ds_put_format_valist(struct ds *ds, const char *format, va_list args_) va_copy(args, args_); available = ds->allocated - ds->length + 1; - needed = vsnprintf(&ds->string[ds->length], available, format, args); + needed = vsnprintf(ds->string + ? &ds->string[ds->length] + : NULL, + available, format, args); va_end(args); ovs_assert(needed < available); diff --git a/lib/meta-flow.c b/lib/meta-flow.c index 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 006837c2e1f5..b24b46d2196b 100644 --- a/lib/ofp-actions.c +++ b/lib/ofp-actions.c @@ -3202,14 +3202,14 @@ set_field_split_str(char *arg, char **key, char **value, char **delim) *value = arg; value_end = strstr(arg, "->"); + if (!value_end) { + return xasprintf("%s: missing `->'", arg); + } + *key = value_end + strlen("->"); if (delim) { *delim = value_end; } - - if (!value_end) { - return xasprintf("%s: missing `->'", arg); - } if (strlen(value_end) <= strlen("->")) { return xasprintf("%s: missing field name following `->'", arg); } diff --git a/lib/ofpbuf.c b/lib/ofpbuf.c index 271105bdea17..0330681b2518 100644 --- a/lib/ofpbuf.c +++ b/lib/ofpbuf.c @@ -436,6 +436,10 @@ ofpbuf_reserve(struct ofpbuf *b, size_t size) void * ofpbuf_push_uninit(struct ofpbuf *b, size_t size) { + if (!size) { + return b->data; + } + ofpbuf_prealloc_headroom(b, size); b->data = (char*)b->data - size; b->size += size; diff --git a/lib/ovsdb-data.c b/lib/ovsdb-data.c index 6b1c20ff85a0..306b8d0d946a 100644 --- a/lib/ovsdb-data.c +++ b/lib/ovsdb-data.c @@ -1957,6 +1957,19 @@ ovsdb_datum_add_unsafe(struct ovsdb_datum *datum, } } +void +ovsdb_datum_add_unsafe_index(struct ovsdb_datum *dst, + const struct ovsdb_datum *src, + size_t idx, + const struct ovsdb_type *type) +{ + const union ovsdb_atom *key = &src->keys[idx]; + const union ovsdb_atom *value = type->value.type != OVSDB_TYPE_VOID + ? &src->values[idx] + : NULL; + ovsdb_datum_add_unsafe(dst, key, value, type, NULL); +} + /* Adds 'n' atoms starting from index 'start_idx' from 'src' to the end of * 'dst'. 'dst' should have enough memory allocated to hold the additional * 'n' atoms. Atoms are not cloned, i.e. 'dst' will reference the same data. @@ -2165,12 +2178,10 @@ ovsdb_datum_added_removed(struct ovsdb_datum *added, int c = ovsdb_atom_compare_3way(&old->keys[oi], &new->keys[ni], type->key.type); if (c < 0) { - ovsdb_datum_add_unsafe(removed, &old->keys[oi], &old->values[oi], - type, NULL); + ovsdb_datum_add_unsafe_index(removed, old, oi, type); oi++; } else if (c > 0) { - ovsdb_datum_add_unsafe(added, &new->keys[ni], &new->values[ni], - type, NULL); + ovsdb_datum_add_unsafe_index(added, new, ni, type); ni++; } else { if (type->value.type != OVSDB_TYPE_VOID && @@ -2186,13 +2197,11 @@ ovsdb_datum_added_removed(struct ovsdb_datum *added, } for (; oi < old->n; oi++) { - ovsdb_datum_add_unsafe(removed, &old->keys[oi], &old->values[oi], - type, NULL); + ovsdb_datum_add_unsafe_index(removed, old, oi, type); } for (; ni < new->n; ni++) { - ovsdb_datum_add_unsafe(added, &new->keys[ni], &new->values[ni], - type, NULL); + ovsdb_datum_add_unsafe_index(added, new, ni, type); } } @@ -2228,12 +2237,10 @@ ovsdb_datum_diff(struct ovsdb_datum *diff, int c = ovsdb_atom_compare_3way(&old->keys[oi], &new->keys[ni], type->key.type); if (c < 0) { - ovsdb_datum_add_unsafe(diff, &old->keys[oi], &old->values[oi], - type, NULL); + ovsdb_datum_add_unsafe_index(diff, old, oi, type); oi++; } else if (c > 0) { - ovsdb_datum_add_unsafe(diff, &new->keys[ni], &new->values[ni], - type, NULL); + ovsdb_datum_add_unsafe_index(diff, new, ni, type); ni++; } else { if (type->value.type != OVSDB_TYPE_VOID && @@ -2247,13 +2254,11 @@ ovsdb_datum_diff(struct ovsdb_datum *diff, } for (; oi < old->n; oi++) { - ovsdb_datum_add_unsafe(diff, &old->keys[oi], &old->values[oi], - type, NULL); + ovsdb_datum_add_unsafe_index(diff, old, oi, type); } for (; ni < new->n; ni++) { - ovsdb_datum_add_unsafe(diff, &new->keys[ni], &new->values[ni], - type, NULL); + ovsdb_datum_add_unsafe_index(diff, new, ni, type); } } diff --git a/lib/ovsdb-data.h b/lib/ovsdb-data.h index 47115a7b85b7..31c7c27e6b93 100644 --- a/lib/ovsdb-data.h +++ b/lib/ovsdb-data.h @@ -280,6 +280,10 @@ void ovsdb_datum_add_unsafe(struct ovsdb_datum *, const union ovsdb_atom *value, const struct ovsdb_type *, const union ovsdb_atom *range_end_atom); +void ovsdb_datum_add_unsafe_index(struct ovsdb_datum *dst, + const struct ovsdb_datum *src, + size_t idx, + const struct ovsdb_type *type); /* Transactions with named-uuid row names. */ struct json *ovsdb_datum_to_json_with_row_names(const struct ovsdb_datum *, diff --git a/lib/sset.c b/lib/sset.c index b2e3f43ec91b..b18b948fa4e7 100644 --- a/lib/sset.c +++ b/lib/sset.c @@ -312,7 +312,9 @@ sset_at_position(const struct sset *set, struct sset_position *pos) struct hmap_node *hmap_node; hmap_node = hmap_at_position(&set->map, &pos->pos); - return SSET_NODE_FROM_HMAP_NODE(hmap_node); + return hmap_node + ? SSET_NODE_FROM_HMAP_NODE(hmap_node) + : NULL; } /* Replaces 'a' by the intersection of 'a' and 'b'. That is, removes from 'a' diff --git a/lib/tnl-ports.c b/lib/tnl-ports.c index 58269d3b1631..dd3320ccbb96 100644 --- a/lib/tnl-ports.c +++ b/lib/tnl-ports.c @@ -71,7 +71,7 @@ tnl_port_cast(const struct cls_rule *cr) { BUILD_ASSERT_DECL(offsetof(struct tnl_port_in, cr) == 0); - return CONTAINER_OF(cr, struct tnl_port_in, cr); + return cr ? CONTAINER_OF(cr, struct tnl_port_in, cr) : NULL; } static void diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 56aeac720935..c3ffbbb13b5b 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -8902,7 +8902,7 @@ eviction_group_hash_rule(struct rule *rule) hash = table->eviction_group_id_basis; miniflow_expand(rule->cr.match.flow, &flow); for (sf = table->eviction_fields; - sf < &table->eviction_fields[table->n_eviction_fields]; + sf && sf < &table->eviction_fields[table->n_eviction_fields]; sf++) { if (mf_are_prereqs_ok(sf->field, &flow, NULL)) {
This is undefined behavior and was reported by UB Sanitizer: lib/meta-flow.c:3445:16: runtime error: member access within null pointer of type 'struct vl_mf_field' #0 0x6aad0f in mf_get_vl_mff lib/meta-flow.c:3445 #1 0x6d96d7 in mf_from_oxm_header lib/nx-match.c:260 #2 0x6d9e2e in nx_pull_header__ lib/nx-match.c:341 #3 0x6daafa in nx_pull_header lib/nx-match.c:488 #4 0x6abcb6 in mf_vl_mff_nx_pull_header lib/meta-flow.c:3605 #5 0x73b9be in decode_NXAST_RAW_REG_MOVE lib/ofp-actions.c:2652 #6 0x764ccd in ofpact_decode lib/ofp-actions.inc2:4681 [...] lib/sset.c:315:12: runtime error: applying zero offset to null pointer #0 0xcc2e6a in sset_at_position /root/ovs/lib/sset.c:315:12 #1 0x5734b3 in port_dump_next /root/ovs/ofproto/ofproto-dpif.c:4083:20 [...] lib/ovsdb-data.c:2194:56: runtime error: applying zero offset to null pointer #0 0x5e9530 in ovsdb_datum_added_removed /root/ovs/lib/ovsdb-data.c:2194:56 #1 0x4d6258 in update_row_ref_count /root/ovs/ovsdb/transaction.c:335:17 #2 0x4c360b in for_each_txn_row /root/ovs/ovsdb/transaction.c:1572:33 [...] lib/ofpbuf.c:440:30: runtime error: applying zero offset to null pointer #0 0x75066d in ofpbuf_push_uninit lib/ofpbuf.c:440 #1 0x46ac8a in ovnacts_parse lib/actions.c:4190 #2 0x46ad91 in ovnacts_parse_string lib/actions.c:4208 #3 0x4106d1 in test_parse_actions tests/test-ovn.c:1324 [...] lib/ofp-actions.c:3205:22: runtime error: applying non-zero offset 2 to null pointer #0 0x6e1641 in set_field_split_str /root/ovs/lib/ofp-actions.c:3205:22 [...] lib/tnl-ports.c:74:12: runtime error: applying zero offset to null pointer #0 0xceffe7 in tnl_port_cast /root/ovs/lib/tnl-ports.c:74:12 #1 0xcf14c3 in map_insert /root/ovs/lib/tnl-ports.c:116:13 [...] ofproto/ofproto.c:8905:16: runtime error: applying zero offset to null pointer #0 0x556795 in eviction_group_hash_rule /root/ovs/ofproto/ofproto.c:8905:16 #1 0x503f8d in eviction_group_add_rule /root/ovs/ofproto/ofproto.c:9022:42 [...] Also, it's valid to have an empty ofpact list and we should be able to try to iterate through it. UB Sanitizer report: include/openvswitch/ofp-actions.h:222:12: runtime error: applying zero offset to null pointer #0 0x665d69 in ofpact_end /root/ovs/./include/openvswitch/ofp-actions.h:222:12 #1 0x66b2cf in ofpacts_put_openflow_actions /root/ovs/lib/ofp-actions.c:8861:5 #2 0x6ffdd1 in ofputil_encode_flow_mod /root/ovs/lib/ofp-flow.c:447:9 [...] Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- include/openvswitch/ofp-actions.h | 4 +++- include/openvswitch/ofpbuf.h | 17 ++++++++++++----- lib/dynamic-string.c | 10 ++++++++-- lib/meta-flow.c | 4 +++- lib/ofp-actions.c | 8 ++++---- lib/ofpbuf.c | 4 ++++ lib/ovsdb-data.c | 37 +++++++++++++++++++++---------------- lib/ovsdb-data.h | 4 ++++ lib/sset.c | 4 +++- lib/tnl-ports.c | 2 +- ofproto/ofproto.c | 2 +- 11 files changed, 64 insertions(+), 32 deletions(-)