Re: [PATCH v8 09/10] Validate symbol file using build-id

[Doug Evans <dje at google dot com>]

On Sun, Jun 21, 2015 at 5:16 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> Hi,
>
> updated for:
>         commit c74f7d1c6c5a968330208757f476c67a4bb66643
>         Author: Jon Turney <jon.turney@dronecode.org.uk>
>         Date:   Tue Apr 7 20:49:08 2015 +0100
>             Allow gdb to find debug symbols file by build-id for PE file format also
>
>
> Jan
>
>
> ---------- Forwarded message ----------
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> To:
> Cc:
> Date: Sun, 21 Jun 2015 11:51:52 +0200
> Subject: [PATCH 1/2] Validate symbol file using build-id
> Hi,
>
> consumer part of the "build-id" attribute.
>
> Approved by:
>         https://sourceware.org/ml/gdb-patches/2014-05/msg00424.html
>
>
> Jan
>
>
> gdb/ChangeLog
> 2014-02-26  Aleksandar Ristovski  <aristovski@qnx.com
>             Jan Kratochvil  <jan.kratochvil@redhat.com>
>
>         Validate symbol file using build-id.
>         * NEWS (Changes since GDB 7.9): Add 'set solib-build-id-force'
>         and 'show solib-build-id-force'.  Add build-id attribute.
>         * solib-darwin.c (_initialize_darwin_solib): Assign validate value.
>         * solib-dsbt.c (_initialize_dsbt_solib): Ditto.
>         * solib-frv.c (_initialize_frv_solib): Ditto.
>         * solib-spu.c (set_spu_solib_ops): Ditto.
>         * solib-svr4.c: Include rsp-low.h.
>         (NOTE_GNU_BUILD_ID_NAME): New define.
>         (svr4_validate): New function.
>         (svr4_copy_library_list): Duplicate field build_id.
>         (library_list_start_library): Parse 'build-id' attribute.
>         (svr4_library_attributes): Add 'build-id' attribute.
>         (_initialize_svr4_solib): Assign validate value.
>         * solib-target.c (solib.h): Include.
>         (_initialize_solib_target): Assign validate value.
>         * solib.c (solib_build_id_force, show_solib_build_id_force): New.
>         (solib_map_sections): Use ops->validate.
>         (clear_so): Free build_id.
>         (default_solib_validate): New function.
>         (_initialize_solib): Add "solib-build-id-force".
>         * solib.h (default_solib_validate): New declaration.
>         * solist.h (struct so_list): New fields 'build_idsz' and 'build_id'.
>         (target_so_ops): New field 'validate'.
>
> gdb/doc/ChangeLog
> 2014-03-02  Jan Kratochvil  <jan.kratochvil@redhat.com>
>
>         * gdb.texinfo (Files): Add 'set solib-build-id-force'
>         and 'show solib-build-id-force'.

IIUC this applies to symbol files (the main program) too, right?

If so, having solib in the option name is confusing.

set build-id-force
or
set require-build-id-match
or some such would be clearer.

> diff --git a/gdb/NEWS b/gdb/NEWS
> index 3ec5851..8cbe1fc 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -159,6 +159,12 @@ set debug linux-namespaces
>  show debug linux-namespaces
>    Control display of debugging info regarding Linux namespaces.
>
> +set solib-build-id-force (on|off)
> +show solib-build-id-force
> +  Inferior shared library and symbol file may contain unique build-id.
> +  If both build-ids are present but they do not match then this setting
> +  enables (on) or disables (off) loading of such symbol file.
> +
>  * The command 'thread apply all' can now support new option '-ascending'
>    to call its specified command for all threads in ascending order.
>
> @@ -233,6 +239,12 @@ fork-events and vfork-events features in qSupported
>  * GDB now supports access to vector registers on S/390 GNU/Linux
>    targets.
>
> +* New features in the GDB remote stub, GDBserver
> +
> +  ** library-list-svr4 contains also optional attribute 'build-id' for
> +     each library.  GDB does not load library with build-id that
> +     does not match such attribute.
> +
>  * Removed command line options
>
>  -xdb  HP-UX XDB compatibility mode.
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 1460b7f..f7e4405 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -17912,6 +17912,44 @@ libraries that were loaded by explicit user requests are not
>  discarded.
>  @end table
>
> +@table @code
> +@kindex set solib-build-id-force
> +@cindex override @value{GDBN} build-id check
> +@item set solib-build-id-force @var{mode}
> +Setting to override @value{GDBN} build-id check.
> +
> +Inferior shared libraries and symbol files may contain unique build-id.
> +By default @value{GDBN} will ignore symbol files with non-matching build-id
> +while printing:
> +
> +@smallexample
> +  warning: Shared object "libfoo.so.1" could not be validated (remote
> +  build ID 2bc1745e does not match local build ID a08f8767) and will be
> +  ignored; or use 'set solib-build-id-force'.
> +@end smallexample
> +
> +Turning on this setting would load such symbol file while still printing:
> +
> +@smallexample
> +  warning: Shared object "libfoo.so.1" could not be validated (remote
> +  build ID 2bc1745e does not match local build ID a08f8767) but it is
> +  being loaded due to 'set solib-build-id-force'.
> +@end smallexample
> +
> +If remote build-id is present but it does not match local build-id (or local
> +build-id is not present) then this setting enables (@var{mode} is @code{on}) or
> +disables (@var{mode} is @code{off}) loading of such symbol file.  On systems
> +where build-id is not present in the remote system this setting has no effect.
> +The default value is @code{off}.
> +
> +Loading non-matching symbol file may confuse debugging including breakage
> +of backtrace output.
> +
> +@kindex show solib-build-id-force
> +@item show solib-build-id-force
> +Display the current mode of build-id check override.
> +@end table
> +
>  Sometimes you may wish that @value{GDBN} stops and gives you control
>  when any of shared library events happen.  The best way to do this is
>  to use @code{catch load} and @code{catch unload} (@pxref{Set
> diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c
> index f96841f..9309c35 100644
> --- a/gdb/solib-darwin.c
> +++ b/gdb/solib-darwin.c
> @@ -634,4 +634,5 @@ _initialize_darwin_solib (void)
>    darwin_so_ops.in_dynsym_resolve_code = darwin_in_dynsym_resolve_code;
>    darwin_so_ops.lookup_lib_global_symbol = darwin_lookup_lib_symbol;
>    darwin_so_ops.bfd_open = darwin_bfd_open;
> +  darwin_so_ops.validate = default_solib_validate;
>  }
> diff --git a/gdb/solib-dsbt.c b/gdb/solib-dsbt.c
> index 7da5833..9fe6533 100644
> --- a/gdb/solib-dsbt.c
> +++ b/gdb/solib-dsbt.c
> @@ -1080,6 +1080,7 @@ _initialize_dsbt_solib (void)
>    dsbt_so_ops.open_symbol_file_object = open_symbol_file_object;
>    dsbt_so_ops.in_dynsym_resolve_code = dsbt_in_dynsym_resolve_code;
>    dsbt_so_ops.bfd_open = solib_bfd_open;
> +  dsbt_so_ops.validate = default_solib_validate;
>
>    /* Debug this file's internals.  */
>    add_setshow_zuinteger_cmd ("solib-dsbt", class_maintenance,
> diff --git a/gdb/solib-frv.c b/gdb/solib-frv.c
> index f7ef38b..b768146 100644
> --- a/gdb/solib-frv.c
> +++ b/gdb/solib-frv.c
> @@ -1183,6 +1183,7 @@ _initialize_frv_solib (void)
>    frv_so_ops.open_symbol_file_object = open_symbol_file_object;
>    frv_so_ops.in_dynsym_resolve_code = frv_in_dynsym_resolve_code;
>    frv_so_ops.bfd_open = solib_bfd_open;
> +  frv_so_ops.validate = default_solib_validate;
>
>    /* Debug this file's internals.  */
>    add_setshow_zuinteger_cmd ("solib-frv", class_maintenance,
> diff --git a/gdb/solib-spu.c b/gdb/solib-spu.c
> index 44fbf91..d162884 100644
> --- a/gdb/solib-spu.c
> +++ b/gdb/solib-spu.c
> @@ -519,6 +519,7 @@ set_spu_solib_ops (struct gdbarch *gdbarch)
>        spu_so_ops.current_sos = spu_current_sos;
>        spu_so_ops.bfd_open = spu_bfd_open;
>        spu_so_ops.lookup_lib_global_symbol = spu_lookup_lib_symbol;
> +      spu_so_ops.validate = default_solib_validate;
>      }
>
>    set_solib_ops (gdbarch, &spu_so_ops);
> diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
> index 909dfb7..b434c1f 100644
> --- a/gdb/solib-svr4.c
> +++ b/gdb/solib-svr4.c
> @@ -45,6 +45,9 @@
>  #include "auxv.h"
>  #include "gdb_bfd.h"
>  #include "probe.h"
> +#include "rsp-low.h"
> +
> +#define NOTE_GNU_BUILD_ID_NAME  ".note.gnu.build-id"
>
>  static struct link_map_offsets *svr4_fetch_link_map_offsets (void);
>  static int svr4_have_link_map_offsets (void);
> @@ -970,6 +973,63 @@ svr4_keep_data_in_core (CORE_ADDR vaddr, unsigned long size)
>    return (name_lm >= vaddr && name_lm < vaddr + size);
>  }
>
> +/* Validate SO by comparing build-id from the associated bfd and
> +   corresponding build-id from target memory.  */

Please document that the result is an error message or NULL for success
(including missing build id), and that the caller must free it.
I realize you say so in the docs for the "validate" "method",
but the comment here doesn't mention it is the validate method
(which would be a fine alternative to repeating all the docs
of the method).

> +
> +static char *
> +svr4_validate (const struct so_list *const so)
> +{
> +  const bfd_byte *local_id;
> +  size_t local_idsz;
> +
> +  gdb_assert (so != NULL);
> +
> +  /* Target doesn't support reporting the build ID or the remote shared library
> +     does not have build ID.  */
> +  if (so->build_id == NULL)
> +    return NULL;
> +
> +  /* Build ID may be present in the local file, just GDB is unable to retrieve
> +     it.  As it has been reported by gdbserver it is not FSF gdbserver.  */
> +  if (so->abfd == NULL
> +      || !bfd_check_format (so->abfd, bfd_object))
> +    return NULL;
> +
> +  /* GDB has verified the local file really does not contain the build ID.  */
> +  if (so->abfd->build_id == NULL)
> +    {
> +      char *remote_hex;
> +
> +      remote_hex = alloca (so->build_idsz * 2 + 1);
> +      bin2hex (so->build_id, remote_hex, so->build_idsz);
> +
> +      return xstrprintf (_("remote build ID is %s "
> +                          "but local file does not have build ID"),
> +                        remote_hex);
> +    }
> +
> +  local_id = so->abfd->build_id->data;
> +  local_idsz = so->abfd->build_id->size;
> +
> +  if (so->build_idsz != local_idsz
> +      || memcmp (so->build_id, local_id, so->build_idsz) != 0)
> +    {
> +      char *remote_hex, *local_hex;
> +
> +      remote_hex = alloca (so->build_idsz * 2 + 1);
> +      bin2hex (so->build_id, remote_hex, so->build_idsz);
> +      local_hex = alloca (local_idsz * 2 + 1);
> +      bin2hex (local_id, local_hex, local_idsz);
> +
> +      return xstrprintf (_("remote build ID %s "
> +                          "does not match local build ID %s"),
> +                        remote_hex, local_hex);
> +    }
> +
> +  /* Both build IDs are present and they match.  */
> +  return NULL;
> +}
> +
>  /* Implement the "open_symbol_file_object" target_so_ops method.
>
>     If no open symbol file, attempt to locate and open the main symbol
> @@ -1108,6 +1168,12 @@ svr4_copy_library_list (struct so_list *src)
>        newobj->lm_info = xmalloc (sizeof (struct lm_info));
>        memcpy (newobj->lm_info, src->lm_info, sizeof (struct lm_info));
>
> +      if (newobj->build_id != NULL)
> +       {
> +         newobj->build_id = xmalloc (src->build_idsz);
> +         memcpy (newobj->build_id, src->build_id, src->build_idsz);
> +       }
> +
>        newobj->next = NULL;
>        *link = newobj;
>        link = &newobj->next;
> @@ -1135,6 +1201,9 @@ library_list_start_library (struct gdb_xml_parser *parser,
>    ULONGEST *lmp = xml_find_attribute (attributes, "lm")->value;
>    ULONGEST *l_addrp = xml_find_attribute (attributes, "l_addr")->value;
>    ULONGEST *l_ldp = xml_find_attribute (attributes, "l_ld")->value;
> +  const struct gdb_xml_value *const att_build_id
> +    = xml_find_attribute (attributes, "build-id");
> +  const char *const hex_build_id = att_build_id ? att_build_id->value : NULL;
>    struct so_list *new_elem;
>
>    new_elem = XCNEW (struct so_list);
> @@ -1146,6 +1215,25 @@ library_list_start_library (struct gdb_xml_parser *parser,
>    strncpy (new_elem->so_name, name, sizeof (new_elem->so_name) - 1);
>    new_elem->so_name[sizeof (new_elem->so_name) - 1] = 0;
>    strcpy (new_elem->so_original_name, new_elem->so_name);
> +  if (hex_build_id != NULL)
> +    {
> +      const size_t hex_build_id_len = strlen (hex_build_id);
> +
> +      if (hex_build_id_len > 0 && (hex_build_id_len & 1U) == 0)
> +       {
> +         const size_t build_idsz = hex_build_id_len / 2;
> +
> +         new_elem->build_id = xmalloc (build_idsz);
> +         new_elem->build_idsz = hex2bin (hex_build_id, new_elem->build_id,
> +                                         build_idsz);
> +         if (new_elem->build_idsz != build_idsz)
> +           {

This happens for a malformed build id, right?
A warning would be useful here.
It'd also be nice to have a warning for an odd count.

> +             xfree (new_elem->build_id);
> +             new_elem->build_id = NULL;
> +             new_elem->build_idsz = 0;
> +           }
> +       }
> +    }
>
>    *list->tailp = new_elem;
>    list->tailp = &new_elem->next;
> @@ -1180,6 +1268,7 @@ static const struct gdb_xml_attribute svr4_library_attributes[] =
>    { "lm", GDB_XML_AF_NONE, gdb_xml_parse_attr_ulongest, NULL },
>    { "l_addr", GDB_XML_AF_NONE, gdb_xml_parse_attr_ulongest, NULL },
>    { "l_ld", GDB_XML_AF_NONE, gdb_xml_parse_attr_ulongest, NULL },
> +  { "build-id", GDB_XML_AF_OPTIONAL, NULL, NULL },
>    { NULL, GDB_XML_AF_NONE, NULL, NULL }
>  };
>
> @@ -3258,4 +3347,5 @@ _initialize_svr4_solib (void)
>    svr4_so_ops.keep_data_in_core = svr4_keep_data_in_core;
>    svr4_so_ops.update_breakpoints = svr4_update_solib_event_breakpoints;
>    svr4_so_ops.handle_event = svr4_handle_solib_event;
> +  svr4_so_ops.validate = svr4_validate;
>  }
> diff --git a/gdb/solib-target.c b/gdb/solib-target.c
> index f14363a..13cbd26 100644
> --- a/gdb/solib-target.c
> +++ b/gdb/solib-target.c
> @@ -25,6 +25,7 @@
>  #include "target.h"
>  #include "vec.h"
>  #include "solib-target.h"
> +#include "solib.h"
>
>  /* Private data for each loaded library.  */
>  struct lm_info
> @@ -506,6 +507,7 @@ _initialize_solib_target (void)
>    solib_target_so_ops.in_dynsym_resolve_code
>      = solib_target_in_dynsym_resolve_code;
>    solib_target_so_ops.bfd_open = solib_bfd_open;
> +  solib_target_so_ops.validate = default_solib_validate;
>
>    /* Set current_target_so_ops to solib_target_so_ops if not already
>       set.  */
> diff --git a/gdb/solib.c b/gdb/solib.c
> index 0010c2f..4bd47d0 100644
> --- a/gdb/solib.c
> +++ b/gdb/solib.c
> @@ -523,6 +523,20 @@ solib_bfd_open (char *pathname)
>    return abfd;
>  }
>
> +/* Boolean for command 'set solib-build-id-force'.  */
> +static int solib_build_id_force = 0;
> +
> +/* Implement 'show solib-build-id-force'.  */
> +
> +static void
> +show_solib_build_id_force (struct ui_file *file, int from_tty,
> +                          struct cmd_list_element *c, const char *value)
> +{
> +  fprintf_filtered (file, _("Loading of shared libraries "
> +                           "with non-matching build-id is %s.\n"),
> +                   value);
> +}
> +
>  /* Given a pointer to one of the shared objects in our list of mapped
>     objects, use the recorded name to open a bfd descriptor for the
>     object, build a section table, relocate all the section addresses
> @@ -539,7 +553,7 @@ static int
>  solib_map_sections (struct so_list *so)
>  {
>    const struct target_so_ops *ops = solib_ops (target_gdbarch ());
> -  char *filename;
> +  char *filename, *validate_error;
>    struct target_section *p;
>    struct cleanup *old_chain;
>    bfd *abfd;
> @@ -555,6 +569,27 @@ solib_map_sections (struct so_list *so)
>    /* Leave bfd open, core_xfer_memory and "info files" need it.  */
>    so->abfd = abfd;
>
> +  gdb_assert (ops->validate != NULL);
> +
> +  validate_error = ops->validate (so);
> +  if (validate_error != NULL)
> +    {
> +      if (!solib_build_id_force)
> +       {
> +         warning (_("Shared object \"%s\" could not be validated (%s) and "
> +                    "will be ignored; or use 'set solib-build-id-force'."),
> +                  so->so_name, validate_error);
> +         xfree (validate_error);
> +         gdb_bfd_unref (so->abfd);
> +         so->abfd = NULL;
> +         return 0;
> +       }
> +      warning (_("Shared object \"%s\" could not be validated (%s) "
> +                "but it is being loaded due to 'set solib-build-id-force'."),
> +              so->so_name, validate_error);
> +      xfree (validate_error);
> +    }
> +
>    /* Copy the full path name into so_name, allowing symbol_file_add
>       to find it later.  This also affects the =library-loaded GDB/MI
>       event, and in particular the part of that notification providing
> @@ -631,6 +666,9 @@ clear_so (struct so_list *so)
>       of the symbol file.  */
>    strcpy (so->so_name, so->so_original_name);
>
> +  xfree (so->build_id);
> +  so->build_id = NULL;
> +
>    /* Do the same for target-specific data.  */
>    if (ops->clear_so != NULL)
>      ops->clear_so (so);
> @@ -1662,6 +1700,14 @@ remove_user_added_objfile (struct objfile *objfile)
>      }
>  }
>
> +/* Default implementation does not perform any validation.  */
> +
> +char *
> +default_solib_validate (const struct so_list *const so)
> +{
> +  return NULL; /* No validation.  */
> +}
> +
>  extern initialize_file_ftype _initialize_solib; /* -Wmissing-prototypes */
>
>  void
> @@ -1719,4 +1765,18 @@ PATH and LD_LIBRARY_PATH."),
>                                      reload_shared_libraries,
>                                      show_solib_search_path,
>                                      &setlist, &showlist);
> +
> +  add_setshow_boolean_cmd ("solib-build-id-force", class_support,
> +                          &solib_build_id_force, _("\
> +Set loading of shared libraries with non-matching build-id."), _("\
> +Show loading of shared libraries with non-matching build-id."), _("\
> +Inferior shared library and symbol file may contain unique build-id.\n\
> +If both build-ids are present but they do not match then this setting\n\
> +enables (on) or disables (off) loading of such symbol file.\n\
> +Loading non-matching symbol file may confuse debugging including breakage\n\
> +of backtrace output."),
> +                          NULL,
> +                          show_solib_build_id_force,
> +                          &setlist, &showlist);
> +
>  }
> diff --git a/gdb/solib.h b/gdb/solib.h
> index 336971d..c3bf529 100644
> --- a/gdb/solib.h
> +++ b/gdb/solib.h
> @@ -98,4 +98,8 @@ extern void update_solib_breakpoints (void);
>
>  extern void handle_solib_event (void);
>
> +/* Default validation always returns 1.  */
> +
> +extern char *default_solib_validate (const struct so_list *so);
> +
>  #endif /* SOLIB_H */
> diff --git a/gdb/solist.h b/gdb/solist.h
> index 7021f5c..f0d8653 100644
> --- a/gdb/solist.h
> +++ b/gdb/solist.h
> @@ -75,6 +75,22 @@ struct so_list
>         There may not be just one (e.g. if two segments are relocated
>         differently); but this is only used for "info sharedlibrary".  */
>      CORE_ADDR addr_low, addr_high;
> +
> +    /* Build id in raw format, contains verbatim contents of
> +       .note.gnu.build-id including note header.

Including the note header will be confusing to readers.
Is there a reason to include it?

OTOH, given the above call to hex2bin,
does this really include the note header?

> ... This is actual
> +       BUILD_ID which comes either from the remote target via qXfer
> +       packet or via reading target memory.  Therefore, it may differ
> +       from the build-id of the associated bfd.  In a normal
> +       scenario, this so would soon lose its abfd due to failed
> +       validation.

I can't read this last sentence.
What are you trying to say here?

> +       Reading target memory should be done by following execution view
> +       of the binary (following program headers in the case of ELF).
> +       Computing address from the linking view (following ELF section
> +       headers) may give incorrect build-id memory address despite the
> +       symbols still match.
> +       Such an example is a prelinked vs.  unprelinked i386 ELF file.  */
> +    size_t build_idsz;
> +    gdb_byte *build_id;
>    };
>
>  struct target_so_ops
> @@ -168,6 +184,11 @@ struct target_so_ops
>         NULL, in which case no specific preprocessing is necessary
>         for this target.  */
>      void (*handle_event) (void);
> +
> +    /* Return NULL if SO does match target SO it is supposed to
> +       represent.  Otherwise return string describing why it does not match.
> +       Caller has to free the string.  */
> +    char *(*validate) (const struct so_list *so);
>    };
>
>  /* Free the memory associated with a (so_list *).  */
> --
> 2.1.0
>