On 4/6/22 18:36, Ilya Maximets wrote: > On 4/6/22 18:31, Aaron Conole wrote: >> Ilya Maximets <i.maxim...@ovn.org> writes: >> >>> On 4/6/22 16:53, Aaron Conole wrote: >>>> Dumitru Ceara <dce...@redhat.com> writes: >>>> >>>>> On 4/5/22 21:20, Aaron Conole wrote: >>>>>> Dumitru Ceara <dce...@redhat.com> writes: >>>>>> >>>>>>> On 4/5/22 16:41, Aaron Conole wrote: >>>>>>>> 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. >>>>>>>> >>>>>>> >>>>>>> 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. >>>>>>> >>>>> >>>>> [...] >>>> >> > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
