Re: [Libunwind-devel] [PATCH V2] elf: Follow .gnu_debuglink when resolving function names

[Bert Wesarg]

Dave,

thats looks good. Though not tested yet.

Thanks.

Bert

On Fri, Feb 17, 2017 at 6:37 PM, Dave Watson <address@hidden> wrote:
> Centralize gnu_debuglink logic in elfxx.  Remove previous duplicated logic
> from Gfind_proc_info and os-linux.
>
> Logic is roughly the same as previous load_debug_frame, but uses VLAs
> instead of malloc.
> ---
>  src/dwarf/Gfind_proc_info-lsb.c | 149 
> ++++++----------------------------------
>  src/elfxx.c                     |  96 +++++++++++++++++++++++++-
>  src/elfxx.h                     |   1 +
>  src/os-linux.c                  |   8 +--
>  4 files changed, 117 insertions(+), 137 deletions(-)
>
> diff --git a/src/dwarf/Gfind_proc_info-lsb.c b/src/dwarf/Gfind_proc_info-lsb.c
> index af0d75c..4559495 100644
> --- a/src/dwarf/Gfind_proc_info-lsb.c
> +++ b/src/dwarf/Gfind_proc_info-lsb.c
> @@ -86,151 +86,42 @@ linear_search (unw_addr_space_t as, unw_word_t ip,
>  /* Load .debug_frame section from FILE.  Allocates and returns space
>     in *BUF, and sets *BUFSIZE to its size.  IS_LOCAL is 1 if using the
>     local process, in which case we can search the system debug file
> -   directory; 0 for other address spaces, in which case we do not; or
> -   -1 for recursive calls following .gnu_debuglink.  Returns 0 on
> -   success, 1 on error.  Succeeds even if the file contains no
> -   .debug_frame.  */
> +   directory; 0 for other address spaces, in which case we do
> +   not. Returns 0 on success, 1 on error.  Succeeds even if the file
> +   contains no .debug_frame.  */
>  /* XXX: Could use mmap; but elf_map_image keeps tons mapped in.  */
>
>  static int
>  load_debug_frame (const char *file, char **buf, size_t *bufsize, int 
> is_local)
>  {
> -  FILE *f;
> -  Elf_W (Ehdr) ehdr;
> -  Elf_W (Half) shstrndx;
> -  Elf_W (Shdr) *sec_hdrs = NULL;
> -  char *stringtab = NULL;
> -  unsigned int i;
> -  size_t linksize = 0;
> -  char *linkbuf = NULL;
> -
> -  *buf = NULL;
> -  *bufsize = 0;
> -
> -  f = fopen (file, "r");
> -
> -  if (!f)
> -    return 1;
> -
> -  if (fread (&ehdr, sizeof (Elf_W (Ehdr)), 1, f) != 1)
> -    goto file_error;
> -
> -  shstrndx = ehdr.e_shstrndx;
> -
> -  Debug (4, "opened file '%s'. Section header at offset %d\n",
> -         file, (int) ehdr.e_shoff);
> -
> -  fseek (f, ehdr.e_shoff, SEEK_SET);
> -  sec_hdrs = calloc (ehdr.e_shnum, sizeof (Elf_W (Shdr)));
> -  if (fread (sec_hdrs, sizeof (Elf_W (Shdr)), ehdr.e_shnum, f) != 
> ehdr.e_shnum)
> -    goto file_error;
> -
> -  Debug (4, "loading string table of size %zd\n",
> -           sec_hdrs[shstrndx].sh_size);
> -  stringtab = malloc (sec_hdrs[shstrndx].sh_size);
> -  fseek (f, sec_hdrs[shstrndx].sh_offset, SEEK_SET);
> -  if (fread (stringtab, 1, sec_hdrs[shstrndx].sh_size, f) != 
> sec_hdrs[shstrndx].sh_size)
> -    goto file_error;
> -
> -  for (i = 1; i < ehdr.e_shnum && *buf == NULL; i++)
> -    {
> -      char *secname = &stringtab[sec_hdrs[i].sh_name];
> -
> -      if (strcmp (secname, ".debug_frame") == 0)
> -        {
> -          *bufsize = sec_hdrs[i].sh_size;
> -          *buf = malloc (*bufsize);
> -
> -          fseek (f, sec_hdrs[i].sh_offset, SEEK_SET);
> -          if (fread (*buf, 1, *bufsize, f) != *bufsize)
> -            goto file_error;
> -
> -          Debug (4, "read %zd bytes of .debug_frame from offset %zd\n",
> -                 *bufsize, sec_hdrs[i].sh_offset);
> -        }
> -      else if (strcmp (secname, ".gnu_debuglink") == 0)
> -        {
> -          linksize = sec_hdrs[i].sh_size;
> -          linkbuf = malloc (linksize);
> -
> -          fseek (f, sec_hdrs[i].sh_offset, SEEK_SET);
> -          if (fread (linkbuf, 1, linksize, f) != linksize)
> -            goto file_error;
> -
> -          Debug (4, "read %zd bytes of .gnu_debuglink from offset %zd\n",
> -                 linksize, sec_hdrs[i].sh_offset);
> -        }
> -    }
> +  struct elf_image ei;
> +  Elf_W (Shdr) *shdr;
> +  int ret;
>
> -  free (stringtab);
> -  free (sec_hdrs);
> +  ei.image = NULL;
>
> -  fclose (f);
> +  ret = elf_w (load_debuglink) (file, &ei, is_local);
> +  if (ret != 0)
> +    return ret;
>
> -  /* Ignore separate debug files which contain a .gnu_debuglink section. */
> -  if (linkbuf && is_local == -1)
> +  shdr = elf_w (find_section) (&ei, ".debug_frame");
> +  if (!shdr ||
> +      (shdr->sh_offset + shdr->sh_size > ei.size))
>      {
> -      free (linkbuf);
> +      munmap(ei.image, ei.size);
>        return 1;
>      }
>
> -  if (*buf == NULL && linkbuf != NULL && memchr (linkbuf, 0, linksize) != 
> NULL)
> -    {
> -      char *newname, *basedir, *p;
> -      static const char *debugdir = "/usr/lib/debug";
> -      int ret;
> -
> -      /* XXX: Don't bother with the checksum; just search for the file.  */
> -      basedir = malloc (strlen (file) + 1);
> -      newname = malloc (strlen (linkbuf) + strlen (debugdir)
> -                        + strlen (file) + 9);
> -
> -      p = strrchr (file, '/');
> -      if (p != NULL)
> -        {
> -          memcpy (basedir, file, p - file);
> -          basedir[p - file] = '\0';
> -        }
> -      else
> -        basedir[0] = 0;
> -
> -      strcpy (newname, basedir);
> -      strcat (newname, "/");
> -      strcat (newname, linkbuf);
> -      ret = load_debug_frame (newname, buf, bufsize, -1);
> +  *bufsize = shdr->sh_size;
> +  *buf = malloc (*bufsize);
>
> -      if (ret == 1)
> -        {
> -          strcpy (newname, basedir);
> -          strcat (newname, "/.debug/");
> -          strcat (newname, linkbuf);
> -          ret = load_debug_frame (newname, buf, bufsize, -1);
> -        }
> -
> -      if (ret == 1 && is_local == 1)
> -        {
> -          strcpy (newname, debugdir);
> -          strcat (newname, basedir);
> -          strcat (newname, "/");
> -          strcat (newname, linkbuf);
> -          ret = load_debug_frame (newname, buf, bufsize, -1);
> -        }
> +  memcpy(*buf, shdr->sh_offset + ei.image, *bufsize);
>
> -      free (basedir);
> -      free (newname);
> -    }
> -  free (linkbuf);
> +  Debug (4, "read %zd bytes of .debug_frame from offset %zd\n",
> +        *bufsize, shdr->sh_offset);
>
> +  munmap(ei.image, ei.size);
>    return 0;
> -
> -/* An error reading image file. Release resources and return error code */
> -file_error:
> -  free(stringtab);
> -  free(sec_hdrs);
> -  free(linkbuf);
> -  fclose(f);
> -
> -  return 1;
>  }
>
>  /* Locate the binary which originated the contents of address ADDR. Return
> diff --git a/src/elfxx.c b/src/elfxx.c
> index 25bd5a2..685cf2f 100644
> --- a/src/elfxx.c
> +++ b/src/elfxx.c
> @@ -312,8 +312,13 @@ elf_w (get_proc_name) (unw_addr_space_t as, pid_t pid, 
> unw_word_t ip,
>    unsigned long segbase, mapoff;
>    struct elf_image ei;
>    int ret;
> +  char file[PATH_MAX];
>
> -  ret = tdep_get_elf_image (&ei, pid, ip, &segbase, &mapoff, NULL, 0);
> +  ret = tdep_get_elf_image (&ei, pid, ip, &segbase, &mapoff, file, PATH_MAX);
> +  if (ret < 0)
> +    return ret;
> +
> +  ret = elf_w (load_debuglink) (file, &ei, 1);
>    if (ret < 0)
>      return ret;
>
> @@ -368,3 +373,92 @@ elf_w (find_section) (struct elf_image *ei, const char* 
> secname)
>    /* section not found */
>    return 0;
>  }
> +
> +/* Load a debug section, following .gnu_debuglink if appropriate
> + * Loads ei from file if not already mapped.
> + * If is_local, will also search sys directories /usr/local/dbg
> + *
> + * Returns 0 on success, failure otherwise.
> + * ei will be mapped to file or the located .gnu_debuglink from file
> + */
> +HIDDEN int
> +elf_w (load_debuglink) (const char* file, struct elf_image *ei, int is_local)
> +{
> +  int ret;
> +  Elf_W (Shdr) *shdr;
> +
> +  if (!ei->image)
> +    {
> +      ret = elf_map_image(ei, file);
> +      if (ret)
> +       return ret;
> +    }
> +
> +  /* Ignore separate debug files which contain a .gnu_debuglink section. */
> +  if (is_local == -1) {
> +    return 0;
> +  }
> +
> +  shdr = elf_w (find_section) (ei, ".gnu_debuglink");
> +  if (shdr) {
> +    if (shdr->sh_size >= PATH_MAX ||
> +       (shdr->sh_offset + shdr->sh_size > ei->size))
> +      {
> +       return 0;
> +      }
> +
> +    {
> +      char linkbuf[shdr->sh_size];
> +      char *link = ((char *) ei->image) + shdr->sh_offset;
> +      char *p;
> +      static const char *debugdir = "/usr/lib/debug";
> +      char basedir[strlen(file) + 1];
> +      char newname[shdr->sh_size + strlen (debugdir) + strlen (file) + 9];
> +
> +      memcpy(linkbuf, link, shdr->sh_size);
> +
> +      if (memchr (linkbuf, 0, shdr->sh_size) == NULL)
> +       return 0;
> +
> +      munmap (ei->image, ei->size);
> +      ei->image = NULL;
> +
> +      Debug(1, "Found debuglink section, following %s\n", linkbuf);
> +
> +      p = strrchr (file, '/');
> +      if (p != NULL)
> +       {
> +         memcpy (basedir, file, p - file);
> +         basedir[p - file] = '\0';
> +       }
> +      else
> +       basedir[0] = 0;
> +
> +      strcpy (newname, basedir);
> +      strcat (newname, "/");
> +      strcat (newname, linkbuf);
> +      ret = elf_w (load_debuglink) (newname, ei, -1);
> +
> +      if (ret == -1)
> +       {
> +         strcpy (newname, basedir);
> +         strcat (newname, "/.debug/");
> +         strcat (newname, linkbuf);
> +         ret = elf_w (load_debuglink) (newname, ei, -1);
> +       }
> +
> +      if (ret == -1 && is_local == 1)
> +       {
> +         strcpy (newname, debugdir);
> +         strcat (newname, basedir);
> +         strcat (newname, "/");
> +         strcat (newname, linkbuf);
> +         ret = elf_w (load_debuglink) (newname, ei, -1);
> +       }
> +
> +      return ret;
> +    }
> +  }
> +
> +  return 0;
> +}
> diff --git a/src/elfxx.h b/src/elfxx.h
> index aa08f9e..830432c 100644
> --- a/src/elfxx.h
> +++ b/src/elfxx.h
> @@ -55,6 +55,7 @@ extern int elf_w (get_proc_name_in_image) (unw_addr_space_t 
> as,
>                                             char *buf, size_t buf_len, 
> unw_word_t *offp);
>
>  extern Elf_W (Shdr)* elf_w (find_section) (struct elf_image *ei, const char* 
> secname);
> +extern int elf_w (load_debuglink) (const char* file, struct elf_image *ei, 
> int is_local);
>
>  static inline int
>  elf_w (valid_object) (struct elf_image *ei)
> diff --git a/src/os-linux.c b/src/os-linux.c
> index dc861a8..25fe52a 100644
> --- a/src/os-linux.c
> +++ b/src/os-linux.c
> @@ -37,7 +37,6 @@ tdep_get_elf_image (struct elf_image *ei, pid_t pid, 
> unw_word_t ip,
>    struct map_iterator mi;
>    int found = 0, rc;
>    unsigned long hi;
> -  char debug_path[PATH_MAX];
>
>    if (maps_init (&mi, pid) < 0)
>      return -1;
> @@ -58,12 +57,7 @@ tdep_get_elf_image (struct elf_image *ei, pid_t pid, 
> unw_word_t ip,
>      {
>        strncpy(path, mi.path, pathlen);
>      }
> -  snprintf (debug_path, sizeof (debug_path), "/usr/lib/debug%s", mi.path);
> -  rc = elf_map_image (ei, debug_path);
> -  if (rc != 0)
> -    {
> -      rc = elf_map_image (ei, mi.path);
> -    }
> +  rc = elf_map_image (ei, mi.path);
>    maps_close (&mi);
>    return rc;
>  }
> --
> 2.8.0-rc2
>