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