[Bug libdw/30047] libdw unable to handle DW_TAG_unspecified_type
https://sourceware.org/bugzilla/show_bug.cgi?id=30047 Mark Wielaard changed: What|Removed |Added Attachment #14633|0 |1 is obsolete|| --- Comment #6 from Mark Wielaard --- Created attachment 14657 --> https://sourceware.org/bugzilla/attachment.cgi?id=14657&action=edit backends: Handle DW_TAG_unspecified_type in dwarf_peeled_die_type (In reply to Martin Liska from comment #5) > May I please ping this issue as one needs it with the latest binutils > release (2.40). Yes, thanks. I didn't forget, but I changed my mind how to best handle this issue. See the new patch. This changes the code so that an DW_TAG_unspecified_type is treated just as if the function doesn't have a return type. I think that is a better fix because there might be more code out there that uses dwfl_module_return_value_location and might not handle an error in this case. And there isn't actually much that can be done with an unspecified type, for normal cases it is as if there is no return type. I did adjust the testcase to show how you can see whether it is a missing return type or an unspecified return type in case you program does care. -- You are receiving this mail because: You are on the CC list for the bug.
Re: [PATCH] libdw: check memory access in get_(u|s)leb128
Hi Mark, > Did you actually find situations where these functions were called with addrp > >= endp? Yes, for example libdw/libdw_form.c:91:7. > It turns out that get_[su]leb128 dominates some operations and really does > have to be as fast as possible. So I do like to know what the impact is of > this change. This patch just moves __libdw_max_len_uleb128 to the beginning of the function and adds only one new if. So hopefully it shouldn't affect performance at all.
Re: [PATCH] libdw: check memory access in get_(u|s)leb128
Hi Aleksei,
On Tue, 2023-02-07 at 16:17 +, Aleksei Vetrov wrote:
> > Did you actually find situations where these functions were called
> > with addrp
> > > = endp?
>
> Yes, for example libdw/libdw_form.c:91:7.
>
Urgh. There are actually 3 places in that function that need a guard.
Then I started reviewing all the library code and came up with more
than 10 other places that had a guard missing (see attached). I'll
clean this up and also audit elflint.c and readelf.c and submit a
proper patch for that.
> > It turns out that get_[su]leb128 dominates some operations and really does
> > have to be as fast as possible. So I do like to know what the impact is of
> > this change.
>
> This patch just moves __libdw_max_len_uleb128 to the beginning of the function
> and adds only one new if. So hopefully it shouldn't affect performance at all.
Yeah. At first I thought it might be unnecessary because we already
have this check in the calling code. But given that I quickly found 10+
places were that wasn't true I don't feel very confident about that
now...
Then I thought maybe we can just remove the calling code checks and
simply always check like you are proposing. But the checks are slightly
different. The caller guards signal bad data errors, while yours are
hardening checks that make sure no invalid data is read. It is better
to have both. Even if it might slow down the code.
I'll finish the audit of readelf.c and elflint.c and see if adding both
those checks and your hardening checks don't pessimize the code too
much. Hopefully it won't and we can just add all the extra checks.
Cheers,
Mark
diff --git a/libdw/cfi.c b/libdw/cfi.c
index 6d08ca90..a7174405 100644
--- a/libdw/cfi.c
+++ b/libdw/cfi.c
@@ -239,6 +239,7 @@ execute_cfi (Dwarf_CFI *cache,
case DW_CFA_offset_extended_sf:
get_uleb128 (operand, program, end);
+ cfi_assert (program < end);
get_sleb128 (sf_offset, program, end);
offset_extended_sf:
offset = sf_offset * cie->data_alignment_factor;
@@ -294,6 +295,7 @@ execute_cfi (Dwarf_CFI *cache,
get_uleb128 (regno, program, end);
/* DW_FORM_block is a ULEB128 length followed by that many bytes. */
offset = program - (const uint8_t *) cache->data->d.d_buf;
+ cfi_assert (program < end);
get_uleb128 (operand, program, end);
cfi_assert (operand <= (Dwarf_Word) (end - program));
program += operand;
diff --git a/libdw/dwarf_child.c b/libdw/dwarf_child.c
index c8c8bb61..96a0d608 100644
--- a/libdw/dwarf_child.c
+++ b/libdw/dwarf_child.c
@@ -73,10 +73,13 @@ __libdw_find_attr (Dwarf_Die *die, unsigned int search_name,
if (attr_form == DW_FORM_indirect)
{
+ if (readp >= endp)
+ goto invalid;
get_uleb128 (attr_form, readp, endp);
if (attr_form == DW_FORM_indirect ||
attr_form == DW_FORM_implicit_const)
{
+ invalid:
__libdw_seterrno (DWARF_E_INVALID_DWARF);
return NULL;
}
diff --git a/libdw/dwarf_frame_register.c b/libdw/dwarf_frame_register.c
index bcf3fa03..a6b7c4c1 100644
--- a/libdw/dwarf_frame_register.c
+++ b/libdw/dwarf_frame_register.c
@@ -100,6 +100,11 @@ dwarf_frame_register (Dwarf_Frame *fs, int regno, Dwarf_Op ops_mem[3],
const uint8_t *p = fs->cache->data->d.d_buf + reg->value;
const uint8_t *end = (fs->cache->data->d.d_buf
+ fs->cache->data->d.d_size);
+ if (p >= end)
+ {
+ __libdw_seterrno (DWARF_E_INVALID_DWARF);
+ return -1;
+ }
get_uleb128 (block.length, p, end);
block.data = (void *) p;
diff --git a/libdw/dwarf_getabbrev.c b/libdw/dwarf_getabbrev.c
index 13bee493..5b02333f 100644
--- a/libdw/dwarf_getabbrev.c
+++ b/libdw/dwarf_getabbrev.c
@@ -77,6 +77,7 @@ __libdw_getabbrev (Dwarf *dbg, struct Dwarf_CU *cu, Dwarf_Off offset,
+ dbg->sectiondata[IDX_debug_abbrev]->d_size);
const unsigned char *start_abbrevp = abbrevp;
unsigned int code;
+ // We start off with abbrevp at offset, which is checked above.
get_uleb128 (code, abbrevp, end);
/* Check whether this code is already in the hash table. */
diff --git a/libdw/dwarf_getlocation.c b/libdw/dwarf_getlocation.c
index d0d78163..4e8c047b 100644
--- a/libdw/dwarf_getlocation.c
+++ b/libdw/dwarf_getlocation.c
@@ -545,7 +545,7 @@ __libdw_intern_expression (Dwarf *dbg, bool other_byte_order,
case DW_OP_deref_type:
case DW_OP_GNU_deref_type:
case DW_OP_xderef_type:
- if (unlikely (data + 1 >= end_data))
+ if (unlikely (data + 2 >= end_data))
goto invalid;
newloc->number = *data++;
get_uleb128 (newloc->number2, data, end_data);
diff --git a/libdw/dwarf_getsrclines.c b/libdw/dwarf_getsrclines.c
index 2c1d7a40..df003c5f 100644
--- a/libdw/dwarf_getsrclines.c
+++ b/libdw/dwarf_getsrclines.c
@@ -572,6 +572,8 @@ read_srclines (Dwarf *dbg,
goto invalid_data;
size_t nfiles;
+ if ((size_t) (lineendp - linep) < 1)
+ goto invalid_data;
get_uleb128 (nfiles, linep, lineendp);
if (nforms == 0 && nfiles != 0)
diff --git a/libdw/encoded-value.
Re: [PATCH RFC 00/11] Add Memory Sanitizer support
Hi Ilya,
On Mon, Feb 06, 2023 at 11:25:02PM +0100, Ilya Leoshkevich via Elfutils-devel
wrote:
> This series adds minimalistic support for Memory Sanitizer (MSan) [1].
> MSan is compiler instrumentation for detecting accesses to
> uninitialized memory.
>
> The motivation behind this is to be able to link elfutils into projects
> instrumented with MSan, since it essentially requires all the code
> running in a process to be instrumented.
Interesting. For regular CI testing we do use ubsan, valgrind and/or
asan. So msan might not find many new issues in the elfutils code
itself. But being able to link the elfutils libraries instrumented with
msan against other projects build with msan might be very useful.
> The goal is to provide a setup where elfutils is linked only with zlib
> and most tests pass. Here is the description of the setup that I'm
> using:
>
> - LLVM with argp_parse() instrumentation [2].
>
> - zlib-ng instrumented with MSan:
>
> git clone g...@github.com:zlib-ng/zlib-ng.git
> cmake -DWITH_SANITIZER=Memory -DZLIB_COMPAT=ON -DWITH_GTEST=OFF \
> -DCMAKE_C_COMPILER=clang -DCMAKE_INSTALL_PREFIX=/tmp/zlib-ng
> make install
> export CPATH=/tmp/zlib-ng/include
> export LIBRARY_PATH=/tmp/zlib-ng/lib
>
> - Hack: zlib is used by a lot of system utilities, so adding
> MSan-instrumented zlib to LD_LIBRARY_PATH causes a lot of grief.
> Let elfutils test infrastructure add it there only for running
> tests:
>
> ln -s /tmp/zlib-ng/lib/libz.so.1 libelf/
>
> - elfutils uses printf("%n"), so tweak MSan to unpoison the respective
> arguments. Also disable fast unwinding to get better backtraces:
>
> export MSAN_OPTIONS=check_printf=1,fast_unwind_on_malloc=0
>
> - Minimal configuration of elfutils instrumented with MSan:
>
> autoreconf -i
> CC=clang ./configure --enable-maintainer-mode \
>--enable-sanitize-memory --without-bzlib \
>--without-lzma --without-zstd \
>--disable-debuginfod --disable-libdebuginfod \
>--disable-demangler
Aren't there instrumented versions of bzip2, lzma/xz and/or zstd?
Can't debuginfod and libdebuginfod be instrumented?
Is the demangler disabled because you don't link against (an
instrumented) libstdc++?
> Results:
>
>
> Testsuite summary for elfutils 0.188
>
> # TOTAL: 235
> # PASS: 221
> # SKIP: 14
> # XFAIL: 0
> # FAIL: 0
> # XPASS: 0
> # ERROR: 0
>
Very good.
> The patches take care of the following:
>
> - Fixing clang build.
Yeah, it is a pity msan hasn't been integrated with gcc, we often find
issues with clang.
> - Adding small tweaks to get rid of false positives (no real issues
> were found, most likely because elfutils is already tested with
> valgrind).
> - Dealing with "-self" tests, which now see MSan runtime compiled
> into elfutils binaries.
> - MSan enablement itself.
>
> Ilya Leoshkevich (11):
> libdwfl: Fix debuginfod_client redefinition
> libasm: Fix xdefault_pattern initialization
> printversion: Fix unused variable
> readelf: Fix set but not used parameter
> readelf: Fix set but not used variable
> Initialize reglocs for VMCOREINFO
> addr2line: Do not test demangling in run-addr2line-i-test.sh
> x86_64_return_value_location: Support lvalue and rvalue references
> configure: Use -fno-addrsig if possible
> configure: Add --disable-demangle
> configure: Add --enable-sanitize-memory
Thanks for splitting things out so nicely in separate patches.
Cheers,
Mark
Re: [PATCH RFC 01/11] libdwfl: Fix debuginfod_client redefinition
Hi Ilyam On Mon, Feb 06, 2023 at 11:25:03PM +0100, Ilya Leoshkevich via Elfutils-devel wrote: > clang complains: > > In file included from debuginfod-client.c:38: > ./../debuginfod/debuginfod.h:47:34: error: redefinition of typedef > 'debuginfod_client' is a C11 feature [-Werror,-Wtypedef-redefinition] > typedef struct debuginfod_client debuginfod_client; > ^ > ./libdwfl.h:53:34: note: previous definition is here > typedef struct debuginfod_client debuginfod_client; > ^ > > config/eu.am specifies -std=gnu99, and upgrading just for this is an > overkill. So is #including "debuginfod.h", since we don't know if users > even have it. So fix by using "struct debuginfod_client" instead. This > may break the clients that use dwfl_get_debuginfod_client() without > #including "debuginfod.h", but such cases should be rare. This was recently reported by someone else and fixed differently: commit 45576ab5f24cd39669a418fa8e005b4d04f8e9ca Author: Mark Wielaard Date: Mon Feb 6 10:21:58 2023 +0100 debuginfod: Make sure there is only one typedef for debuginfod_client Both debuginfod.h and libdwfl.h have a simple typedef for struct debuginfod_client. Some compilers pedantically warn when including both headers that such typedefs are only officially supported in C11. So guard them with _ELFUTILS_DEBUGINFOD_CLIENT_TYPEDEF to make them happy. https://sourceware.org/bugzilla/show_bug.cgi?id=30077 Signed-off-by: Mark Wielaard Does that work for you? Thanks, Mark
Re: [PATCH RFC 02/11] libasm: Fix xdefault_pattern initialization
Hi Ilya,
On Mon, Feb 06, 2023 at 11:25:04PM +0100, Ilya Leoshkevich via Elfutils-devel
wrote:
> clang complains:
>
> asm_newscn.c:48:22: error: field 'pattern' with variable sized type
> 'struct FillPattern' not at the end of a struct or class is a GNU extension
> [-Werror,-Wgnu-variable-sized-type-not-at-end]
> struct FillPattern pattern;
> ^
>
> Fix by using a union instead. Define the second union member to be a
> char array 1 byte larger than struct FillPattern. This should be legal
> according to 6.7.9:
>
> If an object that has static or thread storage duration is not
> initialized explicitly, then ... if it is a union, the first named
> member is initialized (recursively) according to these rules, and
> any padding is initialized to zero bits.
>
> Signed-off-by: Ilya Leoshkevich
> ---
> libasm/asm_newscn.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/libasm/asm_newscn.c b/libasm/asm_newscn.c
> index d258d969..32a3b598 100644
> --- a/libasm/asm_newscn.c
> +++ b/libasm/asm_newscn.c
> @@ -43,17 +43,16 @@
> /* Memory for the default pattern. The type uses a flexible array
> which does work well with a static initializer. So we play some
> dirty tricks here. */
> -static const struct
> +static const union
> {
>struct FillPattern pattern;
> - char zero;
> + char zeroes[sizeof(struct FillPattern) + 1];
> } xdefault_pattern =
>{
> .pattern =
> {
>.len = 1
> },
> -.zero = '\0'
>};
Yes, I think this works. Could you update the comment just before this
with some of the commit message explanation? Your explanation is much
better than "play some dirty trick" :)
> const struct FillPattern *__libasm_default_pattern =
> &xdefault_pattern.pattern;
I am surprised this doesn't need a cast. Do you know why?
Thanks,
Mark
Re: [PATCH RFC 01/11] libdwfl: Fix debuginfod_client redefinition
On Tue, 2023-02-07 at 20:22 +0100, Mark Wielaard wrote: > Hi Ilyam > > On Mon, Feb 06, 2023 at 11:25:03PM +0100, Ilya Leoshkevich via > Elfutils-devel wrote: > > clang complains: > > > > In file included from debuginfod-client.c:38: > > ./../debuginfod/debuginfod.h:47:34: error: redefinition of > > typedef 'debuginfod_client' is a C11 feature [-Werror,-Wtypedef- > > redefinition] > > typedef struct debuginfod_client debuginfod_client; > > ^ > > ./libdwfl.h:53:34: note: previous definition is here > > typedef struct debuginfod_client debuginfod_client; > > ^ > > > > config/eu.am specifies -std=gnu99, and upgrading just for this is > > an > > overkill. So is #including "debuginfod.h", since we don't know if > > users > > even have it. So fix by using "struct debuginfod_client" instead. > > This > > may break the clients that use dwfl_get_debuginfod_client() without > > #including "debuginfod.h", but such cases should be rare. > > This was recently reported by someone else and fixed differently: > > commit 45576ab5f24cd39669a418fa8e005b4d04f8e9ca > Author: Mark Wielaard > Date: Mon Feb 6 10:21:58 2023 +0100 > > debuginfod: Make sure there is only one typedef for > debuginfod_client > > Both debuginfod.h and libdwfl.h have a simple typedef for struct > debuginfod_client. Some compilers pedantically warn when > including > both headers that such typedefs are only officially supported in > C11. So guard them with _ELFUTILS_DEBUGINFOD_CLIENT_TYPEDEF to > make them happy. > > https://sourceware.org/bugzilla/show_bug.cgi?id=30077 > > Signed-off-by: Mark Wielaard > > Does that work for you? > > Thanks, > > Mark Thanks, this works. This patch can be dropped.
Re: [PATCH RFC 00/11] Add Memory Sanitizer support
On Tue, 2023-02-07 at 20:05 +0100, Mark Wielaard wrote: > Hi Ilya, > > On Mon, Feb 06, 2023 at 11:25:02PM +0100, Ilya Leoshkevich via > Elfutils-devel wrote: > > This series adds minimalistic support for Memory Sanitizer (MSan) > > [1]. > > MSan is compiler instrumentation for detecting accesses to > > uninitialized memory. [...] > > - Minimal configuration of elfutils instrumented with MSan: > > > > autoreconf -i > > CC=clang ./configure --enable-maintainer-mode \ > > --enable-sanitize-memory --without-bzlib \ > > --without-lzma --without-zstd \ > > --disable-debuginfod --disable-libdebuginfod > > \ > > --disable-demangler > > Aren't there instrumented versions of bzip2, lzma/xz and/or zstd? > > Can't debuginfod and libdebuginfod be instrumented? > > Is the demangler disabled because you don't link against (an > instrumented) libstdc++? I think with some effort instrumenting the dependencies is possible. bzlib and lzma are not particularly large, and zstd should support this out of the box. Regarding C++, an instrumented LLVM's libc++ should also just work. With all this, it should be possible to test elfutils with MSan without disabling the extra functionality. But since you already test with valgrind, I figured it would be highly unlikely that I find new bugs, and decided to limit the scope here. For my current purposes - linking elfutils into libbpf - this proved to be enough. [...] Best regards, Ilya
Re: [PATCH RFC 02/11] libasm: Fix xdefault_pattern initialization
On Tue, 2023-02-07 at 20:41 +0100, Mark Wielaard wrote:
> Hi Ilya,
>
> On Mon, Feb 06, 2023 at 11:25:04PM +0100, Ilya Leoshkevich via
> Elfutils-devel wrote:
> > clang complains:
> >
> > asm_newscn.c:48:22: error: field 'pattern' with variable sized
> > type 'struct FillPattern' not at the end of a struct or class is a
> > GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
> > struct FillPattern pattern;
> > ^
> >
> > Fix by using a union instead. Define the second union member to be
> > a
> > char array 1 byte larger than struct FillPattern. This should be
> > legal
> > according to 6.7.9:
> >
> > If an object that has static or thread storage duration is not
> > initialized explicitly, then ... if it is a union, the first
> > named
> > member is initialized (recursively) according to these rules,
> > and
> > any padding is initialized to zero bits.
> >
> > Signed-off-by: Ilya Leoshkevich
> > ---
> > libasm/asm_newscn.c | 5 ++---
> > 1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/libasm/asm_newscn.c b/libasm/asm_newscn.c
> > index d258d969..32a3b598 100644
> > --- a/libasm/asm_newscn.c
> > +++ b/libasm/asm_newscn.c
> > @@ -43,17 +43,16 @@
> > /* Memory for the default pattern. The type uses a flexible array
> > which does work well with a static initializer. So we play
> > some
> > dirty tricks here. */
> > -static const struct
> > +static const union
> > {
> > struct FillPattern pattern;
> > - char zero;
> > + char zeroes[sizeof(struct FillPattern) + 1];
> > } xdefault_pattern =
> > {
> > .pattern =
> > {
> > .len = 1
> > },
> > - .zero = '\0'
> > };
>
> Yes, I think this works. Could you update the comment just before
> this
> with some of the commit message explanation? Your explanation is much
> better than "play some dirty trick" :)
Thanks, will do.
> > const struct FillPattern *__libasm_default_pattern =
> > &xdefault_pattern.pattern;
>
> I am surprised this doesn't need a cast. Do you know why?
We are referencing the union's .pattern member, not the entire union,
so the types still match.
>
> Thanks,
>
> Mark
Re: [PATCH RFC 03/11] printversion: Fix unused variable
Hi Ilya (CC Frank),
On Mon, Feb 06, 2023 at 11:25:05PM +0100, Ilya Leoshkevich via Elfutils-devel
wrote:
> clang complains:
>
> debuginfod.cxx:354:1: error: unused variable 'apba__'
> [-Werror,-Wunused-const-variable]
> ARGP_PROGRAM_BUG_ADDRESS_DEF = PACKAGE_BUGREPORT;
> ^
> ../lib/printversion.h:47:21: note: expanded from macro
> 'ARGP_PROGRAM_BUG_ADDRESS_DEF'
> const char *const apba__ __asm ("argp_program_bug_address")
> ^
>
> This is as expected: it's used by argp via the
> "argp_program_bug_address" name, which is not visible on the C level.
> Add __attribute__ ((used)) to make sure that the compiler emits it.
Actually I think it found a real issue. Note that the same construct
is used the C eu tools. But in debuginfod.cxx it says:
/* Name and version of program. */
/* ARGP_PROGRAM_VERSION_HOOK_DEF = print_version; */ // not this simple for C++
/* Bug report address. */
ARGP_PROGRAM_BUG_ADDRESS_DEF = PACKAGE_BUGREPORT;
Note how ARGP_PROGRAM_VERSION_HOOK_DEF is commented out and in main it
has:
/* Parse and process arguments. */
int remaining;
argp_program_version_hook = print_version; // this works
(void) argp_parse (&argp, argc, argv, ARGP_IN_ORDER, &remaining, NULL);
So it sets print_version, but not argp_program_bug_address.
And indeed debuginfod --help is missing the bug reporting address.
I don't really know/understand why the printversion.h macro trick doesn't work
with C++ (symbol mangling?). But we do need at least this patch:
diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
index 4271acf4..0ec326d5 100644
--- a/debuginfod/debuginfod.cxx
+++ b/debuginfod/debuginfod.cxx
@@ -4172,6 +4165,7 @@ main (int argc, char *argv[])
/* Parse and process arguments. */
int remaining;
argp_program_version_hook = print_version; // this works
+ argp_program_bug_address = PACKAGE_BUGREPORT;
(void) argp_parse (&argp, argc, argv, ARGP_IN_ORDER, &remaining, NULL);
if (remaining != argc)
error (EXIT_FAILURE, 0,
Then debuginfod --help will say: Report bugs to
https://sourceware.org/bugzilla.
That of course doesn't help with the -Wunused-const-variable warning.
If we cannot figure out the magic variable naming trick with with C++
then maybe we can just not include printversion.h and do it "by hand"?
(as attached)
Cheers,
Mark
diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
index 4271acf4..1ea90645 100644
--- a/debuginfod/debuginfod.cxx
+++ b/debuginfod/debuginfod.cxx
@@ -45,7 +45,6 @@ extern "C" {
#endif
extern "C" {
-#include "printversion.h"
#include "system.h"
}
@@ -345,13 +344,11 @@ static const char DEBUGINFOD_SQLITE_CLEANUP_DDL[] =
;
-
-
-/* Name and version of program. */
-/* ARGP_PROGRAM_VERSION_HOOK_DEF = print_version; */ // not this simple for C++
-
-/* Bug report address. */
-ARGP_PROGRAM_BUG_ADDRESS_DEF = PACKAGE_BUGREPORT;
+extern "C" {
+/* Defined in version.c. Explicitly declared here because including
+ print_version.h magic variable tricks don't work in C++. */
+void print_version (FILE *stream, struct argp_state *state);
+}
/* Definitions of arguments for argp functions. */
static const struct argp_option options[] =
@@ -4172,6 +4169,7 @@ main (int argc, char *argv[])
/* Parse and process arguments. */
int remaining;
argp_program_version_hook = print_version; // this works
+ argp_program_bug_address = PACKAGE_BUGREPORT;
(void) argp_parse (&argp, argc, argv, ARGP_IN_ORDER, &remaining, NULL);
if (remaining != argc)
error (EXIT_FAILURE, 0,
[OB PATCH] debuginfod-client.c: Download section even if cached executable didn't contain it.
Committing as obvious.
Before attempting to download a section, cache_find_section tries to
extract the section from existing files in the cache. If it's determined
that the section must not exist, cache_find_section returns -ENOENT to
indicate that the download should be skipped.
This patch fixes a bug where cache_find_section returns -ENOENT even
though the section exists. If the cache contains the executable but
not the debuginfo with the given build-id and the section only exists
in the debuginfo (such as any of the .debug_* sections), then
debuginfod_find_section returns -ENOENT even if the section could be
downloaded.
Fix this by having cache_find_section not return -ENOENT unless cached
debuginfo was able to be read.
Signed-off-by: Aaron Merey
---
debuginfod/ChangeLog | 5 +
debuginfod/debuginfod-client.c | 26 --
2 files changed, 21 insertions(+), 10 deletions(-)
diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 2ddb7ca0..e961cce9 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,8 @@
+2023-02-07 Aaron Merey
+
+ * debuginfod-client.c (cache_find_section): Avoid returning -ENOENT if
+ debuginfo wasn't found.
+
2022-12-21 Mark Wielaard
* debuginfod-client.c: Define CURL_AT_LEAST_VERSION.
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index a16165bd..430d3cf4 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -785,23 +785,24 @@ out:
an ELF/DWARF section with name SCN_NAME. If found, extract the section
to a separate file in TARGET_CACHE_DIR and return a file descriptor
for the section file. The path for this file will be stored in USR_PATH.
- Return a negative errno if unsuccessful. */
+ Return a negative errno if unsuccessful. -ENOENT indicates that SCN_NAME
+ is confirmed to not exist. */
static int
cache_find_section (const char *scn_name, const char *target_cache_dir,
char **usr_path)
{
- int fd;
+ int debug_fd;
int rc = -EEXIST;
char parent_path[PATH_MAX];
/* Check the debuginfo first. */
snprintf (parent_path, PATH_MAX, "%s/debuginfo", target_cache_dir);
- fd = open (parent_path, O_RDONLY);
- if (fd >= 0)
+ debug_fd = open (parent_path, O_RDONLY);
+ if (debug_fd >= 0)
{
- rc = extract_section (fd, scn_name, parent_path, usr_path);
- close (fd);
+ rc = extract_section (debug_fd, scn_name, parent_path, usr_path);
+ close (debug_fd);
}
/* If the debuginfo file couldn't be found or the section type was
@@ -809,12 +810,17 @@ cache_find_section (const char *scn_name, const char
*target_cache_dir,
if (rc == -EEXIST)
{
snprintf (parent_path, PATH_MAX, "%s/executable", target_cache_dir);
- fd = open (parent_path, O_RDONLY);
+ int exec_fd = open (parent_path, O_RDONLY);
- if (fd >= 0)
+ if (exec_fd >= 0)
{
- rc = extract_section (fd, scn_name, parent_path, usr_path);
- close (fd);
+ rc = extract_section (exec_fd, scn_name, parent_path, usr_path);
+ close (exec_fd);
+
+ /* Don't return -ENOENT if the debuginfo wasn't opened. The
+section may exist in the debuginfo but not the executable. */
+ if (debug_fd < 0 && rc == -ENOENT)
+ rc = -EREMOTE;
}
}
--
2.39.1
☠ Buildbot (Sourceware): elfutils - failed test (failure) (master)
A new failure has been detected on builder elfutils-gentoo-sparc while building elfutils. Full details are available at: https://builder.sourceware.org/buildbot/#builders/225/builds/10 Build state: failed test (failure) Revision: 53b596ef4018693403395d702045c15762af3da7 Worker: gentoo-sparc Build Reason: (unknown) Blamelist: Aaron Merey Steps: - 0: worker_preparation ( success ) - 1: set package name ( success ) - 2: git checkout ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/10/steps/2/logs/stdio - 3: autoreconf ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/10/steps/3/logs/stdio - 4: configure ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/10/steps/4/logs/stdio - config.log: https://builder.sourceware.org/buildbot/#builders/225/builds/10/steps/4/logs/config_log - 5: get version ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/10/steps/5/logs/stdio - property changes: https://builder.sourceware.org/buildbot/#builders/225/builds/10/steps/5/logs/property_changes - 6: make ( warnings ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/10/steps/6/logs/stdio - warnings (3): https://builder.sourceware.org/buildbot/#builders/225/builds/10/steps/6/logs/warnings__3_ - 7: make check ( failure ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/10/steps/7/logs/stdio - test-suite.log: https://builder.sourceware.org/buildbot/#builders/225/builds/10/steps/7/logs/test-suite_log - 8: prep ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/10/steps/8/logs/stdio - 9: build bunsen.cpio.gz ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/10/steps/9/logs/stdio - 10: fetch bunsen.cpio.gz ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/10/steps/10/logs/stdio - 11: unpack bunsen.cpio.gz ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/10/steps/11/logs/stdio - 12: pass .bunsen.source.gitname ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/10/steps/12/logs/stdio - 13: pass .bunsen.source.gitdescribe ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/10/steps/13/logs/stdio - 14: pass .bunsen.source.gitbranch ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/10/steps/14/logs/stdio - 15: pass .bunsen.source.gitrepo ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/10/steps/15/logs/stdio - 16: upload to bunsen ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/10/steps/16/logs/stdio - 17: clean up ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/10/steps/17/logs/stdio - 18: make distclean ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/10/steps/18/logs/stdio
Re: ☠ Buildbot (Sourceware): elfutils - failed test (failure) (master)
On Tue, Feb 7, 2023 at 9:43 PM builder--- via Elfutils-devel wrote: > > A new failure has been detected on builder elfutils-gentoo-sparc while > building elfutils. > > Full details are available at: > https://builder.sourceware.org/buildbot/#builders/225/builds/10 > > Build state: failed test (failure) > Revision: 53b596ef4018693403395d702045c15762af3da7 > Worker: gentoo-sparc > Build Reason: (unknown) > Blamelist: Aaron Merey > > Steps: > [...] > - 7: make check ( failure ) > Logs: > - stdio: > https://builder.sourceware.org/buildbot/#builders/225/builds/10/steps/7/logs/stdio > - test-suite.log: > https://builder.sourceware.org/buildbot/#builders/225/builds/10/steps/7/logs/test-suite_log Failure due to make command timeout. It looks like the only test that didn't finish is run-debuginfod-section.sh, which is related to my changes. But I'm not seeing how the changes might cause a timeout and unfortunately test-suite.log is empty. Aaron
