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

[Tim Wiederhake]

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 at 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