[PATCH] libdwfl: Don't try to convert too many bytes in dwfl_link_map_report
When trying to read (corrupt) phdrs from a core file we only want
to read and convert the bytes we could read. Also make sure we don't
try to allocate too big buffers.
https://sourceware.org/bugzilla/show_bug.cgi?id=28666
Signed-off-by: Mark Wielaard
---
libdwfl/ChangeLog | 6 ++
libdwfl/link_map.c | 17 +++--
2 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index 57b2c494..852b199a 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,3 +1,9 @@
+2021-12-08 Mark Wielaard
+
+ * link_map.c (dwfl_link_map_report): Limit malloc size to max
+ possible. When converting make sure we don't exceed the number
+ of bytes available in either in.d_buf nor out.d_buf.
+
2021-11-18 Matthias Maennich
* linux-kernel-modules.c (dwfl_linux_kernel_report_modules):
diff --git a/libdwfl/link_map.c b/libdwfl/link_map.c
index 1e7d4502..1c298a8e 100644
--- a/libdwfl/link_map.c
+++ b/libdwfl/link_map.c
@@ -847,6 +847,11 @@ dwfl_link_map_report (Dwfl *dwfl, const void *auxv, size_t
auxv_size,
/* Note this in the !in_ok path. That means memory_callback
failed. But the callback might still have reset the d_size
value (to zero). So explicitly set it here again. */
+ if (unlikely (phnum > SIZE_MAX / phent))
+ {
+ __libdwfl_seterrno (DWFL_E_NOMEM);
+ return false;
+ }
in.d_size = phnum * phent;
in.d_buf = malloc (in.d_size);
if (unlikely (in.d_buf == NULL))
@@ -876,6 +881,13 @@ dwfl_link_map_report (Dwfl *dwfl, const void *auxv, size_t
auxv_size,
return false;
}
size_t nbytes = phnum * phent;
+ /* We can only process as many bytes/phnum as there are
+in in.d_size. The data might have been truncated. */
+ if (nbytes > in.d_size)
+ {
+ nbytes = in.d_size;
+ phnum = nbytes / phent;
+ }
void *buf = malloc (nbytes);
Elf32_Phdr (*p32)[phnum] = buf;
Elf64_Phdr (*p64)[phnum] = buf;
@@ -888,10 +900,11 @@ dwfl_link_map_report (Dwfl *dwfl, const void *auxv,
size_t auxv_size,
{
.d_type = ELF_T_PHDR,
.d_version = EV_CURRENT,
- .d_size = phnum * phent,
+ .d_size = nbytes,
.d_buf = buf
};
- in.d_size = out.d_size;
+ if (in.d_size > out.d_size)
+ in.d_size = out.d_size;
if (likely ((elfclass == ELFCLASS32
? elf32_xlatetom : elf64_xlatetom)
(&out, &in, elfdata) != NULL))
--
2.18.4
[Bug libelf/28666] memmove() reads out-of-range in elf32_xlatetom
https://sourceware.org/bugzilla/show_bug.cgi?id=28666 --- Comment #3 from Mark Wielaard --- Proposed patch: https://patchwork.sourceware.org/project/elfutils/patch/20211208133606.7658-1-m...@klomp.org/ -- You are receiving this mail because: You are on the CC list for the bug.
Re: [PATCHv2] debuginfod: Check result of calling MHD_add_response_header.
Hi Frank, On Wed, 2021-12-01 at 10:23 -0500, Frank Ch. Eigler wrote: > > Although unlikely the MHD_add_response_header can fail for > > various reasons. If it fails something odd is going on. > > So check we can actually add a response header and log an > > error if we cannot. > > TBH I wouldn't bother even this much checking. It just uglifies the > code. If it would make covscan happier, I'd stick a (void) in front > of the add-header calls. That is really just like ignoring the issue imho. But you are right that it uglifies the code, I'll wrap the calls in an helper function. > > -MHD_add_response_header (r, "Content-Type", "text/plain"); > > -MHD_RESULT rc = MHD_queue_response (c, code, r); > > +MHD_RESULT rc1, rc2; > > +rc1 = MHD_add_response_header (r, "Content-Type", > > "text/plain"); > > +rc2 = MHD_queue_response (c, code, r); > > MHD_destroy_response (r); > > -return rc; > > +return (rc1 == MHD_NO || rc2 == MHD_NO) ? MHD_NO : MHD_YES; > > e.g. this part won't work: returning MHD_NO causes libmicrohttpd to > send a 503 error back to the caller, regardless of our intended one. OK, so we only want to report MHD_NO here when MHD_queue_response fails. > > +if (MHD_add_response_header (resp, "Last-Modified", > > datebuf) == MHD_NO) > > + if (verbose) > > +obatched(clog) << "Error: couldn't add Last-Modified > > header" > > + << endl; > > } > > e.g., we normally report errors to the logs, regardless of verbosity > settings. OK, I'll remove the if (verbose). > > + if (MHD_add_response_header (r, "Content-Type", > > + "application/octet-stream") == > > MHD_NO > > + || MHD_add_response_header (r, "X-DEBUGINFOD-SIZE", > > + to_string(s.st_size).c_str() > > ) == MHD_NO > > + || MHD_add_response_header (r, "X-DEBUGINFOD-FILE", > > + file.c_str()) == MHD_NO) > > e.g., this formulation makes it impossible to add some headers if a > previous one failed. It is likely that if one fails, then all others fail similarly, but I see your point. Any header is better than no headers at all. Thanks, Mark
[PATCHv3] debuginfod: Check result of calling MHD_add_response_header.
Although unlikely the MHD_add_response_header can fail for
various reasons. If it fails something odd is going on.
So check we can actually add a response header and log an
error if we cannot.
Signed-off-by: Mark Wielaard
---
debuginfod/ChangeLog | 12
debuginfod/debuginfod.cxx | 64 +++
2 files changed, 56 insertions(+), 20 deletions(-)
v3: Use a wrapper function, if there is an error, always log it
diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 8c937d66..8b5df4e1 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,15 @@
+2021-12-08 Mark Wielaard
+
+ * debuginfod.cxx (add_mhd_response_header): New function.
+ (reportable_exception::mhd_send_response): Call
+ MHD_add_response_header.
+ (add_mhd_last_modified): Likewise.
+ (handle_buildid_f_match): Likewise.
+ (handle_buildid_r_match): Likewise.
+ (handle_metrics): Likewise. And check MHD_Response was actually
+ created.
+ (handle_root): Likewise.
+
2021-12-04 Mark Wielaard
* debuginfod-client.c (debuginfod_query_server): Free winning_headers.
diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
index 0d3f0297..e38a551a 100644
--- a/debuginfod/debuginfod.cxx
+++ b/debuginfod/debuginfod.cxx
@@ -1,5 +1,6 @@
/* Debuginfo-over-http server.
Copyright (C) 2019-2021 Red Hat, Inc.
+ Copyright (C) 2021 Mark J. Wielaard
This file is part of elfutils.
This file is free software; you can redistribute it and/or modify
@@ -629,6 +630,9 @@ parse_opt (int key, char *arg,
+static void add_mhd_response_header (struct MHD_Response *r,
+const char *h, const char *v);
+
// represent errors that may get reported to an ostream and/or a libmicrohttpd
connection
struct reportable_exception
@@ -646,7 +650,7 @@ struct reportable_exception
MHD_Response* r = MHD_create_response_from_buffer (message.size(),
(void*) message.c_str(),
MHD_RESPMEM_MUST_COPY);
-MHD_add_response_header (r, "Content-Type", "text/plain");
+add_mhd_response_header (r, "Content-Type", "text/plain");
MHD_RESULT rc = MHD_queue_response (c, code, r);
MHD_destroy_response (r);
return rc;
@@ -1067,6 +1071,15 @@ conninfo (struct MHD_Connection * conn)
+/* Wrapper for MHD_add_response_header that logs an error if we
+ couldn't add the specified header. */
+static void
+add_mhd_response_header (struct MHD_Response *r,
+const char *h, const char *v)
+{
+ if (MHD_add_response_header (r, h, v) == MHD_NO)
+obatched(clog) << "Error: couldn't add '" << h << "' header" << endl;
+}
static void
add_mhd_last_modified (struct MHD_Response *resp, time_t mtime)
@@ -1079,10 +1092,10 @@ add_mhd_last_modified (struct MHD_Response *resp,
time_t mtime)
size_t rc = strftime (datebuf, sizeof (datebuf), "%a, %d %b %Y %T GMT",
nowp);
if (rc > 0 && rc < sizeof (datebuf))
-(void) MHD_add_response_header (resp, "Last-Modified", datebuf);
+add_mhd_response_header (resp, "Last-Modified", datebuf);
}
- (void) MHD_add_response_header (resp, "Cache-Control", "public");
+ add_mhd_response_header (resp, "Cache-Control", "public");
}
@@ -1128,10 +1141,11 @@ handle_buildid_f_match (bool internal_req_t,
}
else
{
- MHD_add_response_header (r, "Content-Type", "application/octet-stream");
std::string file = b_source0.substr(b_source0.find_last_of("/")+1,
b_source0.length());
- MHD_add_response_header (r, "X-DEBUGINFOD-SIZE",
to_string(s.st_size).c_str() );
- MHD_add_response_header (r, "X-DEBUGINFOD-FILE", file.c_str() );
+ add_mhd_response_header (r, "Content-Type", "application/octet-stream");
+ add_mhd_response_header (r, "X-DEBUGINFOD-SIZE",
+ to_string(s.st_size).c_str());
+ add_mhd_response_header (r, "X-DEBUGINFOD-FILE", file.c_str());
add_mhd_last_modified (r, s.st_mtime);
if (verbose > 1)
obatched(clog) << "serving file " << b_source0 << endl;
@@ -1600,10 +1614,11 @@ handle_buildid_r_match (bool internal_req_p,
inc_metric ("http_responses_total","result","archive fdcache");
- MHD_add_response_header (r, "Content-Type", "application/octet-stream");
- MHD_add_response_header (r, "X-DEBUGINFOD-SIZE",
to_string(fs.st_size).c_str());
- MHD_add_response_header (r, "X-DEBUGINFOD-ARCHIVE", b_source0.c_str());
- MHD_add_response_header (r, "X-DEBUGINFOD-FILE", b_source1.c_str());
+ add_mhd_response_header (r, "Content-Type", "application/octet-stream");
+ add_mhd_response_header (r, "X
Re: [PATCH v3] build: allow turning off --no-undefined and -z,defs
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
Re: [PATCH] debuginfod/debuginfod-client.c: correct string format on 32bit arches with 64bit time_t
Hi Alexander, On Sun, 2021-12-05 at 21:45 +0100, Alexander Kanavin wrote: > 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? I think the issue of the code not compiling in some environments is because of the inconsistent use of types. So I rather fix the build errors by fixing the used types than to simply cast the errors away. Cheers, Mark
Re: [PATCHv3] debuginfod: Check result of calling MHD_add_response_header.
Hi -
> Although unlikely the MHD_add_response_header can fail for
> various reasons. If it fails something odd is going on.
> So check we can actually add a response header and log an
> error if we cannot.
Sure, if you insist. :-)
except:
> - *size = os.size();
> - MHD_add_response_header (r, "Content-Type", "text/plain");
> + if (r != NULL)
> +{
> + *size = os.size();
> + add_mhd_response_header (r, "Content-Type", "text/plain");
> +}
Why move the *size assignment in there?
> - *size = version.size ();
> - MHD_add_response_header (r, "Content-Type", "text/plain");
> + if (r != NULL)
> +{
> + *size = version.size ();
> + add_mhd_response_header (r, "Content-Type", "text/plain");
> +}
>return r;
> }
ditto?
- FChE
Re: [PATCHv3] debuginfod: Check result of calling MHD_add_response_header.
Hi Frank,
On Wed, 2021-12-08 at 10:32 -0500, Frank Ch. Eigler wrote:
> except:
>
> > - *size = os.size();
> > - MHD_add_response_header (r, "Content-Type", "text/plain");
> > + if (r != NULL)
> > +{
> > + *size = os.size();
> > + add_mhd_response_header (r, "Content-Type", "text/plain");
> > +}
>
> Why move the *size assignment in there?
>
>
> > - *size = version.size ();
> > - MHD_add_response_header (r, "Content-Type", "text/plain");
> > + if (r != NULL)
> > +{
> > + *size = version.size ();
> > + add_mhd_response_header (r, "Content-Type", "text/plain");
> > +}
> >return r;
> > }
>
> ditto?
Because both statements are unnecessary if r == NULL (aka the response
couldn't even be created). Filling in size would also be lying to the
caller (who has to check the returned response isn't NULL anyway - and
does, I just checked :)
But it is also harmless to do, so if you want I can move it outside the
if statement.
Cheers,
Mark
Re: [PATCHv3] debuginfod: Check result of calling MHD_add_response_header.
Hi - > > Why move the *size assignment in there? > > Because both statements are unnecessary if r == NULL (aka the response > couldn't even be created). [...] > But it is also harmless to do, so if you want I can move it outside the > if statement. OK, whichever, doesn't much matter then. - FChE
obv patch: debuginfod concurrency fix
Hi - Committing as obvious. Author: Frank Ch. Eigler Date: Wed Dec 8 10:20:58 2021 -0500 debuginfod: correct concurrency bug in fdcache metrics The intern() function called set_metrics() outside a necessary lock being held. helgrind identified this race condition. No QA impact. Signed-off-by: Frank Ch. Eigler diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog index 8c937d6629a3..8cbaa9aa6fd9 100644 --- a/debuginfod/ChangeLog +++ b/debuginfod/ChangeLog @@ -1,3 +1,7 @@ +2021-12-08 Frank Ch. Eigler + + * debuginfod.cxx (intern): Call set_metrics() holding the fdcache mutex. + 2021-12-04 Mark Wielaard * debuginfod-client.c (debuginfod_query_server): Free winning_headers. diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx index 0d3f02978ee2..a26e7e8fce37 100644 --- a/debuginfod/debuginfod.cxx +++ b/debuginfod/debuginfod.cxx @@ -1354,8 +1354,8 @@ class libarchive_fdcache if (verbose > 3) obatched(clog) << "fdcache interned a=" << a << " b=" << b << " fd=" << fd << " mb=" << mb << " front=" << front_p << endl; + set_metrics(); } -set_metrics(); // NB: we age the cache at lookup time too if (statfs_free_enough_p(tmpdir, "tmpdir", fdcache_mintmp))
[COMMITTED] libdwfl: Don't read beyond end of file in dwfl_segment_report_module
The ELF might not be fully mapped into memory (which probably means the phdrs are bogus). Don't try to read beyond what we have in memory already. Reported-by: Evgeny Vereshchagin Signed-off-by: Mark Wielaard --- libdwfl/ChangeLog| 5 + libdwfl/dwfl_segment_report_module.c | 6 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog index 57b2c494..b2a8752a 100644 --- a/libdwfl/ChangeLog +++ b/libdwfl/ChangeLog @@ -1,3 +1,8 @@ +2021-12-08 Mark Wielaard + + * dwfl_segment_report_module.c (dwfl_segment_report_module): Don't + read beyond of (actual) end of (memory) file. + 2021-11-18 Matthias Maennich * linux-kernel-modules.c (dwfl_linux_kernel_report_modules): diff --git a/libdwfl/dwfl_segment_report_module.c b/libdwfl/dwfl_segment_report_module.c index ee9cfa2e..f6a1799e 100644 --- a/libdwfl/dwfl_segment_report_module.c +++ b/libdwfl/dwfl_segment_report_module.c @@ -924,8 +924,12 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, GElf_Off offset = is32 ? p32[i].p_offset : p64[i].p_offset; GElf_Xword filesz = is32 ? p32[i].p_filesz : p64[i].p_filesz; + /* Don't try to read beyond the actual end of file. */ + if (offset >= file_trimmed_end) +continue; + void *into = contents + offset; - size_t read_size = filesz; + size_t read_size = MIN (filesz, file_trimmed_end - offset); (*memory_callback) (dwfl, addr_segndx (dwfl, segment, vaddr + bias, false), &into, &read_size, vaddr + bias, read_size, -- 2.18.4
[PATCH] libdwfl: Add overflow check while iterating in dwfl_segment_report_module
While iterating the notes we could overflow the len variable if the
note name or description was too big. Fix this by adding an (unsigned)
overflow check.
https://sourceware.org/bugzilla/show_bug.cgi?id=28654
Signed-off-by: Mark Wielaard
---
libdwfl/ChangeLog| 5 +
libdwfl/dwfl_segment_report_module.c | 6 +-
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index 520405c8..e1cd70fa 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,3 +1,8 @@
+2021-12-08 Mark Wielaard
+
+ * dwfl_segment_report_module.c (dwfl_segment_report_module): Add
+ len overflow check while iterating notes.
+
2021-12-08 Mark Wielaard
* dwfl_segment_report_module.c (dwfl_segment_report_module): Don't
diff --git a/libdwfl/dwfl_segment_report_module.c
b/libdwfl/dwfl_segment_report_module.c
index f6a1799e..574f02a7 100644
--- a/libdwfl/dwfl_segment_report_module.c
+++ b/libdwfl/dwfl_segment_report_module.c
@@ -543,10 +543,12 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const
char *name,
const GElf_Nhdr *nh = notes;
size_t len = 0;
+ size_t last_len;
while (filesz > len + sizeof (*nh))
{
const void *note_name;
const void *note_desc;
+ last_len = len;
len += sizeof (*nh);
note_name = notes + len;
@@ -555,7 +557,9 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char
*name,
len = align == 8 ? NOTE_ALIGN8 (len) : NOTE_ALIGN4 (len);
note_desc = notes + len;
- if (unlikely (filesz < len + nh->n_descsz))
+ if (unlikely (filesz < len + nh->n_descsz
+|| len < last_len
+|| len + nh->n_descsz < last_len))
break;
if (nh->n_type == NT_GNU_BUILD_ID
--
2.18.4
[Bug libdw/28654] There seems to be an infinite loop somewhere in dwfl_segment_report_module
https://sourceware.org/bugzilla/show_bug.cgi?id=28654 Mark Wielaard changed: What|Removed |Added Last reconfirmed||2021-12-08 Status|UNCONFIRMED |ASSIGNED Ever confirmed|0 |1 Assignee|unassigned at sourceware dot org |mark at klomp dot org CC||mark at klomp dot org --- Comment #1 from Mark Wielaard --- Proposed fix: https://sourceware.org/pipermail/elfutils-devel/2021q4/004463.html -- You are receiving this mail because: You are on the CC list for the bug.
Buildbot failure in Wildebeest Builder on whole buildset
The Buildbot has detected a new failure on builder elfutils-centos-x86_64 while building elfutils. Full details are available at: https://builder.wildebeest.org/buildbot/#builders/1/builds/868 Buildbot URL: https://builder.wildebeest.org/buildbot/ Worker for this Build: centos-x86_64 Build Reason: Blamelist: Mark Wielaard BUILD FAILED: failed test (failure) Sincerely, -The Buildbot
[Bug libelf/28666] memmove() reads out-of-range in elf32_xlatetom
https://sourceware.org/bugzilla/show_bug.cgi?id=28666 --- Comment #4 from Evgeny Vereshchagin --- With that patch applied I can confirm that the issue is gone.Just to make sure also run the unit tests on aarch64, i386, ppc64le and x86_64 and they all passed there. Thanks! -- You are receiving this mail because: You are on the CC list for the bug.
[Bug libdw/28654] There seems to be an infinite loop somewhere in dwfl_segment_report_module
https://sourceware.org/bugzilla/show_bug.cgi?id=28654 --- Comment #2 from Evgeny Vereshchagin --- I applied the patch on top of the master branch with the other two patches related to libwfl applied and ran `src/stack` under Valgrind. I also ran the unit tests on four different architectures just in case and they all passed there. Looks like the issue is gone. Thanks! -- You are receiving this mail because: You are on the CC list for the bug.
Re: [PATCH v3] build: allow turning off --no-undefined and -z,defs
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
[PATCH] libdwfl: Make sure we know the phdr entry size before searching phdrs.
Without the program header entry size we cannot search through the
phdrs.
https://sourceware.org/bugzilla/show_bug.cgi?id=28657
Signed-off-by: Mark Wielaard
---
libdwfl/ChangeLog | 4
libdwfl/link_map.c | 2 +-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index 852b199a..8dd595f8 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,3 +1,7 @@
+2021-12-08 Mark Wielaard
+
+ * link_map.c (dwfl_link_map_report): Make sure phent != 0.
+
2021-12-08 Mark Wielaard
* link_map.c (dwfl_link_map_report): Limit malloc size to max
diff --git a/libdwfl/link_map.c b/libdwfl/link_map.c
index 1c298a8e..623b3062 100644
--- a/libdwfl/link_map.c
+++ b/libdwfl/link_map.c
@@ -784,7 +784,7 @@ dwfl_link_map_report (Dwfl *dwfl, const void *auxv, size_t
auxv_size,
GElf_Xword dyn_filesz = 0;
GElf_Addr dyn_bias = (GElf_Addr) -1;
- if (phdr != 0 && phnum != 0)
+ if (phdr != 0 && phnum != 0 && phent != 0)
{
Dwfl_Module *phdr_mod;
int phdr_segndx = INTUSE(dwfl_addrsegment) (dwfl, phdr, &phdr_mod);
--
2.30.2
[Bug libdw/28657] UBSan seems to report a divison-by-zero in dwfl_link_map_report
https://sourceware.org/bugzilla/show_bug.cgi?id=28657 Mark Wielaard changed: What|Removed |Added CC||mark at klomp dot org Ever confirmed|0 |1 Last reconfirmed||2021-12-08 Status|UNCONFIRMED |ASSIGNED Assignee|unassigned at sourceware dot org |mark at klomp dot org --- Comment #2 from Mark Wielaard --- Proposed patch: https://sourceware.org/pipermail/elfutils-devel/2021q4/004469.html -- You are receiving this mail because: You are on the CC list for the bug.
[Bug libdw/28660] ASan seems to complain about a "heap-buffer-overflow"
https://sourceware.org/bugzilla/show_bug.cgi?id=28660 Mark Wielaard changed: What|Removed |Added CC||mark at klomp dot org --- Comment #1 from Mark Wielaard --- Valgrind also complains about this. But this seems to be resolved by the proposed patch for PR28657. https://sourceware.org/bugzilla/show_bug.cgi?id=28657 Which makes sense since we aren't trying to deal with a zero phdr size. -- You are receiving this mail because: You are on the CC list for the bug.
[Bug libdw/28660] ASan seems to complain about a "heap-buffer-overflow"
https://sourceware.org/bugzilla/show_bug.cgi?id=28660 --- Comment #2 from Evgeny Vereshchagin --- As far as I can see both issues are gone with that patch applied. Thanks! -- You are receiving this mail because: You are on the CC list for the bug.
[Bug libelf/28666] memmove() reads out-of-range in elf32_xlatetom
https://sourceware.org/bugzilla/show_bug.cgi?id=28666 --- Comment #5 from Evgeny Vereshchagin --- I was able to trigger the same issue with a different file by running the fuzz target a bit longer. I'll double check and attach the file -- You are receiving this mail because: You are on the CC list for the bug.
[Bug libelf/28666] memmove() reads out-of-range in elf32_xlatetom
https://sourceware.org/bugzilla/show_bug.cgi?id=28666 --- Comment #6 from Evgeny Vereshchagin --- My bad. The backtrace is different there: ``` 2021-12-08T20:14:08.7167911Z ==21==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7f4f1d328000 at pc 0x00524c9f bp 0x7fff9271bc40 sp 0x7fff9271b408 2021-12-08T20:14:08.7169143Z READ of size 327680 at 0x7f4f1d328000 thread T0 2021-12-08T20:14:08.7170393Z SCARINESS: 26 (multi-byte-read-heap-buffer-overflow) 2021-12-08T20:14:08.7429032Z #0 0x524c9e in __asan_memmove /src/llvm-project/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:30:3 2021-12-08T20:14:08.7434627Z #1 0x63d0b3 in memmove /usr/include/x86_64-linux-gnu/bits/string_fortified.h:40:10 2021-12-08T20:14:08.7438180Z #2 0x63d0b3 in elf32_xlatetom /src/elfutils/libelf/elf32_xlatetom.c:96:2 2021-12-08T20:14:08.7441601Z #3 0x5fc7e8 in dwfl_link_map_report /src/elfutils/libdwfl/link_map.c:1013:12 2021-12-08T20:14:08.7445515Z #4 0x5668da in dwfl_core_file_report /src/elfutils/libdwfl/core-file.c:548:16 2021-12-08T20:14:08.7448767Z #5 0x55eaa0 in LLVMFuzzerTestOneInput /src/fuzz-dwfl-core.c:52:6 ``` I think the issue reported here is gone. Thanks! -- You are receiving this mail because: You are on the CC list for the bug.
[PATCH] libdwfl: Don't trust e_shentsize in dwfl_segment_report_module
When calulating the possible section header table end us the actual size of the section headers (sizeof (Elf32_Shdr) or sizeof (Elf64_Shdr)), not the ELF header e_shentsize value, which can be corrupted. This prevents a posssible overflow, but we check the shdrs_end is sane later anyway. https://sourceware.org/bugzilla/show_bug.cgi?id=28659 Signed-off-by: Mark Wielaard --- libdwfl/ChangeLog| 5 + libdwfl/dwfl_segment_report_module.c | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog index 8dd595f8..5e25c1b0 100644 --- a/libdwfl/ChangeLog +++ b/libdwfl/ChangeLog @@ -1,3 +1,8 @@ +2021-12-08 Mark Wielaard + + * dwfl_segment_report_module.c (dwfl_segment_report_module): Don't + trust e_shentsize. + 2021-12-08 Mark Wielaard * link_map.c (dwfl_link_map_report): Make sure phent != 0. diff --git a/libdwfl/dwfl_segment_report_module.c b/libdwfl/dwfl_segment_report_module.c index ee9cfa2e..5efc9d6a 100644 --- a/libdwfl/dwfl_segment_report_module.c +++ b/libdwfl/dwfl_segment_report_module.c @@ -383,7 +383,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, zero sh_size field. We ignore this here because getting shdrs is just a nice bonus (see below in the type == PT_LOAD case where we trim the last segment). */ - shdrs_end = ehdr.e32.e_shoff + ehdr.e32.e_shnum * ehdr.e32.e_shentsize; + shdrs_end = ehdr.e32.e_shoff + ehdr.e32.e_shnum * sizeof (Elf32_Shdr); break; case ELFCLASS64: @@ -397,7 +397,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, if (phentsize != sizeof (Elf64_Phdr)) goto out; /* See the NOTE above for shdrs_end and ehdr.e32.e_shnum. */ - shdrs_end = ehdr.e64.e_shoff + ehdr.e64.e_shnum * ehdr.e64.e_shentsize; + shdrs_end = ehdr.e64.e_shoff + ehdr.e64.e_shnum * sizeof (Elf64_Shdr); break; default: -- 2.30.2
[Bug libdw/28659] UBSan seems to complain about an "integer overflow" in dwfl_segment_report_module
https://sourceware.org/bugzilla/show_bug.cgi?id=28659 Mark Wielaard changed: What|Removed |Added Status|UNCONFIRMED |ASSIGNED Assignee|unassigned at sourceware dot org |mark at klomp dot org Ever confirmed|0 |1 Last reconfirmed||2021-12-08 CC||mark at klomp dot org --- Comment #1 from Mark Wielaard --- Proposed patch: https://sourceware.org/pipermail/elfutils-devel/2021q4/004475.html Note that the overflow is actually harmless, the shdrs_end is sanity checked later in the code and we don't really need the shdrs (like the comment just before this code already explains). -- You are receiving this mail because: You are on the CC list for the bug.
Re: Buildbot failure in Wildebeest Builder on whole buildset
On Wed, Dec 08, 2021 at 05:20:29PM +, build...@builder.wildebeest.org wrote: > The Buildbot has detected a new failure on builder elfutils-centos-x86_64 > while building elfutils. > Full details are available at: > https://builder.wildebeest.org/buildbot/#builders/1/builds/868 > > Buildbot URL: https://builder.wildebeest.org/buildbot/ > > Worker for this Build: centos-x86_64 This was a make check timeout. I have been unable to replicate it. A rebuild on the same buildbot worker passed.
Re: [PATCHv3] debuginfod: Check result of calling MHD_add_response_header.
On Wed, Dec 08, 2021 at 10:46:29AM -0500, Frank Ch. Eigler via Elfutils-devel wrote: > Hi - > > > > Why move the *size assignment in there? > > > > Because both statements are unnecessary if r == NULL (aka the response > > couldn't even be created). [...] > > But it is also harmless to do, so if you want I can move it outside the > > if statement. > > OK, whichever, doesn't much matter then. Pushed v3 as is. Thanks, Mark
[Bug libdw/28659] UBSan seems to complain about an "integer overflow" in dwfl_segment_report_module
https://sourceware.org/bugzilla/show_bug.cgi?id=28659 --- Comment #2 from Evgeny Vereshchagin --- > Note that the overflow is actually harmless It is but since the fuzz target ran into it almost as soon as it started it prevented the fuzz target from discovering new issues that can be less harmless though. Looks like the issue is gone. Thanks! FWIW judging by https://github.com/evverx/elfutils/pull/40#issuecomment-989275575, it fixed one LGTM alert as well. I'm not sure if I mentioned this anywhere but LGTM builds those reports on a daily basis and those reports can be found at https://lgtm.com/projects/g/evverx/elfutils/alerts/?mode=tree . -- You are receiving this mail because: You are on the CC list for the bug.
[PATCH] libdwfl: Don't install an Elf handle in a Dwfl_Module twice
dwfl_segment_report_module can be called with the same module
name, start and end address twice (probably because of a corrupt
core file). In that case don't override the main.elf handle if
it already exists.
https://sourceware.org/bugzilla/show_bug.cgi?id=28655
Signed-off-by: Mark Wielaard
---
libdwfl/ChangeLog| 5 +
libdwfl/dwfl_segment_report_module.c | 2 +-
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index 5e25c1b0..a896c2b4 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,3 +1,8 @@
+2021-12-08 Mark Wielaard
+
+ * dwfl_segment_report_module.c (dwfl_segment_report_module): Check
+ Dwfl_Module isn't associated with an Elf before installing it.
+
2021-12-08 Mark Wielaard
* dwfl_segment_report_module.c (dwfl_segment_report_module): Don't
diff --git a/libdwfl/dwfl_segment_report_module.c
b/libdwfl/dwfl_segment_report_module.c
index 5efc9d6a..8d749f4c 100644
--- a/libdwfl/dwfl_segment_report_module.c
+++ b/libdwfl/dwfl_segment_report_module.c
@@ -959,7 +959,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char
*name,
elf->flags |= ELF_F_MALLOCED;
}
- if (elf != NULL)
+ if (elf != NULL && mod->main.elf == NULL)
{
/* Install the file in the module. */
mod->main.elf = elf;
--
2.30.2
[Bug libdw/28655] There seems to be a memory leak in file_read_elf
https://sourceware.org/bugzilla/show_bug.cgi?id=28655 Mark Wielaard changed: What|Removed |Added CC||mark at klomp dot org Status|UNCONFIRMED |ASSIGNED Ever confirmed|0 |1 Assignee|unassigned at sourceware dot org |mark at klomp dot org Last reconfirmed||2021-12-08 --- Comment #1 from Mark Wielaard --- Proposed patch: https://sourceware.org/pipermail/elfutils-devel/2021q4/004480.html -- You are receiving this mail because: You are on the CC list for the bug.
Re: [PATCH] readelf: Workaround stringop-truncation error
On Sat, Dec 04, 2021 at 10:15:04PM +0100, Mark Wielaard wrote: > In function ‘strncpy’, > inlined from ‘print_ehdr’ at readelf.c:1175:4: > error: ‘__builtin_strncpy’ specified bound 512 equals destination size >[-Werror=stringop-truncation] > > strncpy doesn't terminate the copied string if there is not enough > room. We compensate later by explicitly adding a zero terminator at > buf[sizeof (buf) - 1]. Normally gcc does see this, but with > -fsanitize=address there is too much (checking) code in between. But > it is actually better to not let strncpy do too much work, so > substract one from the size. Pushed, Mark
Re: [PATCH] tests: varlocs workaround format-overflow errors
On Sat, Dec 04, 2021 at 10:27:29PM +0100, Mark Wielaard wrote: > In function ‘printf’, > inlined from ‘handle_attr’ at varlocs.c:932:3: > error: ‘%s’ directive argument is null [-Werror=format-overflow=] > > The warning is technically correct. A %s argument should not be > NULL. Although in practice all implementations will print it as > "(null)". Workaround this by simply changing the dwarf string > functions to return an "" string. The test is for the correct > names, either "(null)" or "" would make it fail (also remove > a now unnecessary assert, the switch statement will check for unknown > opcodes anyway). Pushed, Mark
Re: [PATCH] debuginfod: Fix debuginfod_pool leak
On Sat, Dec 04, 2021 at 10:33:35PM +0100, Mark Wielaard wrote: > gcc address sanitizer detected a dangling debuginfod_client handler > when debuginfod exits. Make sure to groom the debuginfod client pool > before exit after all threads are done. Pushed, Mark
Re: [PATCH] configure: Add --enable-sanitize-address
Hi, On Sun, Dec 05, 2021 at 02:39:25AM +0100, Mark Wielaard wrote: > --enable-sanitize-address makes sure that all code is compiled with > -fsanitizer=address and all tests run against libasan. > > It can be combined with --enable-sanitize-undefined, but not with > --enable-valgrind. > > In maintainer mode there is one program that causes leaks, i386_gendis, > so disable leak detection for that program. > > One testcase, test_nlist, needs special linker flags, make sure it also > uses -fsanitizer=address when using the address sanitizer. I pushed this. I'll also add --enable-sanitize-address to some of the buildbot CI workers. Cheers, Mark
[Bug libdw/28655] There seems to be a memory leak in file_read_elf
https://sourceware.org/bugzilla/show_bug.cgi?id=28655 --- Comment #2 from Evgeny Vereshchagin --- I can't seem to reproduce that memory leak anymore. Thanks! -- You are receiving this mail because: You are on the CC list for the bug.
Re: [PATCH] debuginfod/debuginfod-client.c: correct string format on 32bit arches with 64bit time_t
Hi - > [...] > 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). Yes, ints are fine for these humane-number-of-seconds kinds of values in the cache configuration. There's no need for maximum length integers or even longs for purposes of storage/parse. In the later interval arithmetic related to time_t values, we can widen them to (time_t) then and there. - FChE
patch rfc: PR28661: debuginfod thread-pool
Hi -
While I think this patch itself is fine, and works around the
libmicrohttpd bug that motivated it, I don't know how to test it
seriously in the testsuite. (We can certainly try few -C options for
parsing & operability.) The error edge cases only appear to occur
under very high load and task limits such as those imposed by systemd
cgroups.
Author: Frank Ch. Eigler
Date: Wed Dec 8 22:41:47 2021 -0500
PR28661: debuginfo connection thread pool support
Add an option -C, which activates libmicrohttpd's thread-pool mode for
handling incoming http connections. Add libmicrohttpd error-logging
callback function so as to receive indication of its internal errors,
and relay counts to our metrics. Some of these internal errors tipped
us off to a microhttpd bug that thread pooling works around. Document
in debuginfod.8 page. Hand-tested against "ulimit -u NNN" shells.
Signed-off-by: Frank Ch. Eigler
diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 21d0721ef080..125e7d816f51 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,11 @@
+2021-12-08 Frank Ch. Eigler
+
+ * debuginfod.cxx (connection_pool): New global.
+ (parse_opt): Parse & check -C option to set it.
+ (error_cb): New callback for libmicrohttpd error counting.
+ (main): Activate MHD_OPTION_THREAD_POOL_SIZE if appropriate.
+ Activate error_cb.
+
2021-12-01 Mark Wielaard
* debuginfod-client.c (debuginfod_query_server): Free tmp_url on
diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
index 0d3f02978ee2..bb0c6cd65670 100644
--- a/debuginfod/debuginfod.cxx
+++ b/debuginfod/debuginfod.cxx
@@ -352,7 +352,9 @@ static const struct argp_option options[] =
{ "rescan-time", 't', "SECONDS", 0, "Number of seconds to wait between
rescans, 0=disable.", 0 },
{ "groom-time", 'g', "SECONDS", 0, "Number of seconds to wait between
database grooming, 0=disable.", 0 },
{ "maxigroom", 'G', NULL, 0, "Run a complete database groom/shrink pass at
startup.", 0 },
- { "concurrency", 'c', "NUM", 0, "Limit scanning thread concurrency to
NUM.", 0 },
+ { "concurrency", 'c', "NUM", 0, "Limit scanning thread concurrency to NUM,
default=#CPUs.", 0 },
+ { "connection-pool", 'C', "NUM", OPTION_ARG_OPTIONAL,
+ "Use webapi connection pool with NUM threads, default=unlim.", 0 },
{ "include", 'I', "REGEX", 0, "Include files matching REGEX, default=all.",
0 },
{ "exclude", 'X', "REGEX", 0, "Exclude files matching REGEX,
default=none.", 0 },
{ "port", 'p', "NUM", 0, "HTTP port to listen on, default 8002.", 0 },
@@ -411,6 +413,7 @@ static unsigned rescan_s = 300;
static unsigned groom_s = 86400;
static bool maxigroom = false;
static unsigned concurrency = std::thread::hardware_concurrency() ?: 1;
+static int connection_pool = 0;
static set source_paths;
static bool scan_files = false;
static map scan_archives;
@@ -557,6 +560,16 @@ parse_opt (int key, char *arg,
concurrency = (unsigned) atoi(arg);
if (concurrency < 1) concurrency = 1;
break;
+case 'C':
+ if (arg)
+{
+ connection_pool = atoi(arg);
+ if (connection_pool < 2)
+argp_failure(state, 1, EINVAL, "-C NUM minimum 2");
+}
+ else // arg not given
+connection_pool = std::thread::hardware_concurrency() * 2 ?: 2;
+ break;
case 'I':
// NB: no problem with unconditional free here - an earlier failed
regcomp would exit program
if (passive_p)
@@ -3684,7 +3698,13 @@ sigusr2_handler (int /* sig */)
}
-
+static void // error logging callback from libmicrohttpd internals
+error_cb (void *arg, const char *fmt, va_list ap)
+{
+ (void) arg;
+ inc_metric("error_count","libmicrohttpd",fmt);
+ (void) vdprintf (STDERR_FILENO, fmt, ap);
+}
// A user-defined sqlite function, to score the sharedness of the
@@ -3829,7 +3849,7 @@ main (int argc, char *argv[])
// Start httpd server threads. Separate pool for IPv4 and IPv6, in
// case the host only has one protocol stack.
- MHD_Daemon *d4 = MHD_start_daemon (MHD_USE_THREAD_PER_CONNECTION
+ MHD_Daemon *d4 = MHD_start_daemon ((connection_pool ? 0 :
MHD_USE_THREAD_PER_CONNECTION)
#if MHD_VERSION >= 0x00095300
| MHD_USE_INTERNAL_POLLING_THREAD
#else
@@ -3839,8 +3859,11 @@ main (int argc, char *argv[])
http_port,
NULL, NULL, /* default accept policy */
handler_cb, NULL, /* handler callback */
+ MHD_OPTION_EXTERNAL_LOGGER, error_cb,
NULL,
+ (connection_pool ?
MHD_OPTION_THREAD_POOL_SIZE : MHD_OPTION_END),
+ (connection_pool ? (int)connection_pool :
MHD_OPTION_END),
MHD_OPTION_END);
- MHD_Daemo
