When running builds with UBSan, some undefined behavior was detected in the iteration of common data data structures in OVS. Coincidentally, a bug was reported [1] whose root cause whas another, this time undetected, undefined behavior in the iteration macros.
>From both cases, we conclude that the way we're currently iterating the data >structures is prone to errors and UB. This series is an attempt to rewrite >those macros in a UB-safe manner. The core problem is that #define OBJECT_CONTAINING(POINTER, OBJECT, MEMBER) macro is being used on invalid POINTER values. In some cases we use NULL to compute the end-of-loop condition. In others, we allow it to point to non-contained objects (e.g: a non-contained stack allocated "struct ovs_list" as in [1]). In order to systematically solve this in all cases this series introduces a new set of macros that implement a multi-variable loop iteration. They declare a hidden iterator variable inside the loop, use to iterate and evaluate the loop condition and only compute its OBJECT_CONTAINING if it satisfies the loop condition. One consequence of this safety guard is that the pointer provided by the user is set to NULL after the loop (if not exited via "break;"). Apart from normal iteration, many OVS data structures have a _SAFE version of the loop which require the user to declare an extra variable to hold the next value of the iterator. The _SAFE version of the multi-variable iterators have the extra benefit of not requiring such extra variable. On relevant data structures, an initial patch rewrites the macros in a backwards compatible manner and a second patch modifies all the callers to remove the unneeded variable. The fist patch would be easy to backport and the second would make code cleaner for the master branch. Testing notes: In order to verify this series removes all the loop-related UB, I've tested it on top of Dumitru's series [2] (without patch 1/11, which can hide some still invalid use of OBJECT_CONTAINING). I've also verified no extra errors are reported through clang-analyzer. Limitations: The proposed approach benefits code readability, therefore the name of the iterator variable is derived from the name of the object pointer given by the caller. This means that in an unlikely but still possible case in which a caller wants to nest two loops with the same iterating pointer object, the inner loop iterator variable will hide the one declared in the outer loop. This limitation is easy to spot (the compiler will warn) and easy to work around (just declaring another object pointer variable). I found no such code in the ovs tree. Credits: The idea was discussed in [3] and proposed by Jakub Jelinek <[email protected]>. [1] https://bugzilla.redhat.com/show_bug.cgi?id=2014942 [2] https://patchwork.ozlabs.org/project/openvswitch/list/?series=277900 [3] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103964 Adrian Moreno (13): util: add multi-variable loop iterator macros util: add safe multi-variable iterators list: use multi-variable helpers for list loops list: ensure iterator is NULL after pop loop list: remove the next variable in safe loops hmap: use multi-variable helpers for hmap loops hmap: implement UB-safe hmap pop iterator hmap: remove the next variable in safe loops cmap: use multi-variable iterators hindex: use multi-variable iterators hindex: remove the next variable in safe loops rculist: use multi-variable helpers for loop macros vtep: use _SAFE iterator if freing the iterator include/openvswitch/hmap.h | 93 +++++++++++++++++++----------------- include/openvswitch/list.h | 70 +++++++++++++++------------ include/openvswitch/shash.h | 5 +- include/openvswitch/util.h | 78 ++++++++++++++++++++++++++++++ lib/cfm.c | 4 +- lib/classifier.c | 4 +- lib/cmap.h | 24 ++++++---- lib/conntrack.c | 8 ++-- lib/dns-resolve.c | 4 +- lib/dpif-netdev.c | 19 ++++---- lib/fat-rwlock.c | 4 +- lib/hindex.h | 40 ++++++++-------- lib/hmapx.c | 4 +- lib/hmapx.h | 5 +- lib/ipf.c | 22 ++++----- lib/json.c | 4 +- lib/lacp.c | 4 +- lib/lldp/lldpd-structs.c | 7 ++- lib/lldp/lldpd.c | 8 ++-- lib/mac-learning.c | 4 +- lib/mcast-snooping.c | 12 ++--- lib/namemap.c | 4 +- lib/netdev-afxdp.c | 4 +- lib/netdev-dpdk.c | 8 ++-- lib/netdev-linux.c | 4 +- lib/netdev-offload-tc.c | 4 +- lib/ofp-msgs.c | 8 ++-- lib/ovs-lldp.c | 12 ++--- lib/ovs-numa.h | 4 +- lib/ovsdb-cs.c | 12 ++--- lib/ovsdb-idl.c | 46 +++++++++--------- lib/ovsdb-map-op.c | 4 +- lib/ovsdb-set-op.c | 4 +- lib/pcap-file.c | 4 +- lib/perf-counter.c | 4 +- lib/poll-loop.c | 4 +- lib/rculist.h | 61 ++++++++++++----------- lib/seq.c | 8 ++-- lib/shash.c | 8 ++-- lib/simap.c | 4 +- lib/simap.h | 5 +- lib/smap.c | 4 +- lib/smap.h | 5 +- lib/stopwatch.c | 4 +- lib/tnl-ports.c | 16 +++---- lib/unixctl.c | 8 ++-- lib/vconn.c | 4 +- ofproto/bond.c | 6 +-- ofproto/connmgr.c | 28 +++++------ ofproto/in-band.c | 4 +- ofproto/netflow.c | 8 ++-- ofproto/ofproto-dpif-ipfix.c | 12 ++--- ofproto/ofproto-dpif-sflow.c | 4 +- ofproto/ofproto-dpif-trace.c | 4 +- ofproto/ofproto-dpif-xlate.c | 12 ++--- ofproto/ofproto-dpif.c | 28 +++++------ ofproto/ofproto.c | 16 +++---- ovsdb/condition.c | 8 ++-- ovsdb/jsonrpc-server.c | 40 ++++++++-------- ovsdb/monitor.c | 36 +++++++------- ovsdb/ovsdb-server.c | 11 ++--- ovsdb/ovsdb-tool.c | 7 ++- ovsdb/ovsdb.c | 4 +- ovsdb/query.c | 4 +- ovsdb/raft-private.c | 4 +- ovsdb/raft.c | 33 +++++++------ ovsdb/relay.c | 4 +- ovsdb/replication.c | 8 ++-- ovsdb/table.c | 4 +- ovsdb/transaction-forward.c | 10 ++-- ovsdb/transaction.c | 36 +++++++------- ovsdb/trigger.c | 4 +- tests/test-cmap.c | 4 +- tests/test-hindex.c | 4 +- tests/test-hmap.c | 4 +- tests/test-list.c | 10 ++-- utilities/ovs-ofctl.c | 4 +- utilities/ovs-vsctl.c | 12 ++--- vswitchd/bridge.c | 72 ++++++++++++++-------------- vtep/vtep-ctl.c | 20 ++++---- 80 files changed, 616 insertions(+), 525 deletions(-) -- 2.34.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
