[libvirt PATCH 0/6] Enable sanitizers
This series enables and adds AddressSanitizer and UndefinedBehaviorSanitizer builds to the CI. See: https://clang.llvm.org/docs/AddressSanitizer.html and https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html These sanitizers already found some issues in libvirt, e.g. 4eb7c621985dad4de911ec394ac628bd1a5b29ab, 1294de209cee6643511265c7e2d4283c047cf652, 8b8c91f487592c6c067847ca59dde405ca17573f, or 1c34211c22de28127a509edbf2cf2f44cb0d891e. There exist two more relevant sanitizers, ThreadSanitizer and MemorySanitizer. Unfortunately, those two require an instrumented build of all dependencies, including libc, to work correctly. Note that clang and gcc have different implementations of these sanitizers, hence the introduction of two new jobs to the CI. The latter one issues a warning about the use of LD_PRELOAD in `virTestMain`, which in this particular case can be safely ignored by setting `ASAN_OPTIONS` to verify_asan_link_order=0` for the gcc build. Cheers, Tim Tim Wiederhake (6): meson: Allow larger stack frames when instrumenting meson: Allow undefined symbols when sanitizers are enabled tests: virfilemock: realpath: Allow non-null second parameter tests: openvzutilstest: Remove duplicate linking with libvirt_openvz.a virt-aa-helper: Remove duplicate linking with src/datatypes.o ci: Enable address and undefined behavior sanitizers .gitlab-ci.yml | 35 +++++++++++++++++++++++++++++++++++ build-aux/syntax-check.mk | 2 +- meson.build | 8 +++++++- src/libvirt_openvz.syms | 2 ++ src/security/meson.build | 1 - tests/meson.build | 2 +- tests/virfilemock.c | 12 ++++++------ 7 files changed, 52 insertions(+), 10 deletions(-) -- 2.26.3
When enabling sanitizers, gcc adds some instrumentation to the code that may enlarge stack frames. Some function's stack frames are already close to the limit of 4096 and are enlarged past that threshold, e.g. virLXCProcessStart which reaches a frame size of 4624 bytes. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- meson.build | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/meson.build b/meson.build index 618cbd6b1d..bbdbe4afd8 100644 --- a/meson.build +++ b/meson.build @@ -278,7 +278,8 @@ cc_flags += [ '-Wformat-y2k', '-Wformat-zero-length', '-Wframe-address', - '-Wframe-larger-than=4096', + # sanitizer instrumentation may enlarge stack frames + '-Wframe-larger-than=@0@'.format(get_option('b_sanitize') in ['', 'none'] ? 4096 : 8192), '-Wfree-nonheap-object', '-Whsa', '-Wif-not-aligned', -- 2.26.3
On Mon, May 03, 2021 at 12:01:41PM +0200, Tim Wiederhake wrote:
When enabling sanitizers, gcc adds some instrumentation to the code that may enlarge stack frames. Some function's stack frames are already close to the limit of 4096 and are enlarged past that threshold, e.g. virLXCProcessStart which reaches a frame size of 4624 bytes.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- meson.build | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/meson.build b/meson.build index 618cbd6b1d..bbdbe4afd8 100644 --- a/meson.build +++ b/meson.build @@ -278,7 +278,8 @@ cc_flags += [ '-Wformat-y2k', '-Wformat-zero-length', '-Wframe-address', - '-Wframe-larger-than=4096', + # sanitizer instrumentation may enlarge stack frames + '-Wframe-larger-than=@0@'.format(get_option('b_sanitize') in ['', 'none'] ? 4096 : 8192), '-Wfree-nonheap-object', '-Whsa', '-Wif-not-aligned',
Looks good but needs some polishing. I would do something similar to what we do with -Walloc-size-larger-than: # sanitizer instrumentation may enlarge stack frames stack_frame_size = get_option('b_sanitize') == 'none' ? 4096 : 8192 cc_flags += [ ... '-Wframe-larger-than=@0@'.format(stack_frame_size), ... ] In addition there is no need to check for empty string as meson will handle that while parsing the command argument. All of the following commands: meson build -Db_sanitize= meson build -Db_sanitize="" meson build -Db_sanitize=" " meson build -Db_sanitize="asdf" result in this error (the value in the error message reflects the one actually used): meson.build:1:0: ERROR: Value "" (of type "string") for combo option "Code sanitizer to use" is not one of the choices. Possible choices are (as string): "none", "address", "thread", "undefined", "memory", "address,undefined". Pavel
When enabling sanitizers, clang adds some function symbols when instrumenting the code. The exact names of those functions are an implementation detail and should therefore not be added to any syms file. This patch prevents build failures due to those symbols not present in the syms file when building with sanitizers enabled. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- meson.build | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/meson.build b/meson.build index bbdbe4afd8..56c1294e7f 100644 --- a/meson.build +++ b/meson.build @@ -497,6 +497,11 @@ libvirt_no_indirect = cc.get_supported_link_arguments([ '-Wl,--no-copy-dt-needed-entries', ]) +if get_option('b_sanitize') not in [ '', 'none' ] + # sanitizers may add additional symbols + libvirt_no_undefined = [] +endif + if host_machine.system() == 'windows' version_script_flags = '-Wl,' else -- 2.26.3
On Mon, May 03, 2021 at 12:01:42PM +0200, Tim Wiederhake wrote:
When enabling sanitizers, clang adds some function symbols when instrumenting the code. The exact names of those functions are an implementation detail and should therefore not be added to any syms file. This patch prevents build failures due to those symbols not present in the syms file when building with sanitizers enabled.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- meson.build | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/meson.build b/meson.build index bbdbe4afd8..56c1294e7f 100644 --- a/meson.build +++ b/meson.build @@ -497,6 +497,11 @@ libvirt_no_indirect = cc.get_supported_link_arguments([ '-Wl,--no-copy-dt-needed-entries', ])
+if get_option('b_sanitize') not in [ '', 'none' ] + # sanitizers may add additional symbols + libvirt_no_undefined = [] +endif
Same as in the previous patch no need to check for empty string. Overwriting libvirt_no_undefined = [] at a different place then it is defined looks wrong to me. How about this: libvirt_no_undefined = [] if get_option('b_sanitize') == 'none': libvirt_no_undefined = cc.get_supported_link_arguments([ '-Wl,-z,defs', ]) endif This makes it cleaner IMHO as the complete logic is at a single place and also we will save one check for supported link arguments if we now that we don't want to use it. Pavel
if host_machine.system() == 'windows' version_script_flags = '-Wl,' else -- 2.26.3
When other preloaded libraries wrap and / or make calls to `realpath` (e.g. LLVM's AddessSanitizer), the second parameter is no longer guaranteed to be NULL. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- build-aux/syntax-check.mk | 2 +- tests/virfilemock.c | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index 552d639119..03b7599abc 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -1740,7 +1740,7 @@ exclude_file_name_regexp--sc_libvirt_unmarked_diagnostics = \ exclude_file_name_regexp--sc_po_check = ^(docs/|src/rpc/gendispatch\.pl$$|tests/commandtest.c$$) exclude_file_name_regexp--sc_prohibit_PATH_MAX = \ - ^build-aux/syntax-check\.mk$$ + ^(build-aux/syntax-check\.mk|tests/virfilemock.c)$$ exclude_file_name_regexp--sc_prohibit_access_xok = \ ^(src/util/virutil\.c)$$ diff --git a/tests/virfilemock.c b/tests/virfilemock.c index 7c9174bdd9..cba66feac0 100644 --- a/tests/virfilemock.c +++ b/tests/virfilemock.c @@ -24,7 +24,6 @@ #if WITH_LINUX_MAGIC_H # include <linux/magic.h> #endif -#include <assert.h> #include "virmock.h" #include "virstring.h" @@ -186,15 +185,16 @@ realpath(const char *path, char *resolved) if (getenv("LIBVIRT_MTAB")) { const char *p; - char *ret; - assert(resolved == NULL); + if (!resolved) + resolved = g_new0(char, PATH_MAX); + if ((p = STRSKIP(path, "/some/symlink"))) - ret = g_strdup_printf("/gluster%s", p); + g_snprintf(resolved, PATH_MAX, "/gluster%s", p); else - ret = g_strdup(path); + g_strlcpy(resolved, path, PATH_MAX); - return ret; + return resolved; } return real_realpath(path, resolved); -- 2.26.3
On Mon, May 03, 2021 at 12:01:43PM +0200, Tim Wiederhake wrote:
When other preloaded libraries wrap and / or make calls to `realpath` (e.g. LLVM's AddessSanitizer), the second parameter is no longer guaranteed to be NULL.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- build-aux/syntax-check.mk | 2 +- tests/virfilemock.c | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index 552d639119..03b7599abc 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -1740,7 +1740,7 @@ exclude_file_name_regexp--sc_libvirt_unmarked_diagnostics = \ exclude_file_name_regexp--sc_po_check = ^(docs/|src/rpc/gendispatch\.pl$$|tests/commandtest.c$$)
exclude_file_name_regexp--sc_prohibit_PATH_MAX = \ - ^build-aux/syntax-check\.mk$$ + ^(build-aux/syntax-check\.mk|tests/virfilemock.c)$$
exclude_file_name_regexp--sc_prohibit_access_xok = \ ^(src/util/virutil\.c)$$ diff --git a/tests/virfilemock.c b/tests/virfilemock.c index 7c9174bdd9..cba66feac0 100644 --- a/tests/virfilemock.c +++ b/tests/virfilemock.c @@ -24,7 +24,6 @@ #if WITH_LINUX_MAGIC_H # include <linux/magic.h> #endif -#include <assert.h>
#include "virmock.h" #include "virstring.h" @@ -186,15 +185,16 @@ realpath(const char *path, char *resolved)
if (getenv("LIBVIRT_MTAB")) { const char *p; - char *ret;
- assert(resolved == NULL); + if (!resolved) + resolved = g_new0(char, PATH_MAX); + if ((p = STRSKIP(path, "/some/symlink"))) - ret = g_strdup_printf("/gluster%s", p); + g_snprintf(resolved, PATH_MAX, "/gluster%s", p); else - ret = g_strdup(path); + g_strlcpy(resolved, path, PATH_MAX);
- return ret; + return resolved;
This seems way too complicated. It will be used only in tests where we know what paths we use. Also the manpage for realpath states the following: If resolved_path is specified as NULL, then realpath() uses malloc(3) to allocate a buffer of up to PATH_MAX bytes to hold the resolved pathname, and returns a pointer to this buffer. The caller should deallocate this buffer using free(3). where the important word part is "up to PATH_MAX". The code introduced by this patch always allocates PATH_MAX bytes. How about this which to me looks like a good enough solution for tests: if (getenv("LIBVIRT_MTAB")) { const char *p; if ((p = STRSKIP(path, "/some/symlink"))) resolved = g_strdup_printf("/gluster%s", p); else resolved = g_strdup(path); return resolved; } Pavel
}
return real_realpath(path, resolved); -- 2.26.3
"openvzutilstest" links, amongst others, against "libvirt_openvz.a" and "libvirt.so". The latter also links against "libvirt_openvz.a", leading to a One-Definition-Rule violation for "openvzLocateConfFile" in "openvz_conf.c". Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/libvirt_openvz.syms | 2 ++ tests/meson.build | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libvirt_openvz.syms b/src/libvirt_openvz.syms index ac0ed0d23e..a1038e51a4 100644 --- a/src/libvirt_openvz.syms +++ b/src/libvirt_openvz.syms @@ -3,10 +3,12 @@ # # openvz/openvz_conf.h +openvzCapsInit; openvzLocateConfFile; openvzReadConfigParam; openvzReadNetworkConf; openvzVEGetStringParam; +openvzXMLOption; # Let emacs know we want case-insensitive sorting # Local Variables: diff --git a/tests/meson.build b/tests/meson.build index 9900983d0c..3c73cbe3b5 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -430,7 +430,7 @@ endif if conf.has('WITH_OPENVZ') tests += [ - { 'name': 'openvzutilstest', 'link_with': [ openvz_lib ] }, + { 'name': 'openvzutilstest' }, ] endif -- 2.26.3
On Mon, May 03, 2021 at 12:01:44PM +0200, Tim Wiederhake wrote:
"openvzutilstest" links, amongst others, against "libvirt_openvz.a" and "libvirt.so". The latter also links against "libvirt_openvz.a", leading to a One-Definition-Rule violation for "openvzLocateConfFile" in "openvz_conf.c".
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/libvirt_openvz.syms | 2 ++ tests/meson.build | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/libvirt_openvz.syms b/src/libvirt_openvz.syms index ac0ed0d23e..a1038e51a4 100644 --- a/src/libvirt_openvz.syms +++ b/src/libvirt_openvz.syms @@ -3,10 +3,12 @@ #
# openvz/openvz_conf.h +openvzCapsInit; openvzLocateConfFile; openvzReadConfigParam; openvzReadNetworkConf; openvzVEGetStringParam; +openvzXMLOption;
This hunk should be in a separate patch as it fixes a different issue where we did not list all required symbols. Pavel
"virt-aa-helper" links, amongst others, against "datatypes.o" and "libvirt.so". The latter links against "libvirt_driver.a" which in turn also links against "datatypes.o", leading to a One-Definition-Rule violoation for "virConnectClass" et al. in "datatypes.c". Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/security/meson.build | 1 - 1 file changed, 1 deletion(-) diff --git a/src/security/meson.build b/src/security/meson.build index 416fec7900..6f5e1dec1d 100644 --- a/src/security/meson.build +++ b/src/security/meson.build @@ -41,7 +41,6 @@ if conf.has('WITH_LIBVIRTD') and conf.has('WITH_APPARMOR') 'name': 'virt-aa-helper', 'sources': [ apparmor_helper_sources, - datatypes_sources, dtrace_gen_objects, ], 'include': [ -- 2.26.3
On Mon, May 03, 2021 at 12:01:45PM +0200, Tim Wiederhake wrote:
"virt-aa-helper" links, amongst others, against "datatypes.o" and "libvirt.so". The latter links against "libvirt_driver.a" which in turn also links against "datatypes.o", leading to a One-Definition-Rule violoation for "virConnectClass" et al. in "datatypes.c".
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/security/meson.build | 1 - 1 file changed, 1 deletion(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
meson supports the following sanitizers: "address" (e.g. out-of-bounds memory access, use-after-free, etc.), "thread" (data races), "undefined" (e.g. signed integer overflow), and "memory" (use of uninitialized memory). Note that not all sanitizers are supported by all compilers, and that more sanitizers exist. Not all sanitizers can be enabled at the same time, but "address" and "undefined" can. Both thread and memory sanitizers require an instrumented build of all dependencies, including libc. gcc and clang use different implementations of these sanitizers and have proven to find different issues. Create CI jobs for both. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- .gitlab-ci.yml | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 8b7df68f47..aa537e65fb 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -71,6 +71,26 @@ stages: rpmbuild --nodeps -ta build/meson-dist/libvirt-*.tar.xz; fi +.sanitizer_build_job: + stage: builds + image: $CI_REGISTRY_IMAGE/ci-ubuntu-2004:latest + needs: + - x64-ubuntu-2004-container + rules: + - if: "$TEMPORARILY_DISABLED" + allow_failure: true + - when: on_success + cache: + paths: + - ccache/ + key: "$CI_JOB_NAME" + before_script: + - *script_variables + script: + - meson build --werror -Db_lundef=false -Db_sanitize="$SANITIZER" + - ninja -C build; + - ninja -C build test; + # Jobs that we delegate to Cirrus CI because they require an operating # system other than Linux. These jobs will only run if the required # setup has been performed on the GitLab account (see ci/README.rst). @@ -517,6 +537,21 @@ mingw64-fedora-rawhide: NAME: fedora-rawhide CROSS: mingw64 +# Sanitizers + +sanitize-gcc: + extends: .sanitizer_build_job + variables: + ASAN_OPTIONS: verify_asan_link_order=0 + CC: gcc + SANITIZER: address,undefined + +sanitize-clang: + extends: .sanitizer_build_job + variables: + CC: clang + SANITIZER: address,undefined + # This artifact published by this job is downloaded by libvirt.org to # be deployed to the web root: -- 2.26.3
On Mon, May 03, 2021 at 12:01:46PM +0200, Tim Wiederhake wrote:
meson supports the following sanitizers: "address" (e.g. out-of-bounds memory access, use-after-free, etc.), "thread" (data races), "undefined" (e.g. signed integer overflow), and "memory" (use of uninitialized memory). Note that not all sanitizers are supported by all compilers, and that more sanitizers exist.
Not all sanitizers can be enabled at the same time, but "address" and "undefined" can. Both thread and memory sanitizers require an instrumented build of all dependencies, including libc.
gcc and clang use different implementations of these sanitizers and have proven to find different issues. Create CI jobs for both.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- .gitlab-ci.yml | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 8b7df68f47..aa537e65fb 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -71,6 +71,26 @@ stages: rpmbuild --nodeps -ta build/meson-dist/libvirt-*.tar.xz; fi
+.sanitizer_build_job: + stage: builds + image: $CI_REGISTRY_IMAGE/ci-ubuntu-2004:latest + needs: + - x64-ubuntu-2004-container
Any specific reason for using Ubuntu 20.04? I would guess that we want to always use the latest compiler. It that's true I'm not sure which distribution to pick: - Fedora $LATEST - we would need to change it every 6 months - Fedora rawhide - may be unstable and randomly fail if the gcc/clang are broken Pavel
+ rules: + - if: "$TEMPORARILY_DISABLED" + allow_failure: true + - when: on_success + cache: + paths: + - ccache/ + key: "$CI_JOB_NAME" + before_script: + - *script_variables + script: + - meson build --werror -Db_lundef=false -Db_sanitize="$SANITIZER" + - ninja -C build; + - ninja -C build test; + # Jobs that we delegate to Cirrus CI because they require an operating # system other than Linux. These jobs will only run if the required # setup has been performed on the GitLab account (see ci/README.rst). @@ -517,6 +537,21 @@ mingw64-fedora-rawhide: NAME: fedora-rawhide CROSS: mingw64
+# Sanitizers + +sanitize-gcc: + extends: .sanitizer_build_job + variables: + ASAN_OPTIONS: verify_asan_link_order=0 + CC: gcc + SANITIZER: address,undefined + +sanitize-clang: + extends: .sanitizer_build_job + variables: + CC: clang + SANITIZER: address,undefined +
# This artifact published by this job is downloaded by libvirt.org to # be deployed to the web root: -- 2.26.3
On Mon, 2021-05-03 at 14:16 +0200, Pavel Hrdina wrote:
On Mon, May 03, 2021 at 12:01:46PM +0200, Tim Wiederhake wrote:
meson supports the following sanitizers: "address" (e.g. out-of- bounds memory access, use-after-free, etc.), "thread" (data races), "undefined" (e.g. signed integer overflow), and "memory" (use of uninitialized memory). Note that not all sanitizers are supported by all compilers, and that more sanitizers exist.
Not all sanitizers can be enabled at the same time, but "address" and "undefined" can. Both thread and memory sanitizers require an instrumented build of all dependencies, including libc.
gcc and clang use different implementations of these sanitizers and have proven to find different issues. Create CI jobs for both.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- .gitlab-ci.yml | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 8b7df68f47..aa537e65fb 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -71,6 +71,26 @@ stages: rpmbuild --nodeps -ta build/meson-dist/libvirt-*.tar.xz; fi
+.sanitizer_build_job: + stage: builds + image: $CI_REGISTRY_IMAGE/ci-ubuntu-2004:latest + needs: + - x64-ubuntu-2004-container
Any specific reason for using Ubuntu 20.04? I would guess that we want to always use the latest compiler. It that's true I'm not sure which distribution to pick:
- Fedora $LATEST - we would need to change it every 6 months
- Fedora rawhide - may be unstable and randomly fail if the gcc/clang are broken
I chose Ubuntu 20.04 for no particular reason other than "the image already has the sanitizers installed and need no change". Fedora 33 and Rawhide both require additional packages installed (libasan and libubsan). libvirt fails to build on Fedora Rawhide with gcc over what I believe to be a bug somewhere in the build chain. Output from one instance of the issue: ``` In file included from /usr/include/string.h:519, from ../source/src/internal.h:28, from ../source/src/rpc/virnettlscontext.h:23, from ../source/src/rpc/virnetclient.h:23, from ../source/src/rpc/virnetclient.c:27: In function ‘memcpy’, inlined from ‘virNetClientCallDispatchReply’ at ../source/src/rpc/virnetclient.c:1156:5, inlined from ‘virNetClientCallDispatch’ at ../source/src/rpc/virnetclient.c:1324:16, inlined from ‘virNetClientIOHandleInput’ at ../source/src/rpc/virnetclient.c:1497:23: /usr/include/bits/string_fortified.h:29:10: error: ‘__builtin_memcpy’ offset [148, 167] from the object at ‘client’ is out of the bounds of referenced subobject ‘prog’ with type ‘unsigned int’ at offset 144 [- Werror=array-bounds] 29 | return __builtin___memcpy_chk (__dest, __src, __len, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 30 | __glibc_objsize0 (__dest)); | ~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from ../source/src/rpc/virnetmessage.h:23, from ../source/src/rpc/virnetclient.h:24, from ../source/src/rpc/virnetclient.c:27: ../source/src/rpc/virnetclient.c: In function ‘virNetClientIOHandleInput’: src/rpc/virnetprotocol.h:48:15: note: subobject ‘prog’ declared here 48 | u_int prog; | ^~~~ cc1: all warnings being treated as errors ``` Notice that source and destination of the memcpy referenced above are of type `virNetMessage`: ``` memcpy(&thecall->msg->header, &client->msg.header, sizeof(client-
msg.header));
Using clang, libvirt builds just fine on Fedora Rawhide, but half of
the tests fail with:
==58582==ERROR: AddressSanitizer failed to allocate 0x0 (0) bytes of SetAlternateSignalStack (error code: 22) (...) ==58582==AddressSanitizer CHECK failed: ../lib/sanitizer_common/sanitizer_common.cpp:54 "((0 && "unable to mmap")) != (0)" (0x0, 0x0) <empty stack> ``` On Fedora 33 (with libasan and libubsan installed manually), libvirt builds and tests fine with both clang and gcc, but find no additional issues in the code base. I cannot make a statement on the differences between the versions of (clang's) libasan and libubsan found in Ubuntu 20.04 and Fedora, but https://github.com/llvm/llvm-project/commits/main/compiler-rt/lib/asan and https://github.com/llvm/llvm-project/commits/main/compiler-rt/lib/ubsan/ suggest that not too many new features are added to sanitizers lately. Cheers, Tim
+ rules: + - if: "$TEMPORARILY_DISABLED" + allow_failure: true + - when: on_success + cache: + paths: + - ccache/ + key: "$CI_JOB_NAME" + before_script: + - *script_variables + script: + - meson build --werror -Db_lundef=false -Db_sanitize="$SANITIZER" + - ninja -C build; + - ninja -C build test; + # Jobs that we delegate to Cirrus CI because they require an operating # system other than Linux. These jobs will only run if the required # setup has been performed on the GitLab account (see ci/README.rst). @@ -517,6 +537,21 @@ mingw64-fedora-rawhide: NAME: fedora-rawhide CROSS: mingw64
+# Sanitizers + +sanitize-gcc: + extends: .sanitizer_build_job + variables: + ASAN_OPTIONS: verify_asan_link_order=0 + CC: gcc + SANITIZER: address,undefined + +sanitize-clang: + extends: .sanitizer_build_job + variables: + CC: clang + SANITIZER: address,undefined +
# This artifact published by this job is downloaded by libvirt.org to # be deployed to the web root: -- 2.26.3
On Tue, May 04, 2021 at 12:50:09PM +0200, Tim Wiederhake wrote:
On Mon, 2021-05-03 at 14:16 +0200, Pavel Hrdina wrote:
On Mon, May 03, 2021 at 12:01:46PM +0200, Tim Wiederhake wrote:
meson supports the following sanitizers: "address" (e.g. out-of- bounds memory access, use-after-free, etc.), "thread" (data races), "undefined" (e.g. signed integer overflow), and "memory" (use of uninitialized memory). Note that not all sanitizers are supported by all compilers, and that more sanitizers exist.
Not all sanitizers can be enabled at the same time, but "address" and "undefined" can. Both thread and memory sanitizers require an instrumented build of all dependencies, including libc.
gcc and clang use different implementations of these sanitizers and have proven to find different issues. Create CI jobs for both.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- .gitlab-ci.yml | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 8b7df68f47..aa537e65fb 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -71,6 +71,26 @@ stages: rpmbuild --nodeps -ta build/meson-dist/libvirt-*.tar.xz; fi
+.sanitizer_build_job: + stage: builds + image: $CI_REGISTRY_IMAGE/ci-ubuntu-2004:latest + needs: + - x64-ubuntu-2004-container
Any specific reason for using Ubuntu 20.04? I would guess that we want to always use the latest compiler. It that's true I'm not sure which distribution to pick:
- Fedora $LATEST - we would need to change it every 6 months
- Fedora rawhide - may be unstable and randomly fail if the gcc/clang are broken
I chose Ubuntu 20.04 for no particular reason other than "the image already has the sanitizers installed and need no change".
Fedora 33 and Rawhide both require additional packages installed (libasan and libubsan).
We should just be adding those to the container - libvirt-ci already knows about them from QEMU.
libvirt fails to build on Fedora Rawhide with gcc over what I believe to be a bug somewhere in the build chain. Output from one instance of the issue:
``` In file included from /usr/include/string.h:519, from ../source/src/internal.h:28, from ../source/src/rpc/virnettlscontext.h:23, from ../source/src/rpc/virnetclient.h:23, from ../source/src/rpc/virnetclient.c:27: In function ‘memcpy’, inlined from ‘virNetClientCallDispatchReply’ at ../source/src/rpc/virnetclient.c:1156:5, inlined from ‘virNetClientCallDispatch’ at ../source/src/rpc/virnetclient.c:1324:16, inlined from ‘virNetClientIOHandleInput’ at ../source/src/rpc/virnetclient.c:1497:23: /usr/include/bits/string_fortified.h:29:10: error: ‘__builtin_memcpy’ offset [148, 167] from the object at ‘client’ is out of the bounds of referenced subobject ‘prog’ with type ‘unsigned int’ at offset 144 [- Werror=array-bounds] 29 | return __builtin___memcpy_chk (__dest, __src, __len, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 30 | __glibc_objsize0 (__dest)); | ~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from ../source/src/rpc/virnetmessage.h:23, from ../source/src/rpc/virnetclient.h:24, from ../source/src/rpc/virnetclient.c:27: ../source/src/rpc/virnetclient.c: In function ‘virNetClientIOHandleInput’: src/rpc/virnetprotocol.h:48:15: note: subobject ‘prog’ declared here 48 | u_int prog; | ^~~~ cc1: all warnings being treated as errors ```
Notice that source and destination of the memcpy referenced above are of type `virNetMessage`: ``` memcpy(&thecall->msg->header, &client->msg.header, sizeof(client-
msg.header));
Rawhide is often hairy as it ships pre-release GCC builds alot of time, so we often end up having to file compiler bugs for regresions. This isn't entirely bad though, because it is better to find and fix problems in rawhide, than wait until they hit stable Fedora or RHEL.
Using clang, libvirt builds just fine on Fedora Rawhide, but half of the tests fail with:
``` ==58582==ERROR: AddressSanitizer failed to allocate 0x0 (0) bytes of SetAlternateSignalStack (error code: 22) (...) ==58582==AddressSanitizer CHECK failed: ../lib/sanitizer_common/sanitizer_common.cpp:54 "((0 && "unable to mmap")) != (0)" (0x0, 0x0) <empty stack> ```
On Fedora 33 (with libasan and libubsan installed manually), libvirt builds and tests fine with both clang and gcc, but find no additional issues in the code base.
I cannot make a statement on the differences between the versions of (clang's) libasan and libubsan found in Ubuntu 20.04 and Fedora, but https://github.com/llvm/llvm-project/commits/main/compiler-rt/lib/asan and https://github.com/llvm/llvm-project/commits/main/compiler-rt/lib/ubsan/ suggest that not too many new features are added to sanitizers lately.
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (3)
-
Daniel P. Berrangé -
Pavel Hrdina -
Tim Wiederhake