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

[Erik Skultety]

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