Re: [libvirt PATCH v2 7/7] ci: Enable address and undefined behavior sanitizers

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(a)redhat.com&gt; > > > --- > > > .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.