[libvirt PATCH v2 0/7] 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. Changes since V1: Incorporated changes suggested by Pavel, except for #6 (now #7): The statement in https://listman.redhat.com/archives/libvir-list/2021-May/msg00070.html on the sanitizers working with Fedora 33 is wrong, I was fooled by caching. The bug described there is present in Fedora 33, 34, and Rawhide. Cheers, Tim Tim Wiederhake (7): meson: Allow larger stack frames when instrumenting meson: Allow undefined symbols when sanitizers are enabled tests: virfilemock: realpath: Allow non-null second parameter openvz: Add missing symbols to libvirt_openvz.syms 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 | 14 ++++++++++---- src/libvirt_openvz.syms | 2 ++ src/security/meson.build | 1 - tests/meson.build | 2 +- tests/virfilemock.c | 20 ++++++++++++-------- 7 files changed, 61 insertions(+), 15 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 | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/meson.build b/meson.build index 1f97842319..997a23f1b0 100644 --- a/meson.build +++ b/meson.build @@ -224,6 +224,9 @@ alloc_max = run_command( 'print(min(2**(@0@ * 8 - 1) - 1, 2**(@1@ * 8) - 1))'.format(ptrdiff_max, size_max), ) +# sanitizer instrumentation may enlarge stack frames +stack_frame_size = get_option('b_sanitize') == 'none' ? 4096 : 8192 + cc_flags += [ '-fasynchronous-unwind-tables', '-fexceptions', @@ -278,7 +281,7 @@ cc_flags += [ '-Wformat-y2k', '-Wformat-zero-length', '-Wframe-address', - '-Wframe-larger-than=4096', + '-Wframe-larger-than=@0@'.format(stack_frame_size), '-Wfree-nonheap-object', '-Whsa', '-Wif-not-aligned', -- 2.26.3
On Thu, May 06, 2021 at 05:08:32PM +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 | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
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 | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/meson.build b/meson.build index 997a23f1b0..02feb0e43d 100644 --- a/meson.build +++ b/meson.build @@ -491,9 +491,12 @@ libvirt_nodelete = cc.get_supported_link_arguments([ '-Wl,-z,nodelete', ]) -libvirt_no_undefined = cc.get_supported_link_arguments([ - '-Wl,-z,defs', -]) +libvirt_no_undefined = [] +if get_option('b_sanitize') == 'none' + libvirt_no_undefined += cc.get_supported_link_arguments([ + '-Wl,-z,defs', + ]) +endif libvirt_no_indirect = cc.get_supported_link_arguments([ '-Wl,--no-copy-dt-needed-entries', -- 2.26.3
On Thu, May 06, 2021 at 05:08:33PM +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 | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
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 | 20 ++++++++++++-------- 2 files changed, 13 insertions(+), 9 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..bf153ab769 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,20 @@ realpath(const char *path, char *resolved) if (getenv("LIBVIRT_MTAB")) { const char *p; - char *ret; - assert(resolved == NULL); - if ((p = STRSKIP(path, "/some/symlink"))) - ret = g_strdup_printf("/gluster%s", p); - else - ret = g_strdup(path); + if ((p = STRSKIP(path, "/some/symlink"))) { + if (resolved) + g_snprintf(resolved, PATH_MAX, "/gluster%s", p); + else + resolved = g_strdup_printf("/gluster%s", p); + } else { + if (resolved) + g_strlcpy(resolved, path, PATH_MAX); + else + resolved = g_strdup(path); + } - return ret; + return resolved; } return real_realpath(path, resolved); -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/libvirt_openvz.syms | 2 ++ 1 file changed, 2 insertions(+) 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: -- 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> --- tests/meson.build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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
"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> Reviewed-by: Pavel Hrdina <phrdina@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
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 89f618e678..4de4c30d7f 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -73,6 +73,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). @@ -521,6 +541,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 Thu, May 06, 2021 at 17:08:38 +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 89f618e678..4de4c30d7f 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -73,6 +73,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
Does this mean that if $TEMPORARILY_DISABLED is not passed then the sanitizer error causes a pipeline failure? If yes then I'd like to know how we are going to waive false-positives as modifying the code is the wrong action in such case.
On Thu, May 06, 2021 at 05:34:55PM +0200, Peter Krempa wrote:
On Thu, May 06, 2021 at 17:08:38 +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 89f618e678..4de4c30d7f 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -73,6 +73,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
Does this mean that if $TEMPORARILY_DISABLED is not passed then the sanitizer error causes a pipeline failure?
Yes, that's exactly what would happen.
If yes then I'd like to know how we are going to waive false-positives as modifying the code is the wrong action in such case.
I agree that sanitizers should probably not cause hard failures of the pipeline. On the other hand that's exactly what would happen with coverity which is also setup as a hard failure, so we kinda have a precedent. The question you need to answer for yourself is - if we set both coverity and sanitizer jobs to soft failures by default, how likely it is that someone is going to look at those failures and fix them in a timely manner? That's why the TEMPORARILY_DISABLED variable exists in the first place, if a failure occurs, someone has to look at the issue, determine that it's a false positive and unless we're immediately able to figure out how to alleviate the issue (e.g. adding a rule to coverity to ignore a certain false positive), we convert the job to a soft failing one. Once we addressed the problem in the sanitizer, we can again enable the job fully. Erik
On Fri, 2021-05-07 at 08:36 +0200, Erik Skultety wrote:
On Thu, May 06, 2021 at 05:34:55PM +0200, Peter Krempa wrote:
On Thu, May 06, 2021 at 17:08:38 +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 89f618e678..4de4c30d7f 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -73,6 +73,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
Does this mean that if $TEMPORARILY_DISABLED is not passed then the sanitizer error causes a pipeline failure?
Yes, that's exactly what would happen.
If yes then I'd like to know how we are going to waive false- positives as modifying the code is the wrong action in such case.
I agree that sanitizers should probably not cause hard failures of the pipeline.
AddressSanitizer finds issues like leaks, use-after-free, and double- free of memory. As there are currently none of these issues found, any new finding would be a regression which in my opinion does warrant a build failure. The sanitizer is not known to produce false positives, but should that situation arise in the future, we can use of suppression lists, see https://clang.llvm.org/docs/AddressSanitizer.html#issue-suppression.
On the other hand that's exactly what would happen with coverity which is also setup as a hard failure, so we kinda have a precedent. The question you need to answer for yourself is - if we set both coverity and sanitizer jobs to soft failures by default, how likely it is that someone is going to look at those failures and fix them in a timely manner? That's why the TEMPORARILY_DISABLED variable exists in the first place, if a failure occurs, someone has to look at the issue, determine that it's a false positive and unless we're immediately able to figure out how to alleviate the issue (e.g. adding a rule to coverity to ignore a certain false positive), we convert the job to a soft failing one. Once we addressed the problem in the sanitizer, we can again enable the job fully.
Erik
I agree. $TEMPORARILY_DISABLED is a rather big hammer though to disable sanitation entirely; I do not see a need for this but left it in for consistency with how other build jobs can be made non-required. Cheers, Tim
On Fri, May 07, 2021 at 09:10:44AM +0200, Tim Wiederhake wrote:
On Fri, 2021-05-07 at 08:36 +0200, Erik Skultety wrote:
On Thu, May 06, 2021 at 05:34:55PM +0200, Peter Krempa wrote:
On Thu, May 06, 2021 at 17:08:38 +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 89f618e678..4de4c30d7f 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -73,6 +73,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
Does this mean that if $TEMPORARILY_DISABLED is not passed then the sanitizer error causes a pipeline failure?
Yes, that's exactly what would happen.
If yes then I'd like to know how we are going to waive false- positives as modifying the code is the wrong action in such case.
I agree that sanitizers should probably not cause hard failures of the pipeline.
AddressSanitizer finds issues like leaks, use-after-free, and double- free of memory. As there are currently none of these issues found, any new finding would be a regression which in my opinion does warrant a build failure.
The sanitizer is not known to produce false positives, but should that situation arise in the future, we can use of suppression lists, see https://clang.llvm.org/docs/AddressSanitizer.html#issue-suppression.
On the other hand that's exactly what would happen with coverity which is also setup as a hard failure, so we kinda have a precedent. The question you need to answer for yourself is - if we set both coverity and sanitizer jobs to soft failures by default, how likely it is that someone is going to look at those failures and fix them in a timely manner? That's why the TEMPORARILY_DISABLED variable exists in the first place, if a failure occurs, someone has to look at the issue, determine that it's a false positive and unless we're immediately able to figure out how to alleviate the issue (e.g. adding a rule to coverity to ignore a certain false positive), we convert the job to a soft failing one. Once we addressed the problem in the sanitizer, we can again enable the job fully.
Erik
I agree. $TEMPORARILY_DISABLED is a rather big hammer though to disable sanitation entirely; I do not see a need for this but left it in for consistency with how other build jobs can be made non-required.
Arguably it may be a big hammer, but I actually don't object to having it in. Like I said, we already have a precedent plus the job spec you propose doesn't hinder readability in any way nor does it add any complexity. Regards, Erik
Ping. On Thu, 2021-05-06 at 17:08 +0200, Tim Wiederhake wrote:
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.
Changes since V1:
Incorporated changes suggested by Pavel, except for #6 (now #7): The statement in https://listman.redhat.com/archives/libvir-list/2021-May/msg00070.html on the sanitizers working with Fedora 33 is wrong, I was fooled by caching. The bug described there is present in Fedora 33, 34, and Rawhide.
Cheers, Tim
Tim Wiederhake (7): meson: Allow larger stack frames when instrumenting meson: Allow undefined symbols when sanitizers are enabled tests: virfilemock: realpath: Allow non-null second parameter openvz: Add missing symbols to libvirt_openvz.syms 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 | 14 ++++++++++---- src/libvirt_openvz.syms | 2 ++ src/security/meson.build | 1 - tests/meson.build | 2 +- tests/virfilemock.c | 20 ++++++++++++-------- 7 files changed, 61 insertions(+), 15 deletions(-)
-- 2.26.3
Ping On Tue, 2021-05-18 at 10:41 +0200, Tim Wiederhake wrote:
Ping.
On Thu, 2021-05-06 at 17:08 +0200, Tim Wiederhake wrote:
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.
Changes since V1:
Incorporated changes suggested by Pavel, except for #6 (now #7): The statement in https://listman.redhat.com/archives/libvir-list/2021-May/msg00070.html on the sanitizers working with Fedora 33 is wrong, I was fooled by caching. The bug described there is present in Fedora 33, 34, and Rawhide.
Cheers, Tim
Tim Wiederhake (7): meson: Allow larger stack frames when instrumenting meson: Allow undefined symbols when sanitizers are enabled tests: virfilemock: realpath: Allow non-null second parameter openvz: Add missing symbols to libvirt_openvz.syms 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 | 14 ++++++++++---- src/libvirt_openvz.syms | 2 ++ src/security/meson.build | 1 - tests/meson.build | 2 +- tests/virfilemock.c | 20 ++++++++++++-------- 7 files changed, 61 insertions(+), 15 deletions(-)
-- 2.26.3
On 5/6/21 5:08 PM, Tim Wiederhake wrote:
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.
Changes since V1:
Incorporated changes suggested by Pavel, except for #6 (now #7): The statement in https://listman.redhat.com/archives/libvir-list/2021-May/msg00070.html on the sanitizers working with Fedora 33 is wrong, I was fooled by caching. The bug described there is present in Fedora 33, 34, and Rawhide.
Cheers, Tim
Tim Wiederhake (7): meson: Allow larger stack frames when instrumenting meson: Allow undefined symbols when sanitizers are enabled tests: virfilemock: realpath: Allow non-null second parameter openvz: Add missing symbols to libvirt_openvz.syms 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 | 14 ++++++++++---- src/libvirt_openvz.syms | 2 ++ src/security/meson.build | 1 - tests/meson.build | 2 +- tests/virfilemock.c | 20 ++++++++++++-------- 7 files changed, 61 insertions(+), 15 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> and pushed. Michal
participants (5)
-
Erik Skultety -
Michal Prívozník -
Pavel Hrdina -
Peter Krempa -
Tim Wiederhake