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