[PATCHv2 0/2] specify a sysroot to search when examining a core file

[PATCHv2 0/2] specify a sysroot to search when examining a core file

[Luke Diamand]

This updates my patch to check for a NULL sysroot, as pointed out by
Dmitry, and to canonicalize sysroot.

https://sourceware.org/ml/elfutils-devel/2019-q1/msg00071.html

Luke Diamand (2):
  libdwfl: specify optional sysroot to search for shared libraries
  eu-stack: add support for sysroot option

 libdw/libdw.map    |  7 ++++++-
 libdwfl/dwfl_end.c |  1 +
 libdwfl/libdwfl.h  |  5 +++++
 libdwfl/libdwflP.h |  1 +
 libdwfl/link_map.c | 26 ++++++++++++++++++++++++--
 src/stack.c        |  9 +++++++++
 6 files changed, 46 insertions(+), 3 deletions(-)

-- 
2.20.1

[PATCHv2 1/2] libdwfl: specify optional sysroot to search for shared libraries

[Luke Diamand]

When searching the list of modules in a core file, if the core was
generated on a different system to the current one, we need to look
in a sysroot for the various shared objects.

For example, we might be looking at a core file from an ARM system
using elfutils running on an x86 host.

This change adds a new function, dwfl_set_sysroot(), which then
gets used when searching for libraries.

Signed-off-by: Luke Diamand <ldiamand@roku.com>
---
 libdw/libdw.map    |  7 ++++++-
 libdwfl/dwfl_end.c |  1 +
 libdwfl/libdwfl.h  |  5 +++++
 libdwfl/libdwflP.h |  1 +
 libdwfl/link_map.c | 26 ++++++++++++++++++++++++--
 5 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/libdw/libdw.map b/libdw/libdw.map
index 55482d58..43a9de2e 100644
--- a/libdw/libdw.map
+++ b/libdw/libdw.map
@@ -360,4 +360,9 @@ ELFUTILS_0.173 {
 ELFUTILS_0.175 {
   global:
     dwelf_elf_begin;
-} ELFUTILS_0.173;
\ No newline at end of file
+} ELFUTILS_0.173;
+
+ELFUTILS_0.176 {
+  global:
+    dwfl_set_sysroot;
+} ELFUTILS_0.175;
diff --git a/libdwfl/dwfl_end.c b/libdwfl/dwfl_end.c
index 74ee9e07..345d9947 100644
--- a/libdwfl/dwfl_end.c
+++ b/libdwfl/dwfl_end.c
@@ -45,6 +45,7 @@ dwfl_end (Dwfl *dwfl)
   free (dwfl->lookup_addr);
   free (dwfl->lookup_module);
   free (dwfl->lookup_segndx);
+  free (dwfl->sysroot);
 
   Dwfl_Module *next = dwfl->modulelist;
   while (next != NULL)
diff --git a/libdwfl/libdwfl.h b/libdwfl/libdwfl.h
index a0c1d357..c11e2f24 100644
--- a/libdwfl/libdwfl.h
+++ b/libdwfl/libdwfl.h
@@ -807,6 +807,11 @@ int dwfl_getthread_frames (Dwfl *dwfl, pid_t tid,
 bool dwfl_frame_pc (Dwfl_Frame *state, Dwarf_Addr *pc, bool *isactivation)
   __nonnull_attribute__ (1, 2);
 
+/* Set the sysroot to use when searching for shared libraries. If not
+ specified, search in the system root.  */
+void dwfl_set_sysroot (Dwfl *dwfl, const char *sysroot)
+  __nonnull_attribute__ (1);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/libdwfl/libdwflP.h b/libdwfl/libdwflP.h
index 941a8b66..db16ab57 100644
--- a/libdwfl/libdwflP.h
+++ b/libdwfl/libdwflP.h
@@ -138,6 +138,7 @@ struct Dwfl
   int lookup_tail_ndx;
 
   struct Dwfl_User_Core *user_core;
+  char *sysroot;	/* sysroot, or NULL to search standard system paths */
 };
 
 #define OFFLINE_REDZONE		0x10000
diff --git a/libdwfl/link_map.c b/libdwfl/link_map.c
index 29307c74..cf18c0a2 100644
--- a/libdwfl/link_map.c
+++ b/libdwfl/link_map.c
@@ -34,6 +34,7 @@
 #include <byteswap.h>
 #include <endian.h>
 #include <fcntl.h>
+#include <stdlib.h>
 
 /* This element is always provided and always has a constant value.
    This makes it an easy thing to scan for to discern the format.  */
@@ -388,8 +389,21 @@ report_r_debug (uint_fast8_t elfclass, uint_fast8_t elfdata,
       if (name != NULL)
 	{
 	  /* This code is mostly inlined dwfl_report_elf.  */
-	  // XXX hook for sysroot
-	  int fd = open (name, O_RDONLY);
+	  char *path_name;
+	  int rc;
+	  const char *sysroot = dwfl->sysroot;
+
+	  /* don't look in the sysroot if the path is already inside the sysroot */
+	  bool name_in_sysroot = sysroot && (strncmp(name, sysroot, strlen(sysroot)) == 0);
+
+	  if (!name_in_sysroot && sysroot)
+	    rc = asprintf(&path_name, "%s/%s", sysroot, name);
+	  else
+	    rc = asprintf(&path_name, "%s", name);
+	  if (unlikely(rc == -1))
+	    return release_buffer(-1);
+
+	  int fd = open (path_name, O_RDONLY);
 	  if (fd >= 0)
 	    {
 	      Elf *elf;
@@ -471,6 +485,7 @@ report_r_debug (uint_fast8_t elfclass, uint_fast8_t elfdata,
 		    close (fd);
 		}
 	    }
+          free(path_name);
 	}
 
       if (mod != NULL)
@@ -1037,3 +1052,10 @@ dwfl_link_map_report (Dwfl *dwfl, const void *auxv, size_t auxv_size,
 			 &integrated_memory_callback, &mcb, r_debug_info);
 }
 INTDEF (dwfl_link_map_report)
+
+void
+dwfl_set_sysroot (Dwfl *dwfl, const char *sysroot)
+{
+  dwfl->sysroot = realpath(sysroot, NULL);
+}
+INTDEF (dwfl_set_sysroot)
-- 
2.20.1

Re: [PATCHv2 1/2] libdwfl: specify optional sysroot to search for shared libraries

[Mark Wielaard]

On Tue, Jan 22, 2019 at 10:16:25AM +0000, Luke Diamand wrote:
> When searching the list of modules in a core file, if the core was
> generated on a different system to the current one, we need to look
> in a sysroot for the various shared objects.
> 
> For example, we might be looking at a core file from an ARM system
> using elfutils running on an x86 host.
> 
> This change adds a new function, dwfl_set_sysroot(), which then
> gets used when searching for libraries.

A few comments below.
Also a ChangeLog entry is missing. I added comments for them.

> Signed-off-by: Luke Diamand <ldiamand@roku.com>
> ---
>  libdw/libdw.map    |  7 ++++++-
>  libdwfl/dwfl_end.c |  1 +
>  libdwfl/libdwfl.h  |  5 +++++
>  libdwfl/libdwflP.h |  1 +
>  libdwfl/link_map.c | 26 ++++++++++++++++++++++++--
>  5 files changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/libdw/libdw.map b/libdw/libdw.map
> index 55482d58..43a9de2e 100644
> --- a/libdw/libdw.map
> +++ b/libdw/libdw.map
> @@ -360,4 +360,9 @@ ELFUTILS_0.173 {
>  ELFUTILS_0.175 {
>    global:
>      dwelf_elf_begin;
> -} ELFUTILS_0.173;
> \ No newline at end of file
> +} ELFUTILS_0.173;
> +
> +ELFUTILS_0.176 {
> +  global:
> +    dwfl_set_sysroot;
> +} ELFUTILS_0.175;

OK.

       * libdw.map (ELFUTILS_0.176): New section, add dwfl_set_rootsysroot.

> diff --git a/libdwfl/dwfl_end.c b/libdwfl/dwfl_end.c
> index 74ee9e07..345d9947 100644
> --- a/libdwfl/dwfl_end.c
> +++ b/libdwfl/dwfl_end.c
> @@ -45,6 +45,7 @@ dwfl_end (Dwfl *dwfl)
>    free (dwfl->lookup_addr);
>    free (dwfl->lookup_module);
>    free (dwfl->lookup_segndx);
> +  free (dwfl->sysroot);
>  
>    Dwfl_Module *next = dwfl->modulelist;
>    while (next != NULL)

OK.

	* libdwfl_end.c (dwfl_end): Free dwfl->sysroot.

> diff --git a/libdwfl/libdwfl.h b/libdwfl/libdwfl.h
> index a0c1d357..c11e2f24 100644
> --- a/libdwfl/libdwfl.h
> +++ b/libdwfl/libdwfl.h
> @@ -807,6 +807,11 @@ int dwfl_getthread_frames (Dwfl *dwfl, pid_t tid,
>  bool dwfl_frame_pc (Dwfl_Frame *state, Dwarf_Addr *pc, bool *isactivation)
>    __nonnull_attribute__ (1, 2);
>  
> +/* Set the sysroot to use when searching for shared libraries. If not
> + specified, search in the system root.  */
> +void dwfl_set_sysroot (Dwfl *dwfl, const char *sysroot)
> +  __nonnull_attribute__ (1);

This should document that a copy is made of sysroot which
is owned by the library, the passed in string is owned and
can be freed after the call.

It probably should return an int, so failure can be indicated
(you call realpath, which can fail).
What happens if sysroot was already set?
Does setting it to NULL clear it?

	* libdwfl.h (dwfl_set_sysroot): New function declaration.

> +
>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/libdwfl/libdwflP.h b/libdwfl/libdwflP.h
> index 941a8b66..db16ab57 100644
> --- a/libdwfl/libdwflP.h
> +++ b/libdwfl/libdwflP.h
> @@ -138,6 +138,7 @@ struct Dwfl
>    int lookup_tail_ndx;
>  
>    struct Dwfl_User_Core *user_core;
> +  char *sysroot;	/* sysroot, or NULL to search standard system paths */
>  };

OK.

	* libdwflP.h (struct Dwfl): Add sysroot field.
>  
>  #define OFFLINE_REDZONE		0x10000
> diff --git a/libdwfl/link_map.c b/libdwfl/link_map.c
> index 29307c74..cf18c0a2 100644
> --- a/libdwfl/link_map.c
> +++ b/libdwfl/link_map.c
> @@ -34,6 +34,7 @@
>  #include <byteswap.h>
>  #include <endian.h>
>  #include <fcntl.h>
> +#include <stdlib.h>
>  
>  /* This element is always provided and always has a constant value.
>     This makes it an easy thing to scan for to discern the format.  */
> @@ -388,8 +389,21 @@ report_r_debug (uint_fast8_t elfclass, uint_fast8_t elfdata,
>        if (name != NULL)
>  	{
>  	  /* This code is mostly inlined dwfl_report_elf.  */
> -	  // XXX hook for sysroot
> -	  int fd = open (name, O_RDONLY);
> +	  char *path_name;
> +	  int rc;
> +	  const char *sysroot = dwfl->sysroot;
> +
> +	  /* don't look in the sysroot if the path is already inside the sysroot */
> +	  bool name_in_sysroot = sysroot && (strncmp(name, sysroot, strlen(sysroot)) == 0);
> +
> +	  if (!name_in_sysroot && sysroot)

The && sysroot is now redundant.

> +	    rc = asprintf(&path_name, "%s/%s", sysroot, name);
> +	  else
> +	    rc = asprintf(&path_name, "%s", name);
> +	  if (unlikely(rc == -1))
> +	    return release_buffer(-1);

Minor optimization.
In theory you could just assign name to path_name here and then...

> +
> +	  int fd = open (path_name, O_RDONLY);
>  	  if (fd >= 0)
>  	    {
>  	      Elf *elf;
> @@ -471,6 +485,7 @@ report_r_debug (uint_fast8_t elfclass, uint_fast8_t elfdata,
>  		    close (fd);
>  		}
>  	    }
> +          free(path_name);

Guard this free with if (name_in_sysroot).
No worries if you think that is ugly, it does make for
easier error handling and resource cleanup.

Please use tab plus 2 spaces to indent.

	* link_map.c (report_r_debug): Check and use Dwfl sysroot.

>  	}
>  
>        if (mod != NULL)
> @@ -1037,3 +1052,10 @@ dwfl_link_map_report (Dwfl *dwfl, const void *auxv, size_t auxv_size,
>  			 &integrated_memory_callback, &mcb, r_debug_info);
>  }
>  INTDEF (dwfl_link_map_report)
> +
> +void
> +dwfl_set_sysroot (Dwfl *dwfl, const char *sysroot)
> +{
> +  dwfl->sysroot = realpath(sysroot, NULL);
> +}
> +INTDEF (dwfl_set_sysroot)

I like it better if this goes into its own file (in theory every public
function has its own implementation file). Also see the comments above
for the function declaration. You probably want to handle sysroot
already been set (error or replace?) and sysroot being NULL (error or
clear sysroot setting?).

Cheers,

Mark

[PATCHv2 2/2] eu-stack: add support for sysroot option

[Luke Diamand]

Use the dwfl_set_sysroot() function to set the sysroot to be
used when analysing a core:

e.g.
   $ eu-stack --core core --sysroot /path/to/sysroot -e crashing_prog

Signed-off-by: Luke Diamand <ldiamand@roku.com>
---
 src/stack.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/stack.c b/src/stack.c
index c5f347e1..5a58cc1b 100644
--- a/src/stack.c
+++ b/src/stack.c
@@ -73,6 +73,7 @@ static int core_fd = -1;
 static Elf *core = NULL;
 static const char *exec = NULL;
 static char *debuginfo_path = NULL;
+static const char *sysroot = NULL;
 
 static const Dwfl_Callbacks proc_callbacks =
   {
@@ -554,6 +555,10 @@ parse_opt (int key, char *arg __attribute__ ((unused)),
       show_modules = true;
       break;
 
+    case 'S':
+      sysroot = arg;
+      break;
+
     case ARGP_KEY_END:
       if (core == NULL && exec != NULL)
 	argp_error (state,
@@ -587,6 +592,8 @@ parse_opt (int key, char *arg __attribute__ ((unused)),
 	  dwfl = dwfl_begin (&core_callbacks);
 	  if (dwfl == NULL)
 	    error (EXIT_BAD, 0, "dwfl_begin: %s", dwfl_errmsg (-1));
+          if (sysroot)
+            dwfl_set_sysroot(dwfl, sysroot);
 	  if (dwfl_core_file_report (dwfl, core, exec) < 0)
 	    error (EXIT_BAD, 0, "dwfl_core_file_report: %s", dwfl_errmsg (-1));
 	}
@@ -670,6 +677,8 @@ main (int argc, char **argv)
 	N_("Show at most MAXFRAMES per thread (default 256, use 0 for unlimited)"), 0 },
       { "list-modules", 'l', NULL, 0,
 	N_("Show module memory map with build-id, elf and debug files detected"), 0 },
+      { "sysroot", 'S', "sysroot", 0,
+	N_("Set the sysroot to search for libraries referenced from the core file"), 0 },
       { NULL, 0, NULL, 0, NULL, 0 }
     };
 
-- 
2.20.1

Re: [PATCHv2 2/2] eu-stack: add support for sysroot option

[Mark Wielaard]

On Tue, Jan 22, 2019 at 10:16:26AM +0000, Luke Diamand wrote:
> Use the dwfl_set_sysroot() function to set the sysroot to be
> used when analysing a core:
> 
> e.g.
>    $ eu-stack --core core --sysroot /path/to/sysroot -e crashing_prog
> 
> Signed-off-by: Luke Diamand <ldiamand@roku.com>

This looks perfect (but is missing a ChangeLog entry).

Thanks,

Mark