Re: [PATCH v3] build: allow turning off --no-undefined and -z,defs
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 +, 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
Re: [PATCH v3] build: allow turning off --no-undefined and -z,defs
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)
Re: [PATCH] debuginfod: sqlite3_sharedprefix_fn should not compare past end of string
On Sat, Dec 04, 2021 at 05:54:07PM -0500, Frank Ch. Eigler wrote: > > gcc address sanitizer detected a read after the end of string in > > sqlite3_sharedprefix_fn. Make sure to stop comparing the strings when > > seeing the zero terminator. > > Yup, OK. Thanks, pushed. Mark
Re: PATCH: testsuite debuginfod
Hi Frank, On Tue, Oct 05, 2021 at 06:55:34PM +0200, Mark Wielaard wrote: > On Thu, 2021-09-30 at 10:50 -0400, Frank Ch. Eigler via Elfutils-devel > wrote: > > commit 85602ff68179053f19a2005df4fc653a69757584 (HEAD -> master) > > Author: Frank Ch. Eigler > > Date: Thu Sep 30 10:48:00 2021 -0400 > > > > debuginfod testsuite: Use ! CMD syntax. > > > > Previously, we had a mishmash of iffy && || constructs to > > reverse the rc of a subprocess that we expected to fail. > > Now use > >! CMD > > or > >! (CMD | CMD) > > more systematically where possible. > > > > Signed-off-by: Frank Ch. Eigler > > Thanks, the && || conditionals do make my head hurt (I am not as > logical as I like). This looks much simpler. > > I wish we can use this. But we use -e to make any failing command > (pipeline) fail the script/test. And man bash says (under -e Exit > immediately): > >The shell does not exit if the command that fails is part of the >command list immediately following a while or until keyword, part >of the test following the if or elif reserved words, part of any >command executed in a && or || list except the command following >the final && or ||, any command in a pipeline but the last, or if >the command's return value is being inverted with !. > > So I am afraid any '! should_fail_but_does_not' will not actually exit > and so doesn't FAIL the test. Could you double check that? Have you looked into this? Is it something we can workaround? Or should we drop this idea for now? Thanks, Mark
Re: [PATCH v2] libebl: recognize FDO Packaging Metadata ELF note
Hi Luca, On Tue, Nov 30, 2021 at 12:25:41PM +0100, Mark Wielaard wrote: > On Thu, 2021-11-25 at 17:02 +, Luca Boccassi via Elfutils-devel > wrote: > > +/* Packaging metadata as defined on > > > https://systemd.io/COREDUMP_PACKAGE_METADATA/ */ > > > +#define FDO_PACKAGING_METADATA 0xcafe1a7e > > > + > > > /* Note section name of program property. */ > > > #define NOTE_GNU_PROPERTY_SECTION_NAME ".note.gnu.property" > > > > I could move this definition to an internal header if the change to > > elf.h blocks this patch, if you prefer? Let me know. > > It looks like it will be integrated into glibc elf.h later this week. > I'll resync elf.h then and apply the other half of your patch. I haven't forgotten about this. The glibc elf.h change has been integrated now. But when I wanted to resync with the elfutils libelf/elf.h version I noticed something that look like ABI breakage: https://sourceware.org/pipermail/libc-alpha/2021-December/133589.html I am trying to get a response to that before syncing and integrating your patch. Sorry for the delay, Mark
Re: [PATCH] debuginfod/debuginfod-client.c: correct string format on 32bit arches with 64bit time_t
Hi, On Sun, Nov 21, 2021 at 11:14:11PM +0100, Alexander Kanavin via Elfutils-devel wrote: > From: Alexander Kanavin > > Use intmax_t to print time_t > > time_t is platform dependent and some of architectures e.g. > x32, riscv32, arc use 64bit time_t even while they are 32bit > architectures, therefore directly using integer printf formats will not > work portably, use intmax_t to typecast time_t into printf family of > functions. OK, so there were some questions about whether using intmax_t and %jd are portable enough, but I think it is clear that everything these days support that. So that is good. > musl 1.2.0 and above uses time64 on all platforms; glibc 2.34 has added > support for > time64 (which might be enabled by distro-wide CFLAGS), with possibly > 2.35 or 2.36 making it enabled by default. > > Use a plain int for scanning cache_config, as that's what the function > returns. So I think you are correct that printing/scanning/casting the time_t around is somewhat unfortunate because the size of time_t is not stable across architectures. Thanks for looking into this. I had no idea time_t was such a problematic data type. What makes it even more curious is that debuginfod_config_cache takes a long, not a time_t, while it is always called with a time_t argument. And then it returns an int... The result is interpreted as a negative errno number or is cast back to a time_t if it is positive. With this patch we write out the interval values "as big as possible" (intmax_t), but read it back in "as small as possible" (int). Would it make sense to also try to read it back in as intmax_t and then cast down to int? Or simply always cast down to int before writing it out, so reading and writing use the same data type? We seem to not expect these intervals to be much bigger than a week, so an int should always be big enough (even when stretched up to a whole year). Thanks, Mark
Re: [PATCH] debuginfod/debuginfod-client.c: correct string format on 32bit arches with 64bit time_t
I'm not sure; the point of this patch is simply to ensure debuginfod builds everywhere without errors. Making the types consistent is perhaps better done as a followup? Alex On Sun, 5 Dec 2021 at 21:31, Mark Wielaard wrote: > Hi, > > On Sun, Nov 21, 2021 at 11:14:11PM +0100, Alexander Kanavin via > Elfutils-devel wrote: > > From: Alexander Kanavin > > > > Use intmax_t to print time_t > > > > time_t is platform dependent and some of architectures e.g. > > x32, riscv32, arc use 64bit time_t even while they are 32bit > > architectures, therefore directly using integer printf formats will not > > work portably, use intmax_t to typecast time_t into printf family of > > functions. > > OK, so there were some questions about whether using intmax_t and %jd > are portable enough, but I think it is clear that everything these > days support that. So that is good. > > > musl 1.2.0 and above uses time64 on all platforms; glibc 2.34 has added > support for > > time64 (which might be enabled by distro-wide CFLAGS), with possibly > > 2.35 or 2.36 making it enabled by default. > > > > Use a plain int for scanning cache_config, as that's what the function > > returns. > > So I think you are correct that printing/scanning/casting the time_t > around is somewhat unfortunate because the size of time_t is not > stable across architectures. Thanks for looking into this. I had no > idea time_t was such a problematic data type. > > What makes it even more curious is that debuginfod_config_cache takes > a long, not a time_t, while it is always called with a time_t > argument. And then it returns an int... The result is interpreted as a > negative errno number or is cast back to a time_t if it is positive. > > With this patch we write out the interval values "as big as possible" > (intmax_t), but read it back in "as small as possible" (int). Would it > make sense to also try to read it back in as intmax_t and then cast > down to int? Or simply always cast down to int before writing it out, > so reading and writing use the same data type? > > We seem to not expect these intervals to be much bigger than a week, > so an int should always be big enough (even when stretched up to a > whole year). > > Thanks, > > Mark >
Re: [PATCH] debuginfod: Fix some memory leaks on debuginfod-client error paths.
On Wed, Dec 01, 2021 at 01:27:23PM +0100, Mark Wielaard wrote: > In a couple of places we might leak some memory when we encounter > an error. tmp_url might leak if realloc failed. escaped_string might > leak when setting up the data handle fails and we don't use it. > And one of the goto out1 should have been goto out2 to make sure > we release all allocated resources on exit (also updated a wrong > comment about that). Pushed, Mark
