Re: [PATCH] src/readelf.c: Close skel_fd

[Mark Wielaard]

Hi Aaron,

On Fri, Jan 24, 2025 at 08:32:58PM -0500, Aaron Merey wrote:
> skel_fd is passed to create_dwfl, which calls dup() on skel_fd.
> create_dwfl handles closing the dup'ed fd but not the original.
> 
> Ensure the original skel_fd is closed after it's passed to create_dwfl.

Nice find.

We should add --track-fds=yes to our valgrind testing.  It would have
found this earlier. Two of our tests currently fail with:

-==75005== Open file descriptor 5: testfile-splitdwarf-4
-==75005==at 0x4A1E023: open (in /usr/lib64/libc.so.6)
-==75005==by 0x41392B: open (fcntl2.h:55)
-==75005==by 0x41392B: print_debug (readelf.c:11919)
-==75005==by 0x416451: process_elf_file (readelf.c:1084)
-==75005==by 0x4170CC: process_dwflmod (readelf.c:840)
-==75005==by 0x489CAE0: dwfl_getmodules (dwfl_getmodules.c:86)
-==75005==by 0x406964: process_file (readelf.c:948)
-==75005==by 0x401D11: main (readelf.c:417)

This file descriptor leak disappears with your patch.

Thanks,

Mark

> Signed-off-by: Aaron Merey 
> ---
>  src/readelf.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/src/readelf.c b/src/readelf.c
> index 3e97b64c..6526db07 100644
> --- a/src/readelf.c
> +++ b/src/readelf.c
> @@ -11921,7 +11921,13 @@ print_debug (Dwfl_Module *dwflmod, Ebl *ebl, 
> GElf_Ehdr *ehdr)
>   fprintf (stderr, "Warning: Couldn't open DWARF skeleton file"
>" '%s'\n", skel_name);
> else
> - skel_dwfl = create_dwfl (skel_fd, skel_name);
> + {
> +   skel_dwfl = create_dwfl (skel_fd, skel_name);
> +
> +   /* skel_fd was dup'ed by create_dwfl.  We can close the
> +  original now.  */
> +   close (skel_fd);
> + }
>  
> if (skel_dwfl != NULL)
>   {
> -- 
> 2.47.1
> 

[Bug general/32598] New: Use --track-fds=yes when running tests under valgrind

[mark at klomp dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=32598

Bug ID: 32598
   Summary: Use --track-fds=yes when running tests under valgrind
   Product: elfutils
   Version: unspecified
Status: NEW
  Severity: normal
  Priority: P2
 Component: general
  Assignee: unassigned at sourceware dot org
  Reporter: mark at klomp dot org
CC: elfutils-devel at sourceware dot org
  Target Milestone: ---

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 8f08779825bb..ec6cc9011a63 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -692,7 +692,7 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh run-ar.sh \


 if USE_VALGRIND
-valgrind_cmd=valgrind -q --leak-check=full --error-exitcode=1
+valgrind_cmd=valgrind -q --leak-check=full --error-exitcode=1 --track-fds=yes
 endif


Various tests currently fail with --enable-valgrind with the above
--track-fds=yes addition. It seems most are testcase issues (the test opens an
ELF file to examine and never closes it). But it would have caught the
following issue:
https://inbox.sourceware.org/elfutils-devel/20250125234023.gf8...@gnu.wildebeest.org/T/#t

-- 
You are receiving this mail because:
You are on the CC list for the bug.

Re: [PATCH] debuginfod-client.c: Avoid freeing uninitialized value

[Mark Wielaard]

Hi Aaron,

On Fri, Jan 24, 2025 at 08:32:48PM -0500, Aaron Merey wrote:
> debuginfod_validate_imasig might call free on an uninitialized sig_buf
> due to a goto that can occur before sig_buf is set to NULL.
> 
> Fix this by setting sig_buf to NULL before the goto.

The first thing after exit_validate is free (sig_buf) and the goto
does indeed jump over the initialization of sig_buf.  So this change looks 
correct to me.

I am surprised gcc didn't warn about this. Maybe in practice all local
variables with initializers get initialized together at the start of
the function?

But relying on that seems wrong (and confusing), so please commit this
cleanup.

Thanks,

Mark

> Signed-off-by: Aaron Merey 
> ---
>  debuginfod/debuginfod-client.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
> index deff19ff..d89beae9 100644
> --- a/debuginfod/debuginfod-client.c
> +++ b/debuginfod/debuginfod-client.c
> @@ -1587,6 +1587,7 @@ debuginfod_validate_imasig (debuginfod_client *c, int 
> fd)
>  {
>int rc = ENOSYS;
>  
> +char* sig_buf = NULL;
>  EVP_MD_CTX *ctx = NULL;
>  if (!c || !c->winning_headers)
>  {
> @@ -1594,7 +1595,6 @@ debuginfod_validate_imasig (debuginfod_client *c, int 
> fd)
>goto exit_validate;
>  }
>  // Extract the HEX IMA-signature from the header
> -char* sig_buf = NULL;
>  char* hdr_ima_sig = strcasestr(c->winning_headers, 
> "x-debuginfod-imasignature");
>  if (!hdr_ima_sig || 1 != sscanf(hdr_ima_sig + 
> strlen("x-debuginfod-imasignature:"), "%ms", &sig_buf))
>  {
> -- 
> 2.47.1
>