[PATCH 7/8] Validate symbol file using build-id.
Aleksandar Ristovski
aristovski@qnx.com
Wed Apr 10 22:35:00 GMT 2013
Minor but important change in svr4_validate:
if target memory read succeeded, but the note read doesn't look like
build-id note, this means the file does not match. Current patch would
say it matches.
Also, the warning emitted in this case is misleading (thus removed).
I can repost the patch, but the diff is this:
@@ -952,8 +952,16 @@ svr4_validate (const struct so_list *const so)
memcpy (build_id, note_raw + build_id_offs,
build_idsz);
}
}
- else
- warning (_("Malformed %s note\n"), NOTE_GNU_BUILD_ID_NAME);
+
+ if (build_id == NULL)
+ {
+ /* If we are here, it means target memory read succeeded
+ but note was not where it was expected according to the
+ abfd. Allow the logic below to perform the check
+ with an impossible build-id and fail validation. */
+ build_idsz = 0;
+ build_id = xstrdup ("");
+ }
}
do_cleanups (cleanups);
On 13-04-09 11:27 AM, Aleksandar Ristovski wrote:
> Notable changes: gear for fetching build-id from target memory that
> was in the previous patcset in svr4_relocate_section_addresses is moved
> back to svr4_validate. This code path is taken only if build-id was
> not fetched via TARGET_OBJECT_LIBRARIES_SVR4, but build-id exists
> in the so.
>
> * solib-darwin.c (_initialize_darwin_solib): Assign validate value.
> * solib-dsbt.c (_initialize_dsbt_solib): Ditto.
> * solib-frv.c (_initialize_frv_solib): Ditto.
> * solib-ia64-hpux.c (ia64_hpux_target_so_ops): Ditto.
> * solib-irix.c (_initialize_irix_solib): Ditto.
> * solib-osf.c (_initialize_osf_solib): Ditto.
> * solib-pa64.c (_initialize_pa64_solib): Ditto.
> * solib-som.c (_initialize_som_solib): Ditto.
> * solib-spu.c (set_spu_solib_ops): Ditto.
> * solib-sunos.c (_initialize_sunos_solib): Ditto.
> * solib-svr4.c (NOTE_GNU_BUILD_ID_NAME): New define.
> (svr4_validate): New function.
> (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_map_sections): Use ops->validate.
> (free_so): Free build_id.
> (solib_validate): New function.
> * solib.h (solib_validate): New declaration.
> * solist.h (so_list): New fields 'build_idsz' and 'build_id'.
> (target_so_ops): New field 'validate'.
> ---
> gdb/solib-darwin.c | 1 +
> gdb/solib-dsbt.c | 1 +
> gdb/solib-frv.c | 1 +
> gdb/solib-ia64-hpux.c | 1 +
> gdb/solib-irix.c | 1 +
> gdb/solib-osf.c | 1 +
> gdb/solib-pa64.c | 1 +
> gdb/solib-som.c | 1 +
> gdb/solib-spu.c | 1 +
> gdb/solib-sunos.c | 1 +
> gdb/solib-svr4.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++++
> gdb/solib-target.c | 2 +
> gdb/solib.c | 20 ++++++++
> gdb/solib.h | 4 ++
> gdb/solist.h | 14 ++++++
> 15 files changed, 180 insertions(+)
>
> diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c
> index b9a4be1..ff57016 100644
> --- a/gdb/solib-darwin.c
> +++ b/gdb/solib-darwin.c
> @@ -647,4 +647,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 ea2acd1..9728470 100644
> --- a/gdb/solib-dsbt.c
> +++ b/gdb/solib-dsbt.c
> @@ -1182,6 +1182,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 57e418f..5621b2a 100644
> --- a/gdb/solib-frv.c
> +++ b/gdb/solib-frv.c
> @@ -1182,6 +1182,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-ia64-hpux.c b/gdb/solib-ia64-hpux.c
> index 67085d7..6fb146c 100644
> --- a/gdb/solib-ia64-hpux.c
> +++ b/gdb/solib-ia64-hpux.c
> @@ -686,6 +686,7 @@ ia64_hpux_target_so_ops (void)
> ops->open_symbol_file_object = ia64_hpux_open_symbol_file_object;
> ops->in_dynsym_resolve_code = ia64_hpux_in_dynsym_resolve_code;
> ops->bfd_open = solib_bfd_open;
> + ops->validate = default_solib_validate;
>
> return ops;
> }
> diff --git a/gdb/solib-irix.c b/gdb/solib-irix.c
> index af3e7d6..871fb19 100644
> --- a/gdb/solib-irix.c
> +++ b/gdb/solib-irix.c
> @@ -652,4 +652,5 @@ _initialize_irix_solib (void)
> irix_so_ops.open_symbol_file_object = irix_open_symbol_file_object;
> irix_so_ops.in_dynsym_resolve_code = irix_in_dynsym_resolve_code;
> irix_so_ops.bfd_open = solib_bfd_open;
> + irix_so_ops.validate = default_solib_validate;
> }
> diff --git a/gdb/solib-osf.c b/gdb/solib-osf.c
> index d05c5c1..5b1cd0b 100644
> --- a/gdb/solib-osf.c
> +++ b/gdb/solib-osf.c
> @@ -633,6 +633,7 @@ _initialize_osf_solib (void)
> osf_so_ops.open_symbol_file_object = osf_open_symbol_file_object;
> osf_so_ops.in_dynsym_resolve_code = osf_in_dynsym_resolve_code;
> osf_so_ops.bfd_open = solib_bfd_open;
> + osf_so_ops.validate = default_solib_validate;
>
> /* FIXME: Don't do this here. *_gdbarch_init() should set so_ops. */
> current_target_so_ops = &osf_so_ops;
> diff --git a/gdb/solib-pa64.c b/gdb/solib-pa64.c
> index f646cfb..795bcda 100644
> --- a/gdb/solib-pa64.c
> +++ b/gdb/solib-pa64.c
> @@ -621,6 +621,7 @@ _initialize_pa64_solib (void)
> pa64_so_ops.open_symbol_file_object = pa64_open_symbol_file_object;
> pa64_so_ops.in_dynsym_resolve_code = pa64_in_dynsym_resolve_code;
> pa64_so_ops.bfd_open = solib_bfd_open;
> + pa64_so_ops.validate = default_solib_validate;
>
> memset (&dld_cache, 0, sizeof (dld_cache));
> }
> diff --git a/gdb/solib-som.c b/gdb/solib-som.c
> index ff7fbaa..cc2d344 100644
> --- a/gdb/solib-som.c
> +++ b/gdb/solib-som.c
> @@ -811,6 +811,7 @@ _initialize_som_solib (void)
> som_so_ops.open_symbol_file_object = som_open_symbol_file_object;
> som_so_ops.in_dynsym_resolve_code = som_in_dynsym_resolve_code;
> som_so_ops.bfd_open = solib_bfd_open;
> + som_so_ops.validate = default_solib_validate;
> }
>
> void
> diff --git a/gdb/solib-spu.c b/gdb/solib-spu.c
> index 7be5232..4cc343b 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-sunos.c b/gdb/solib-sunos.c
> index 5863fc2..71c8ee3 100644
> --- a/gdb/solib-sunos.c
> +++ b/gdb/solib-sunos.c
> @@ -738,6 +738,7 @@ _initialize_sunos_solib (void)
> sunos_so_ops.open_symbol_file_object = open_symbol_file_object;
> sunos_so_ops.in_dynsym_resolve_code = sunos_in_dynsym_resolve_code;
> sunos_so_ops.bfd_open = solib_bfd_open;
> + sunos_so_ops.validate = default_solib_validate;
>
> /* FIXME: Don't do this here. *_gdbarch_init() should set so_ops. */
> current_target_so_ops = &sunos_so_ops;
> diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
> index bb2a4e9..90e421b 100644
> --- a/gdb/solib-svr4.c
> +++ b/gdb/solib-svr4.c
> @@ -47,6 +47,8 @@
> #include "exceptions.h"
> #include "gdb_bfd.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);
> static void svr4_relocate_main_executable (void);
> @@ -871,6 +873,109 @@ 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. */
> +
> +static int
> +svr4_validate (const struct so_list *const so)
> +{
> + gdb_byte *build_id;
> + size_t build_idsz;
> +
> + gdb_assert (so != NULL);
> +
> + if (so->abfd == NULL)
> + return 1;
> +
> + if (!bfd_check_format (so->abfd, bfd_object)
> + || bfd_get_flavour (so->abfd) != bfd_target_elf_flavour
> + || elf_tdata (so->abfd)->build_id == NULL)
> + return 1;
> +
> + build_id = so->build_id;
> + build_idsz = so->build_idsz;
> +
> + if (build_id == NULL)
> + {
> + /* Get build_id from NOTE_GNU_BUILD_ID_NAME section.
> + This is a fallback mechanism for targets that do not
> + implement TARGET_OBJECT_SOLIB_SVR4. */
> +
> + const asection *const asec
> + = bfd_get_section_by_name (so->abfd, NOTE_GNU_BUILD_ID_NAME);
> + ULONGEST bfd_sect_size;
> +
> + if (asec == NULL)
> + return 1;
> +
> + bfd_sect_size = bfd_get_section_size (asec);
> +
> + if ((asec->flags & SEC_LOAD) == SEC_LOAD
> + && bfd_sect_size != 0
> + && strcmp (bfd_section_name (asec->bfd, asec),
> + NOTE_GNU_BUILD_ID_NAME) == 0)
> + {
> + const enum bfd_endian byte_order
> + = gdbarch_byte_order (target_gdbarch ());
> + Elf_External_Note *const note = xmalloc (bfd_sect_size);
> + gdb_byte *const note_raw = (void *) note;
> + struct cleanup *cleanups = make_cleanup (xfree, note);
> +
> + if (target_read_memory (bfd_get_section_vma (so->abfd, asec)
> + + lm_addr_check (so, so->abfd),
> + note_raw, bfd_sect_size) == 0)
> + {
> + build_idsz
> + = extract_unsigned_integer ((gdb_byte *) note->descsz,
> + sizeof (note->descsz),
> + byte_order);
> +
> + if (build_idsz == elf_tdata (so->abfd)->build_id->size)
> + {
> + const char gnu[4] = "GNU\0";
> +
> + if (memcmp (note->name, gnu, 4) == 0)
> + {
> + ULONGEST namesz
> + = extract_unsigned_integer ((gdb_byte *) note->namesz,
> + sizeof (note->namesz),
> + byte_order);
> + CORE_ADDR build_id_offs;
> +
> + /* Rounded to next sizeof (ElfXX_Word). */
> + namesz = ((namesz + (sizeof (note->namesz) - 1))
> + & ~((ULONGEST) (sizeof (note->namesz) - 1)));
> + build_id_offs = (sizeof (note->namesz)
> + + sizeof (note->descsz)
> + + sizeof (note->type) + namesz);
> + build_id = xmalloc (build_idsz);
> + memcpy (build_id, note_raw + build_id_offs, build_idsz);
> + }
> + }
> + else
> + warning (_("Malformed %s note\n"), NOTE_GNU_BUILD_ID_NAME);
> +
> + }
> + do_cleanups (cleanups);
> + }
> + }
> +
> + if (build_id != NULL)
> + {
> + const int match
> + = elf_tdata (so->abfd)->build_id->size == build_idsz
> + && memcmp (build_id, elf_tdata (so->abfd)->build_id->data,
> + elf_tdata (so->abfd)->build_id->size) == 0;
> +
> + if (build_id != so->build_id)
> + xfree (build_id);
> +
> + return match;
> + }
> +
> + return 1;
> +}
> +
> /* Implement the "open_symbol_file_object" target_so_ops method.
>
> If no open symbol file, attempt to locate and open the main symbol
> @@ -999,6 +1104,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 = XZALLOC (struct so_list);
> @@ -1010,6 +1118,26 @@ 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)
> + {
> + new_elem->build_id = xmalloc (hex_build_id_len / 2);
> + new_elem->build_idsz = hex2bin (hex_build_id, new_elem->build_id,
> + hex_build_id_len);
> + if (new_elem->build_idsz != (hex_build_id_len / 2))
> + {
> + warning (_("Gdbserver returned invalid hex encoded build_id '%s'"
> + "(%zu/%zu)\n"),
> + hex_build_id, hex_build_id_len, new_elem->build_idsz);
> + xfree (new_elem->build_id);
> + new_elem->build_id = NULL;
> + new_elem->build_idsz = 0;
> + }
> + }
> + }
>
> *list->tailp = new_elem;
> list->tailp = &new_elem->next;
> @@ -1044,6 +1172,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 }
> };
>
> @@ -2458,4 +2587,5 @@ _initialize_svr4_solib (void)
> svr4_so_ops.lookup_lib_global_symbol = elf_lookup_lib_symbol;
> svr4_so_ops.same = svr4_same;
> svr4_so_ops.keep_data_in_core = svr4_keep_data_in_core;
> + svr4_so_ops.validate = svr4_validate;
> }
> diff --git a/gdb/solib-target.c b/gdb/solib-target.c
> index d897bc0..ed82218 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"
>
> #include "gdb_string.h"
>
> @@ -500,6 +501,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 8129c0f..20c709e 100644
> --- a/gdb/solib.c
> +++ b/gdb/solib.c
> @@ -471,6 +471,17 @@ solib_map_sections (struct so_list *so)
> error (_("Shared library file name is too long."));
> strcpy (so->so_name, bfd_get_filename (abfd));
>
> + gdb_assert (ops->validate != NULL);
> +
> + if (!ops->validate (so))
> + {
> + warning (_("Shared object \"%s\" could not be validated "
> + "and will be ignored."), so->so_name);
> + gdb_bfd_unref (so->abfd);
> + so->abfd = NULL;
> + return 0;
> + }
> +
> if (build_section_table (abfd, &so->sections, &so->sections_end))
> {
> error (_("Can't find the file sections in `%s': %s"),
> @@ -551,6 +562,7 @@ free_so (struct so_list *so)
> {
> struct target_so_ops *ops = solib_ops (target_gdbarch ());
>
> + xfree (so->build_id);
> free_so_symbols (so);
> ops->free_so (so);
>
> @@ -1448,6 +1460,14 @@ gdb_bfd_lookup_symbol (bfd *abfd,
> return symaddr;
> }
>
> +/* Default implementation does not perform any validation. */
> +
> +int
> +default_solib_validate (const struct so_list *const so)
> +{
> + return 1; /* No validation. */
> +}
> +
> extern initialize_file_ftype _initialize_solib; /* -Wmissing-prototypes */
>
> void
> diff --git a/gdb/solib.h b/gdb/solib.h
> index b811866..670949a 100644
> --- a/gdb/solib.h
> +++ b/gdb/solib.h
> @@ -90,4 +90,8 @@ extern CORE_ADDR gdb_bfd_lookup_symbol_from_symtab (bfd *abfd,
> void *),
> void *data);
>
> +/* Default validation always returns 1. */
> +
> +extern int default_solib_validate (const struct so_list *so);
> +
> #endif /* SOLIB_H */
> diff --git a/gdb/solist.h b/gdb/solist.h
> index f784fc3..72e003d 100644
> --- a/gdb/solist.h
> +++ b/gdb/solist.h
> @@ -75,6 +75,16 @@ 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. 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. */
> + size_t build_idsz;
> + gdb_byte *build_id;
> };
>
> struct target_so_ops
> @@ -148,6 +158,10 @@ struct target_so_ops
> core file (in particular, for readonly sections). */
> int (*keep_data_in_core) (CORE_ADDR vaddr,
> unsigned long size);
> +
> + /* Return 0 if SO does not match target SO it is supposed to
> + represent. Return 1 otherwise. */
> + int (*validate) (const struct so_list *so);
> };
>
> /* Free the memory associated with a (so_list *). */
>
More information about the Gdb-patches
mailing list