On 4/5/22 21:20, Aaron Conole wrote:
> Dumitru Ceara <[email protected]> writes:
>
>> On 4/5/22 16:41, Aaron Conole wrote:
>>> Dumitru Ceara <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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.
>>
[...]
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
