[v3] build: allow turning off --no-undefined and -z,defs

  ASan, UBSan and MSan provided by clang aren't compatible with --no-undefined and -z,defs:
https://clang.llvm.org/docs/AddressSanitizer.html#usage
https://github.com/google/sanitizers/issues/380
so to build elfutils with clang with the sanitizers it should be possible
to turn them off.

It's implemented as a standalone option because there are places like
OSS-Fuzz for example where all the sanitizer flags are passed via CFLAGS
and CXXFLAGS: https://google.github.io/oss-fuzz/getting-started/new-project-guide/#Requirements
and it should be possible to just turn off --no-undefined and -z,defs
withut flipping other "configure" options and interfering with all
those fine-grained -fsanitize=... and -fno-sanitize-recover=... compiler flags.  Other than that, while
options like --enable-sanitize-undefined are helpful shortcuts, they
simply can't cover all the usecases and it's still necessary to pass
additional compiler flags to clang via CFLAGS to for example get around issues like
https://github.com/evverx/elfutils/issues/16 and
https://github.com/evverx/elfutils/issues/15.

Issues like https://bugs.llvm.org/show_bug.cgi?id=30333 have been
open since at least 2016 so it seems it's safe to say that it isn't
going to be fixed anytime soon. It's so ingrained that some build
systems complain when `-fsanitize=...` is passed to clang without
turning off no-undefined.

Without this patch something like

sed -i 's/^\(ZDEFS_LDFLAGS=\).*/\1/' configure.ac
find -name Makefile.am | xargs sed -i 's/,--no-undefined//'

should be used to make elfutils compile.

The patch was tested in https://github.com/evverx/elfutils/pull/24 by
compiling elfutils with both gcc and clang with and without ASan/UBsan
and running `make check && make distcheck`. --no-undefined and -z,defs
are still passed by default as expected.

Signed-off-by: Evgeny Vereshchagin <evvers@ya.ru>
---
 ChangeLog              |  5 +++++
 configure.ac           | 31 ++++++++++++++++++++++---------
 debuginfod/Makefile.am |  2 +-
 libasm/Makefile.am     |  2 +-
 libdw/Makefile.am      |  2 +-
 libelf/Makefile.am     |  2 +-
 6 files changed, 31 insertions(+), 13 deletions(-)
  

Hi Evgeny,

I really appreciate you helping us using more analyzers and fuzzers to
improve the code base. I think the address sanitizer already helped
improve the code. But it would help if you replied to the original
reviews and/or mentioned how the different versions of your patch have
changed since the last time. As far as I can see you only changed the
commit message a little this time.

On Fri, Dec 03, 2021 at 06:02:09PM +0000, Evgeny Vereshchagin wrote:
> Other than that, while options like --enable-sanitize-undefined are
> helpful shortcuts, they simply can't cover all the usecases and it's
> still necessary to pass additional compiler flags to clang via
> CFLAGS to for example get around issues like
> https://github.com/evverx/elfutils/issues/16 and
> https://github.com/evverx/elfutils/issues/15.

I really do believe that working from the now proposed
--enable-sanatize-address will be easiest to integrate address
sanitizer support. See how I used it to workaround isssues with the
gcc address sanitizer. You can use it likewise to work around issues
with clang. e.g. the configure check should detect the issue with
--no-undefined and could try if adding -lasan to LDFLAGS helps.

So the other isssues you are seeing with the clang address sanitizer
is "applying non-zero offset ... to null pointer" and "variable length
array bound evaluates to non-positive value 0". I am not sure how
those are actual problems. In the first case the calculated addresses
aren't actually used as (dereferenced) pointers, in the second the
array bound being zero also means the array content isn't used. Do you
know why these issues are flagged? Are there any extra ASAN_OPTIONS
set in these cases?

Cheers,

Mark
  

Hi Mark,

>  But it would help if you replied to the original
> reviews and/or mentioned how the different versions of your patch have
> changed since the last time.

I did but it looks like those emails didn't pass the spam filter. I'll try to figure out what happened there. Sorry about that!

>  As far as I can see you only changed the
> commit message a little this time.

That's correct. I tried to explain in the commit message why `--disable-undefined` is implemented as a standalone option.

> See how I used it to workaround isssues with the
> gcc address sanitizer. You can use it likewise to work around issues
> with clang. e.g. the configure check should detect the issue with
> --no-undefined and could try if adding -lasan to LDFLAGS helps

I saw that patch and I think it should make building elfutils with gcc and running the unit tests under ASan easier. Thanks! But it's based on the assumption that configure controls ASan flags and can change CFLAGS/LDFLAGS however it needs. Unfortunately I can't do that on OSS-Fuzz because all the sanitizer options are passed via CFLAGS there and I can't interfere with those CFLAGS. FWIW it isn't a theoretical issue because elfutils was integrated into OSS-Fuzz in https://github.com/google/oss-fuzz/pull/6944 and has been fuzzed there since then. And there elfutils is also built with MSan as well (which has never been implemented in gcc) and I'm not sure how additional configure options can cover that.

I agree that it would be great to make `--enable-sanitize-{undefined,address}` work with clang as well but I think it can be done later on top of `--disable-undefined`.

>  Do you
> know why these issues are flagged? Are there any extra ASAN_OPTIONS
> set in these cases?

No, there aren't. Those issues are flagged because -fsanitize=undefined in clang by default includes "pointer-overflow" and "vla-bound" (which as far as I know aren't available in gcc)
  

Hi Evgeny,

On Sun, 2021-12-05 at 19:52 +0300, Evgeny Vereshchagin wrote:
> > See how I used it to workaround isssues with the
> > gcc address sanitizer. You can use it likewise to work around
> > issues
> > with clang. e.g. the configure check should detect the issue with
> > --no-undefined and could try if adding -lasan to LDFLAGS helps
> 
> I saw that patch and I think it should make building elfutils with
> gcc and running the unit tests under ASan easier. Thanks! But it's
> based on the assumption that configure controls ASan flags and can
> change CFLAGS/LDFLAGS however it needs. Unfortunately I can't do that
> on OSS-Fuzz because all the sanitizer options are passed via CFLAGS
> there and I can't interfere with those CFLAGS.

But that doesn't really work if you use clang. It would actually work
as is if you used gcc. But I am not sure trying to use arbitrary
sanitizer flags that aren't tested in the upstream project is a good
idea.

I am not against OSS-Fuzz. I have had good experiences with using
fuzzers on the elfutils code base. But I find the project slightly
annoying. It requires a github and a google account and it hides the
results from the upstream project. Also the way they setup the fuzzers
feels odd (like how they try to cram everything through the CFLAGS and
how they try to link against a C++ library even for plain C projects).
I really would like to have any fuzzing targets be part of the upstream
project so we can all run the fuzzers instead of having to rely of
Google.

> I agree that it would be great to make `--enable-sanitize-
> {undefined,address}` work with clang as well but I think it can be
> done later on top of `--disable-undefined`.

I think it should be done as part of --enable-sanitize-address.

> >  Do you
> > know why these issues are flagged? Are there any extra ASAN_OPTIONS
> > set in these cases?
> 
> No, there aren't. Those issues are flagged because
> -fsanitize=undefined in clang by default includes "pointer-overflow"
> and "vla-bound" (which as far as I know aren't available in gcc)

But those seem to report bogus issues. At least in these cases, it
seems the code is fine.

Cheers,

Mark
  

Hi Mark,

> But that doesn't really work if you use clang.

It kind of does because almost everybody who builds their projects with clang sanitizers
turns off `z,defs` and `--no-undefined`. I agree it looks weird (and it's probably weird) but
it's just how it has been done for I don't know how many years. My understanding is
that it will never be fixed mostly because unlike gcc, clang doesn't support "shared" ASan/UBSan/MSan
(or, more precisely it isn't actively maintained there and it isn't used in general).

> sanitizer flags that aren't tested in the upstream project is a good
> idea.
> 

I wouldn't say that they are arbitrary. They have been tested with about 400 projects I think
and new flags are rolled out only if they don't break anything.

> It requires a github and a google account and it hides the
> results from the upstream project.

I don't think github accounts are required there but to due to some limitations gmail accounts
have to be used unfortunately. There is a flag there I can flip to prevent OSS-Fuzz from
restricting bug reports in the first place but I think I wrote it elsewhere already (after this email was sent)
and it'd probably make sense to keep discussing it there.

> Also the way they setup the fuzzers
> feels odd (like how they try to cram everything through the CFLAGS and
> how they try to link against a C++ library even for plain C projects).

They have to support a lot of different build systems there and I think it was decided that
CFLAGS is the only thing that they can use to affect them (which I think makes sense).
clang++ and stlibc++ have something to do with UBSan as far as I know.

> I really would like to have any fuzzing targets be part of the upstream
> project so we can all run the fuzzers instead of having to rely of
> Google.

I'm planning to move the fuzz targets upstream eventually and include them in the test suite
at least but I think they should be
compatible with OSS-Fuzz either way (because it's kind of hard to set up continuous
fuzzing manually)

>> I agree that it would be great to make `--enable-sanitize-
>> {undefined,address}` work with clang as well but I think it can be
>> done later on top of `--disable-undefined`.
> 
> I think it should be done as part of --enable-sanitize-address.

If --enable-sanitize-address controlled it, I'm not sure how I would build elfutils with Memory Sanitizer
(where I have to turn z,defs and no-undefined as well).

> But those seem to report bogus issues. At least in these cases, it
> seems the code is fine.

The rationale behind those checks (and why they are enabled by default) can be found
at https://reviews.llvm.org/D67122. My understanding is that code with that kind of UB
is known to be miscompiled from time to time.

Thanks,
Evgeny Vereshchagin