[PATCH v3 4/6] tests/allfcts.c: Install alternate debug information

[PATCH v3 4/6] tests/allfcts.c: Install alternate debug information

[Florian Weimer]

[-- Attachment #1: Type: text/plain, Size: 2675 bytes --]

This change also adds more error checking and reporting.

Signed-off-by: Florian Weimer <fweimer@redhat.com>
---
 tests/ChangeLog |  4 ++++
 tests/allfcts.c | 30 +++++++++++++++++++++++++++++-
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/tests/ChangeLog b/tests/ChangeLog
index a5fc945..ca47d77 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -5,6 +5,10 @@
 	(TESTS): Add run-debugaltlink.sh.
 	(debugaltlink_LDADD): New variable.
 
+	* allfcts.c (setup_alt): New function.
+	(main): Call it.  Implementation additional error checking and
+	reporting.
+
 2014-04-11  Mark Wielaard  <mjw@redhat.com>
 
 	* Makefile.am (AM_CPPFLAGS): Add -I libdwelf.
diff --git a/tests/allfcts.c b/tests/allfcts.c
index 10e0f07..8d19594 100644
--- a/tests/allfcts.c
+++ b/tests/allfcts.c
@@ -18,8 +18,10 @@
 # include <config.h>
 #endif
 
+#include <err.h>
 #include <fcntl.h>
 #include ELFUTILS_HEADER(dw)
+#include ELFUTILS_HEADER(dwelf)
 #include <stdio.h>
 #include <unistd.h>
 
@@ -37,6 +39,23 @@ cb (Dwarf_Die *func, void *arg __attribute__ ((unused)))
   return DWARF_CB_ABORT;
 }
 
+static Dwarf *
+setup_alt (Dwarf *main)
+{
+  const void *build_id;
+  size_t build_id_len;
+  const char *alt_name = dwelf_dwarf_gnu_debugaltlink
+    (main, &build_id, &build_id_len);
+  if (alt_name == NULL)
+    return NULL;
+  int fd = open (alt_name, O_RDONLY);
+  if (fd < 0)
+    err (1, "open (%s)", alt_name);
+  Dwarf *dbg_alt = dwarf_begin (fd, DWARF_C_READ);
+  close (fd);
+  dwarf_setalt (main, dbg_alt);
+  return dbg_alt;
+}
 
 int
 main (int argc, char *argv[])
@@ -44,6 +63,8 @@ main (int argc, char *argv[])
   for (int i = 1; i < argc; ++i)
     {
       int fd = open (argv[i], O_RDONLY);
+      if (fd < 0)
+	err (1, "open (%s)", argv[i]);
 
       Dwarf *dbg = dwarf_begin (fd, DWARF_C_READ);
       if (dbg != NULL)
@@ -51,6 +72,7 @@ main (int argc, char *argv[])
 	  Dwarf_Off off = 0;
 	  size_t cuhl;
 	  Dwarf_Off noff;
+	  Dwarf *dbg_alt = setup_alt (dbg);
 
 	  while (dwarf_nextcu (dbg, off, &noff, &cuhl, NULL, NULL, NULL) == 0)
 	    {
@@ -62,14 +84,20 @@ main (int argc, char *argv[])
 	      do
 		{
 		  doff = dwarf_getfuncs (die, cb, NULL, doff);
+		  if (dwarf_errno () != 0)
+		    errx (1, "dwarf_getfuncs (%s): %s",
+			  argv[i], dwarf_errmsg (-1));
 		}
-	      while (doff != 0 && dwarf_errno () == 0);
+	      while (doff != 0);
 
 	      off = noff;
 	    }
 
+	  dwarf_end (dbg_alt);
 	  dwarf_end (dbg);
 	}
+      else
+	errx (1, "dwarf_begin (%s): %s", argv[i], dwarf_errmsg (-1));
 
       close (fd);
     }
-- 
1.9.0

Re: [PATCH v3 4/6] tests/allfcts.c: Install alternate debug information

[Florian Weimer]

[-- Attachment #1: Type: text/plain, Size: 2175 bytes --]

On 04/18/2014 01:58 PM, Mark Wielaard wrote:
> On Tue, 2014-04-15 at 16:58 +0200, Florian Weimer wrote:
>> This change also adds more error checking and reporting.
>>
>> +	* allfcts.c (setup_alt): New function.
>> +	(main): Call it.  Implementation additional error checking and
>> +	reporting.
>
>> +static Dwarf *
>> +setup_alt (Dwarf *main)
>> +{
>> +  const void *build_id;
>> +  size_t build_id_len;
>> +  const char *alt_name = dwelf_dwarf_gnu_debugaltlink
>> +    (main, &build_id, &build_id_len);
>> +  if (alt_name == NULL)
>> +    return NULL;
>> +  int fd = open (alt_name, O_RDONLY);
>> +  if (fd < 0)
>> +    err (1, "open (%s)", alt_name);
>> +  Dwarf *dbg_alt = dwarf_begin (fd, DWARF_C_READ);
>> +  close (fd);
>
> I am slightly surprised this actually works. Normally you cannot just
> close the fd of the underlying Dwarf (or Elf). For an Elf you can if you
> force it to have been read completely into memory first by doing
> elf_cntl (elf, ELF_C_FDREAD) as is currently done in try_debugaltlink.

Yes, now I'm surprised as well. :-)

> Which you could do here (on the Elf and then call dwarf_begin_elf) if
> you want to make sure to not leak the fd.

I went this route.

>>   		  doff = dwarf_getfuncs (die, cb, NULL, doff);
>> +		  if (dwarf_errno () != 0)
>> +		    errx (1, "dwarf_getfuncs (%s): %s",
>> +			  argv[i], dwarf_errmsg (-1));
>
> I think this works fine, and the original code did this, but in a less
> convenient way (you wouldn't get any error reported). So keep it as is.
>
> But the way dwarf_getfuncs returns an error state is by returning -1.
> Which isn't really documented and somewhat awkward, so what you do is
> nicer. It is just that theoretically a callback could trigger
> dwarf_errno being set and ignore that as being fine, which might be
> silly, but "legal". You would then pick up a non-fatal dwarf_errno.

I added this check because when I still had an actual error code (not 
zero) for the missing .gnu_debugaltlink section in 
dwelf_dwarf_gnu_debugaltlink, the test case broke in a slightly 
non-obvious way.

-- 
Florian Weimer / Red Hat Product Security Team

Re: [PATCH v3 4/6] tests/allfcts.c: Install alternate debug information

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 1849 bytes --]

On Tue, 2014-04-15 at 16:58 +0200, Florian Weimer wrote:
> This change also adds more error checking and reporting.
>
> +	* allfcts.c (setup_alt): New function.
> +	(main): Call it.  Implementation additional error checking and
> +	reporting.

> +static Dwarf *
> +setup_alt (Dwarf *main)
> +{
> +  const void *build_id;
> +  size_t build_id_len;
> +  const char *alt_name = dwelf_dwarf_gnu_debugaltlink
> +    (main, &build_id, &build_id_len);
> +  if (alt_name == NULL)
> +    return NULL;
> +  int fd = open (alt_name, O_RDONLY);
> +  if (fd < 0)
> +    err (1, "open (%s)", alt_name);
> +  Dwarf *dbg_alt = dwarf_begin (fd, DWARF_C_READ);
> +  close (fd);

I am slightly surprised this actually works. Normally you cannot just
close the fd of the underlying Dwarf (or Elf). For an Elf you can if you
force it to have been read completely into memory first by doing
elf_cntl (elf, ELF_C_FDREAD) as is currently done in try_debugaltlink.
Which you could do here (on the Elf and then call dwarf_begin_elf) if
you want to make sure to not leak the fd. But otherwise you need to keep
the fd around and close it after calling dwarf_end.

>  		  doff = dwarf_getfuncs (die, cb, NULL, doff);
> +		  if (dwarf_errno () != 0)
> +		    errx (1, "dwarf_getfuncs (%s): %s",
> +			  argv[i], dwarf_errmsg (-1));

I think this works fine, and the original code did this, but in a less
convenient way (you wouldn't get any error reported). So keep it as is.

But the way dwarf_getfuncs returns an error state is by returning -1.
Which isn't really documented and somewhat awkward, so what you do is
nicer. It is just that theoretically a callback could trigger
dwarf_errno being set and ignore that as being fine, which might be
silly, but "legal". You would then pick up a non-fatal dwarf_errno.

Cheers,

Mark