Hi Dumitru, One small comment. I held it back a few days ago, but I should probably point it out...
On Mon, Jan 24, 2022 at 9:17 AM Dumitru Ceara <[email protected]> wrote: > > As privately reported by Aaron Conole, and by Jeffrey Walton [0] > there's currently a number of undefined behavior instances in > the OVS code base. Running the OVS (and OVN) tests with UBSan [1] > enabled uncovers these. > > This series fixes the issues reported by UBSan and, throught the last > patch, enables UBSan tests in GitHub Actions CI. > > Note: depending on the order of reviews, if Adrian's "Fix undefined > behavior in loop macros" series [2] (or a follow up) is accepted first, > then patch 12/14 ("util: Avoid false positive UB when iterating > collections.") can be skipped. Adrian's series seems to be the more > correct way of fixing the issue. > > [0] https://mail.openvswitch.org/pipermail/ovs-dev/2022-January/390894.html > [1] https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html > [2] > https://patchwork.ozlabs.org/project/openvswitch/list/?series=282481&state=* > > Changes in v3: > - Added acks to the patches acked by Aaron. > - Addressed Aaron's comments. > - Split previous patch 07/11 "ofp-actions: ofp-errors: Use aligned > structures when decoding ofp actions." into three separate patches > addressing independent issues. > - Reordered patches such that the ones that might need follow up are > towards the end of the series. > - Added a new patch, patch 13/14, fixing a typo in the CFLAGS_ASAN > variables in the script used for building OVS in CI. This typo was > originally copy/pasted in the CFLAGS_UBSAN flags in v1 and v2 of > this series. > > Changes in v2: > - Patch 3/11: > - Remove cache line size aligment markers instead, as suggested by > Ilya. > > Dumitru Ceara (14): > treewide: Don't pass NULL to library functions that expect non-NULL. > dpif-netdev: Fix misaligned access. > stopwatch: Fix buffer underflow when computing percentiles. > treewide: Fix invalid bit shift operations. > ovsdb-idlc: Avoid accessing member within NULL idl index cursors. > bfd: lldp: stp: Fix misaligned packet field access. > dp-packet: Ensure packet base is always non-NULL. > treewide: Avoid offsetting NULL pointers. > ofp-actions: Ensure aligned accesses when setting masked fields. > ofp-errors: Ensure parsed OFPT_ERROR messages are properly aligned. > ofp-actions: Use aligned structures when decoding ofp actions. > util: Avoid false positive UB when iterating collections. > ci: Fix typo in variable name. > ci: Add UB Sanitizer. > > > .ci/linux-build.sh | 10 +++- > .github/workflows/build-and-test.yml | 5 ++ > configure.ac | 1 > include/openvswitch/ofp-actions.h | 5 ++ > include/openvswitch/ofpbuf.h | 19 ++++++-- > include/openvswitch/util.h | 5 ++ > lib/bfd.c | 51 ++++++++++++--------- > lib/dp-packet.c | 2 - > lib/dpif-netdev-private-thread.h | 2 - > lib/dpif-netdev.c | 6 ++- > lib/dpif-netlink.c | 2 - > lib/dynamic-string.c | 10 +++- > lib/lldp/lldp.c | 4 +- > lib/meta-flow.c | 10 ++++ > lib/nx-match.c | 7 +++ > lib/ofp-actions.c | 81 > ++++++++++++++++++++++++++-------- > lib/ofp-errors.c | 2 + > lib/ofpbuf.c | 47 ++++++++++++++++++++ > lib/ovsdb-data.c | 41 ++++++++++------- > lib/ovsdb-data.h | 4 ++ > lib/sset.c | 4 +- > lib/stopwatch.c | 4 -- > lib/stp.c | 16 +++---- > lib/tnl-ports.c | 2 - > ofproto/ofproto-dpif-rid.c | 6 ++- > ofproto/ofproto-dpif-xlate.c | 16 +++++-- > ofproto/ofproto.c | 2 - > ovsdb/monitor.c | 4 ++ > ovsdb/ovsdb-idlc.in | 2 - > tests/atlocal.in | 16 +++++++ > tests/automake.mk | 1 > tests/daemon.at | 8 +++ > tests/ovs-macros.at | 5 ++ > tests/ovsdb-server.at | 16 +++++++ > tests/test-hash.c | 2 - > tests/test-util.c | 13 +++-- > 36 files changed, 324 insertions(+), 107 deletions(-) In "[ovs-dev,v2,02/11] treewide: Don't pass NULL to library functions that expect non-NULL", I see this pattern: diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index a790df5fd697..f62202a8b53d 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -4506,8 +4506,10 @@ dp_netdev_actions_create(const struct nlattr *actions, size_t size) struct dp_netdev_actions *netdev_actions; netdev_actions = xmalloc(sizeof *netdev_actions + size); - memcpy(netdev_actions->actions, actions, size); netdev_actions->size = size; + if (size) { + memcpy(netdev_actions->actions, actions, size); + } return netdev_actions; } It looks like `size` is being used for a proxy for the NULL pointers. I personally do not do that (though I did it in the past). Nowadays I precisely address the violation and finding: + if (netdev_actions->actions && actions) { + memcpy(netdev_actions->actions, actions, size); + } The violation is a NULL pointer to memcpy, not a 0-size. I'm not sure if you will ever run into a case where ptr=NULL and size=non-0. I know of some libraries that do meet that condition, but I'm not sure if ovs falls into the category. If ovs does encounter the case, then you will surely SEGFAULT in libraries like Musl. Musl is different from glibc. Musl crashes if fed a NULL pointer. Rich's position is that it is better to crash rather than hide the error. Jeff _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
