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

[Erik Skultety]

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

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