On 1/24/22 17:42, Jeffrey Walton wrote: > Hi Dumitru, Hi Jeff,
> > One small comment. I held it back a few days ago, but I should > probably point it out... > Thanks for looking into this. > 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. glibc crashes too when memcpy is passed NULL as argument and non-zero size; at least the glibc on my test machine (glibc-2.33-20.fc34.x86_64). While if I pass it NULL arrays and 0 size it doesn't crash. UBSan still flags it though. As far as I know, OVS doesn't use this pattern (null pointers with non-zero size arguments). And, if it does, in my opinion that's a bug and should cause a crash. > > Jeff > Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
