| Message ID | 20220124142038.11777.16794.stgit@dceara.remote.csb |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix UndefinedBehaviorSanitizer reported issues and enable it in CI. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On Mon, Jan 24, 2022 at 9:21 AM Dumitru Ceara <dceara@redhat.com> wrote: > > Note: There still is an UB instance when using SSE4.2 as reported here: > https://mail.openvswitch.org/pipermail/ovs-dev/2022-January/390904.html > > Acked-by: Aaron Conole <aconole@redhat.com> > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > v3: > - Fix typo in variable name. > - Added SSE4.2 UB note to commit log. > --- > .ci/linux-build.sh | 6 ++++++ > .github/workflows/build-and-test.yml | 5 +++++ > configure.ac | 1 + > tests/atlocal.in | 16 ++++++++++++++++ > tests/automake.mk | 1 + > tests/daemon.at | 8 ++++++++ > tests/ovs-macros.at | 5 +++++ > tests/ovsdb-server.at | 16 ++++++++++++++++ > 8 files changed, 58 insertions(+) > > diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh > index 6cd38ff3efb5..afd4379e6923 100755 > --- a/.ci/linux-build.sh > +++ b/.ci/linux-build.sh > @@ -250,6 +250,12 @@ if [ "$ASAN" ]; then > CFLAGS_FOR_OVS="${CFLAGS_FOR_OVS} ${CFLAGS_ASAN}" > fi > > +if [ "$UBSAN" ]; then > + # Use the default options configured in tests/atlocal.in, in UBSAN_OPTIONS. > + CFLAGS_UBSAN="-O1 -fno-omit-frame-pointer -fno-common -fsanitize=undefined" > + CFLAGS_FOR_OVS="${CFLAGS_FOR_OVS} ${CFLAGS_UBSAN}" > +fi > + > save_OPTS="${OPTS} $*" > OPTS="${EXTRA_OPTS} ${save_OPTS}" Sanitizer flags usually placed for compile and link. So they usually go in CFLAGS and LDFLAGS. You will be Ok with putting them in CFLAGS only if you use GNU Make's default: $ make -p | grep '^LINK\.c' LINK.c = $(CC) $(CFLAGS) $(CPPFLAGS) $(LDFLAGS) $(TARGET_ARCH) But you will need to add them to LDFLAGS if you invoke LD directly. You might also want to add -fno-sanitize=integer-divide-by-zero -fno-sanitize=float-divide-by-zero. Divide-by-0's are usually not needed due to IEEE-754, and if left in usually cause more trouble then they are worth. > diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml > index eac3504e48f8..9e3583781baa 100644 > --- a/.github/workflows/build-and-test.yml > +++ b/.github/workflows/build-and-test.yml > @@ -13,6 +13,7 @@ jobs: > linux-headers-$(uname -r) build-essential fakeroot devscripts equivs > AFXDP: ${{ matrix.afxdp }} > ASAN: ${{ matrix.asan }} > + UBSAN: ${{ matrix.ubsan }} > CC: ${{ matrix.compiler }} > DEB_PACKAGE: ${{ matrix.deb_package }} > DPDK: ${{ matrix.dpdk }} > @@ -44,6 +45,10 @@ jobs: > testsuite: test > kernel: 3.16 > asan: asan > + - compiler: clang > + testsuite: test > + kernel: 3.16 > + ubsan: ubsan > > - compiler: gcc > testsuite: test > diff --git a/configure.ac b/configure.ac > index 298ea85aba22..198653a60528 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -197,6 +197,7 @@ OVS_CHECK_LINUX_SCTP_CT > OVS_CHECK_LINUX_VIRTIO_TYPES > OVS_CHECK_DPDK > OVS_CHECK_PRAGMA_MESSAGE > +AC_SUBST([CFLAGS]) > AC_SUBST([OVS_CFLAGS]) > AC_SUBST([OVS_LDFLAGS]) > > diff --git a/tests/atlocal.in b/tests/atlocal.in > index a0ad239ecef2..142ea2090bc8 100644 > --- a/tests/atlocal.in > +++ b/tests/atlocal.in > @@ -4,6 +4,7 @@ OPENSSL_SUPPORTS_SNI='@OPENSSL_SUPPORTS_SNI@' > HAVE_UNBOUND='@HAVE_UNBOUND@' > EGREP='@EGREP@' > PYTHON3='@PYTHON3@' > +CFLAGS='@CFLAGS@' > > # PYTHONCOERCECLOCALE=0 disables the Unicode compatibility warning on > # stderr that breaks almost any Python3 test (PEP 0538) > @@ -197,6 +198,16 @@ else > DIFF_SUPPORTS_NORMAL_FORMAT=no > fi > > +# Check whether UB Sanitizer is being used. > +case "$CFLAGS" in > +*fsanitize=undefined*) > + TESTS_WITH_UBSAN=yes > + ;; > +*) > + TESTS_WITH_UBSAN=no > + ;; > +esac > + > # Turn off proxies. > unset http_proxy > unset https_proxy > @@ -222,3 +233,8 @@ export OVS_CTL_TIMEOUT > # matter break everything. > ASAN_OPTIONS=detect_leaks=0:abort_on_error=true:log_path=asan:$ASAN_OPTIONS > export ASAN_OPTIONS > + > +# Add some default flags for UndefinedBehaviorSanitizer, if it was used > +# for the build. > +UBSAN_OPTIONS=print_stacktrace=1:halt_on_error=true:log_path=ubsan:$UBSAN_OPTIONS > +export UBSAN_OPTIONS > diff --git a/tests/automake.mk b/tests/automake.mk > index 8a9151f81b86..a526be84b493 100644 > --- a/tests/automake.mk > +++ b/tests/automake.mk > @@ -213,6 +213,7 @@ check-local: > set $(SHELL) '$(TESTSUITE)' -C tests AUTOTEST_PATH=$(AUTOTEST_PATH); \ > "$$@" $(TESTSUITEFLAGS) || \ > (test -z "$$(find $(TESTSUITE_DIR) -name 'asan.*')" && \ > + test -z "$$(find $(TESTSUITE_DIR) -name 'ubsan.*')" && \ > test X'$(RECHECK)' = Xyes && "$$@" --recheck) > > # Python Coverage support. > diff --git a/tests/daemon.at b/tests/daemon.at > index 39d9aa391e9b..d7981f9d23a6 100644 > --- a/tests/daemon.at > +++ b/tests/daemon.at > @@ -81,6 +81,10 @@ AT_SKIP_IF([test "$IS_WIN32" = "yes"]) > # This test intentionally causes SIGSEGV, so make Address Sanitizer ignore it. > ASAN_OPTIONS=$ASAN_OPTIONS:handle_segv=0; export ASAN_OPTIONS > > +# Skip it if UB Sanitizer is being used. There's no way to disable the > +# SEGV check at runtime. > +AT_SKIP_IF([test $TESTS_WITH_UBSAN = yes]) > + > # Start the daemon and wait for the pidfile to get created. > on_exit 'kill $(cat *.pid)' > AT_CHECK([ovsdb-server --monitor --pidfile --no-db 2>/dev/null & echo $!], > @@ -149,6 +153,10 @@ AT_SKIP_IF([test "$IS_WIN32" = "yes"]) > # This test intentionally causes SIGSEGV, so make Address Sanitizer ignore it. > ASAN_OPTIONS=$ASAN_OPTIONS:handle_segv=0; export ASAN_OPTIONS > > +# Skip it if UB Sanitizer is being used. There's no way to disable the > +# SEGV check at runtime. > +AT_SKIP_IF([test $TESTS_WITH_UBSAN = yes]) > + > on_exit 'kill $(cat *.pid)' > > # Start the daemon and make sure that the pidfile exists immediately. > diff --git a/tests/ovs-macros.at b/tests/ovs-macros.at > index 66545da5728f..7899583f3c21 100644 > --- a/tests/ovs-macros.at > +++ b/tests/ovs-macros.at > @@ -216,6 +216,11 @@ ovs_cleanup() { > cat asan.* > AT_FAIL_IF([:]) > fi > + if test "$(echo ubsan.*)" != 'ubsan.*'; then > + echo "Undefined Behavior Sanitizer reported errors in:" ubsan.* > + cat ubsan.* > + AT_FAIL_IF([:]) > + fi > } > > ovs_wait () { > diff --git a/tests/ovsdb-server.at b/tests/ovsdb-server.at > index 2b742f78b079..1e00be60684b 100644 > --- a/tests/ovsdb-server.at > +++ b/tests/ovsdb-server.at > @@ -292,6 +292,10 @@ AT_SKIP_IF([test "$IS_WIN32" = "yes"]) > # This test intentionally causes SIGSEGV, so make Address Sanitizer ignore it. > ASAN_OPTIONS=$ASAN_OPTIONS:handle_segv=0; export ASAN_OPTIONS > > +# Skip it if UB Sanitizer is being used. There's no way to disable the > +# SEGV check at runtime. > +AT_SKIP_IF([test $TESTS_WITH_UBSAN = yes]) > + > # Start ovsdb-server, initially with one db. > ordinal_schema > schema > AT_CHECK([ovsdb-tool create db1 schema], [0], [ignore], [ignore]) > @@ -327,6 +331,10 @@ AT_SKIP_IF([test "$IS_WIN32" = "yes"]) > # This test intentionally causes SIGSEGV, so make Address Sanitizer ignore it. > ASAN_OPTIONS=$ASAN_OPTIONS:handle_segv=0; export ASAN_OPTIONS > > +# Skip it if UB Sanitizer is being used. There's no way to disable the > +# SEGV check at runtime. > +AT_SKIP_IF([test $TESTS_WITH_UBSAN = yes]) > + > # Start ovsdb-server, initially with one db. > ordinal_schema > schema > AT_CHECK([ovsdb-tool create db1 schema], [0], [ignore], [ignore]) > @@ -477,6 +485,10 @@ AT_SKIP_IF([test "$IS_WIN32" = "yes"]) > # This test intentionally causes SIGSEGV, so make Address Sanitizer ignore it. > ASAN_OPTIONS=$ASAN_OPTIONS:handle_segv=0; export ASAN_OPTIONS > > +# Skip it if UB Sanitizer is being used. There's no way to disable the > +# SEGV check at runtime. > +AT_SKIP_IF([test $TESTS_WITH_UBSAN = yes]) > + > # Start ovsdb-server, initially with no remotes. > ordinal_schema > schema > AT_CHECK([ovsdb-tool create db schema], [0], [ignore], [ignore]) > @@ -512,6 +524,10 @@ AT_SKIP_IF([test "$IS_WIN32" = "yes"]) > # This test intentionally causes SIGSEGV, so make Address Sanitizer ignore it. > ASAN_OPTIONS=$ASAN_OPTIONS:handle_segv=0; export ASAN_OPTIONS > > +# Skip it if UB Sanitizer is being used. There's no way to disable the > +# SEGV check at runtime. > +AT_SKIP_IF([test $TESTS_WITH_UBSAN = yes]) > + > # Start ovsdb-server, initially with no remotes. > ordinal_schema > schema > AT_CHECK([ovsdb-tool create db schema], [0], [ignore], [ignore]) Jeff
On 1/24/22 17:57, Jeffrey Walton wrote: > On Mon, Jan 24, 2022 at 9:21 AM Dumitru Ceara <dceara@redhat.com> wrote: >> >> Note: There still is an UB instance when using SSE4.2 as reported here: >> https://mail.openvswitch.org/pipermail/ovs-dev/2022-January/390904.html >> >> Acked-by: Aaron Conole <aconole@redhat.com> >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> --- >> v3: >> - Fix typo in variable name. >> - Added SSE4.2 UB note to commit log. >> --- >> .ci/linux-build.sh | 6 ++++++ >> .github/workflows/build-and-test.yml | 5 +++++ >> configure.ac | 1 + >> tests/atlocal.in | 16 ++++++++++++++++ >> tests/automake.mk | 1 + >> tests/daemon.at | 8 ++++++++ >> tests/ovs-macros.at | 5 +++++ >> tests/ovsdb-server.at | 16 ++++++++++++++++ >> 8 files changed, 58 insertions(+) >> >> diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh >> index 6cd38ff3efb5..afd4379e6923 100755 >> --- a/.ci/linux-build.sh >> +++ b/.ci/linux-build.sh >> @@ -250,6 +250,12 @@ if [ "$ASAN" ]; then >> CFLAGS_FOR_OVS="${CFLAGS_FOR_OVS} ${CFLAGS_ASAN}" >> fi >> >> +if [ "$UBSAN" ]; then >> + # Use the default options configured in tests/atlocal.in, in UBSAN_OPTIONS. >> + CFLAGS_UBSAN="-O1 -fno-omit-frame-pointer -fno-common -fsanitize=undefined" >> + CFLAGS_FOR_OVS="${CFLAGS_FOR_OVS} ${CFLAGS_UBSAN}" >> +fi >> + >> save_OPTS="${OPTS} $*" >> OPTS="${EXTRA_OPTS} ${save_OPTS}" > > Sanitizer flags usually placed for compile and link. So they usually > go in CFLAGS and LDFLAGS. You will be Ok with putting them in CFLAGS > only if you use GNU Make's default: > > $ make -p | grep '^LINK\.c' > LINK.c = $(CC) $(CFLAGS) $(CPPFLAGS) $(LDFLAGS) $(TARGET_ARCH) > > But you will need to add them to LDFLAGS if you invoke LD directly. > This script is only used for building OVS in CI, it looks to me like it's going to pass the flags fine both at compile and link time: https://github.com/dceara/ovs/runs/4922391120?check_suite_focus=true#step:12:5116 > You might also want to add -fno-sanitize=integer-divide-by-zero > -fno-sanitize=float-divide-by-zero. Divide-by-0's are usually not > needed due to IEEE-754, and if left in usually cause more trouble then > they are worth. > Interesting, I see float-divide-by-zero is not even included in -fsanitize=undefined in more recent releases (e.g., clang 12). In GitHub actions we run on Ubuntu 18.04. I'll check to see what actually happens there. Thanks for pointing this out!
diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh index 6cd38ff3efb5..afd4379e6923 100755 --- a/.ci/linux-build.sh +++ b/.ci/linux-build.sh @@ -250,6 +250,12 @@ if [ "$ASAN" ]; then CFLAGS_FOR_OVS="${CFLAGS_FOR_OVS} ${CFLAGS_ASAN}" fi +if [ "$UBSAN" ]; then + # Use the default options configured in tests/atlocal.in, in UBSAN_OPTIONS. + CFLAGS_UBSAN="-O1 -fno-omit-frame-pointer -fno-common -fsanitize=undefined" + CFLAGS_FOR_OVS="${CFLAGS_FOR_OVS} ${CFLAGS_UBSAN}" +fi + save_OPTS="${OPTS} $*" OPTS="${EXTRA_OPTS} ${save_OPTS}" diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml index eac3504e48f8..9e3583781baa 100644 --- a/.github/workflows/build-and-test.yml +++ b/.github/workflows/build-and-test.yml @@ -13,6 +13,7 @@ jobs: linux-headers-$(uname -r) build-essential fakeroot devscripts equivs AFXDP: ${{ matrix.afxdp }} ASAN: ${{ matrix.asan }} + UBSAN: ${{ matrix.ubsan }} CC: ${{ matrix.compiler }} DEB_PACKAGE: ${{ matrix.deb_package }} DPDK: ${{ matrix.dpdk }} @@ -44,6 +45,10 @@ jobs: testsuite: test kernel: 3.16 asan: asan + - compiler: clang + testsuite: test + kernel: 3.16 + ubsan: ubsan - compiler: gcc testsuite: test diff --git a/configure.ac b/configure.ac index 298ea85aba22..198653a60528 100644 --- a/configure.ac +++ b/configure.ac @@ -197,6 +197,7 @@ OVS_CHECK_LINUX_SCTP_CT OVS_CHECK_LINUX_VIRTIO_TYPES OVS_CHECK_DPDK OVS_CHECK_PRAGMA_MESSAGE +AC_SUBST([CFLAGS]) AC_SUBST([OVS_CFLAGS]) AC_SUBST([OVS_LDFLAGS]) diff --git a/tests/atlocal.in b/tests/atlocal.in index a0ad239ecef2..142ea2090bc8 100644 --- a/tests/atlocal.in +++ b/tests/atlocal.in @@ -4,6 +4,7 @@ OPENSSL_SUPPORTS_SNI='@OPENSSL_SUPPORTS_SNI@' HAVE_UNBOUND='@HAVE_UNBOUND@' EGREP='@EGREP@' PYTHON3='@PYTHON3@' +CFLAGS='@CFLAGS@' # PYTHONCOERCECLOCALE=0 disables the Unicode compatibility warning on # stderr that breaks almost any Python3 test (PEP 0538) @@ -197,6 +198,16 @@ else DIFF_SUPPORTS_NORMAL_FORMAT=no fi +# Check whether UB Sanitizer is being used. +case "$CFLAGS" in +*fsanitize=undefined*) + TESTS_WITH_UBSAN=yes + ;; +*) + TESTS_WITH_UBSAN=no + ;; +esac + # Turn off proxies. unset http_proxy unset https_proxy @@ -222,3 +233,8 @@ export OVS_CTL_TIMEOUT # matter break everything. ASAN_OPTIONS=detect_leaks=0:abort_on_error=true:log_path=asan:$ASAN_OPTIONS export ASAN_OPTIONS + +# Add some default flags for UndefinedBehaviorSanitizer, if it was used +# for the build. +UBSAN_OPTIONS=print_stacktrace=1:halt_on_error=true:log_path=ubsan:$UBSAN_OPTIONS +export UBSAN_OPTIONS diff --git a/tests/automake.mk b/tests/automake.mk index 8a9151f81b86..a526be84b493 100644 --- a/tests/automake.mk +++ b/tests/automake.mk @@ -213,6 +213,7 @@ check-local: set $(SHELL) '$(TESTSUITE)' -C tests AUTOTEST_PATH=$(AUTOTEST_PATH); \ "$$@" $(TESTSUITEFLAGS) || \ (test -z "$$(find $(TESTSUITE_DIR) -name 'asan.*')" && \ + test -z "$$(find $(TESTSUITE_DIR) -name 'ubsan.*')" && \ test X'$(RECHECK)' = Xyes && "$$@" --recheck) # Python Coverage support. diff --git a/tests/daemon.at b/tests/daemon.at index 39d9aa391e9b..d7981f9d23a6 100644 --- a/tests/daemon.at +++ b/tests/daemon.at @@ -81,6 +81,10 @@ AT_SKIP_IF([test "$IS_WIN32" = "yes"]) # This test intentionally causes SIGSEGV, so make Address Sanitizer ignore it. ASAN_OPTIONS=$ASAN_OPTIONS:handle_segv=0; export ASAN_OPTIONS +# Skip it if UB Sanitizer is being used. There's no way to disable the +# SEGV check at runtime. +AT_SKIP_IF([test $TESTS_WITH_UBSAN = yes]) + # Start the daemon and wait for the pidfile to get created. on_exit 'kill $(cat *.pid)' AT_CHECK([ovsdb-server --monitor --pidfile --no-db 2>/dev/null & echo $!], @@ -149,6 +153,10 @@ AT_SKIP_IF([test "$IS_WIN32" = "yes"]) # This test intentionally causes SIGSEGV, so make Address Sanitizer ignore it. ASAN_OPTIONS=$ASAN_OPTIONS:handle_segv=0; export ASAN_OPTIONS +# Skip it if UB Sanitizer is being used. There's no way to disable the +# SEGV check at runtime. +AT_SKIP_IF([test $TESTS_WITH_UBSAN = yes]) + on_exit 'kill $(cat *.pid)' # Start the daemon and make sure that the pidfile exists immediately. diff --git a/tests/ovs-macros.at b/tests/ovs-macros.at index 66545da5728f..7899583f3c21 100644 --- a/tests/ovs-macros.at +++ b/tests/ovs-macros.at @@ -216,6 +216,11 @@ ovs_cleanup() { cat asan.* AT_FAIL_IF([:]) fi + if test "$(echo ubsan.*)" != 'ubsan.*'; then + echo "Undefined Behavior Sanitizer reported errors in:" ubsan.* + cat ubsan.* + AT_FAIL_IF([:]) + fi } ovs_wait () { diff --git a/tests/ovsdb-server.at b/tests/ovsdb-server.at index 2b742f78b079..1e00be60684b 100644 --- a/tests/ovsdb-server.at +++ b/tests/ovsdb-server.at @@ -292,6 +292,10 @@ AT_SKIP_IF([test "$IS_WIN32" = "yes"]) # This test intentionally causes SIGSEGV, so make Address Sanitizer ignore it. ASAN_OPTIONS=$ASAN_OPTIONS:handle_segv=0; export ASAN_OPTIONS +# Skip it if UB Sanitizer is being used. There's no way to disable the +# SEGV check at runtime. +AT_SKIP_IF([test $TESTS_WITH_UBSAN = yes]) + # Start ovsdb-server, initially with one db. ordinal_schema > schema AT_CHECK([ovsdb-tool create db1 schema], [0], [ignore], [ignore]) @@ -327,6 +331,10 @@ AT_SKIP_IF([test "$IS_WIN32" = "yes"]) # This test intentionally causes SIGSEGV, so make Address Sanitizer ignore it. ASAN_OPTIONS=$ASAN_OPTIONS:handle_segv=0; export ASAN_OPTIONS +# Skip it if UB Sanitizer is being used. There's no way to disable the +# SEGV check at runtime. +AT_SKIP_IF([test $TESTS_WITH_UBSAN = yes]) + # Start ovsdb-server, initially with one db. ordinal_schema > schema AT_CHECK([ovsdb-tool create db1 schema], [0], [ignore], [ignore]) @@ -477,6 +485,10 @@ AT_SKIP_IF([test "$IS_WIN32" = "yes"]) # This test intentionally causes SIGSEGV, so make Address Sanitizer ignore it. ASAN_OPTIONS=$ASAN_OPTIONS:handle_segv=0; export ASAN_OPTIONS +# Skip it if UB Sanitizer is being used. There's no way to disable the +# SEGV check at runtime. +AT_SKIP_IF([test $TESTS_WITH_UBSAN = yes]) + # Start ovsdb-server, initially with no remotes. ordinal_schema > schema AT_CHECK([ovsdb-tool create db schema], [0], [ignore], [ignore]) @@ -512,6 +524,10 @@ AT_SKIP_IF([test "$IS_WIN32" = "yes"]) # This test intentionally causes SIGSEGV, so make Address Sanitizer ignore it. ASAN_OPTIONS=$ASAN_OPTIONS:handle_segv=0; export ASAN_OPTIONS +# Skip it if UB Sanitizer is being used. There's no way to disable the +# SEGV check at runtime. +AT_SKIP_IF([test $TESTS_WITH_UBSAN = yes]) + # Start ovsdb-server, initially with no remotes. ordinal_schema > schema AT_CHECK([ovsdb-tool create db schema], [0], [ignore], [ignore])