Re: [PATCH 7/8] Validate symbol file using build-id.

Jan Kratochvil
2013-04-14T14:17:51Z

On Tue, 09 Apr 2013 17:27:44 +0200, Aleksandar Ristovski wrote:
[...]
> --- a/gdb/solib-svr4.c
> +++ b/gdb/solib-svr4.c
[...]
> @@ -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),

I see it is the most easy one how to get the target VMA of the options we were
discussing.

> + 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)));

Like in the previous patch the rounding should be 4 bytes according to the
standard. Not a sizeof of anything.

> + build_id_offs = (sizeof (note->namesz)
> + + sizeof (note->descsz)
> + + sizeof (note->type) + namesz);

I find more simple/clear to use offsetof (which is even more correct due to
possible alignments) but I do not mind either way.

> + build_id = xmalloc (build_idsz);

I would say that build_id should be protected by cleanups but there is
currently no risky function called between here and BUILD_ID comparison below
so I do not mind.

> + memcpy (build_id, note_raw + build_id_offs, build_idsz);
> + }
> + }
> + else
> + warning (_("Malformed %s note\n"), NOTE_GNU_BUILD_ID_NAME);

Again some better identification, vaddr, in fact also PID should be printed in
all these cases.

> +
> + }
> + 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;

GNU Coding Standards require parentheses on multiple-line expressions:
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);

Also it would be apropriate to use a temporary variable
struct elf_build_id *somename = elf_tdata (so->abfd)->build_id;
than to repeat the long expression again and again.
Also such variable should be at the top of this function as there is already
accessed 'elf_tdata (so->abfd)->build_id' at the top of this function.

> +
> + 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);

The hex2bin COUNT parameter is in bytes, not hex characters (divide it by 2).

> + if (new_elem->build_idsz != (hex_build_id_len / 2))

Already written before:

Check it the opposite way:
if (2 * new_elem->build_idsz != hex_build_id_len)

as otherwise odd length of the "build-id" attribute string will not be caught
as invalid.

> + {
> + warning (_("Gdbserver returned invalid hex encoded build_id '%s'"

Use "gdbserver", not capitalized.

> + "(%zu/%zu)\n"),

Already written before:

I would omit this (%zu/%zu) part. Primarily currently %z is not allowed as it
is not compatible with some OSes, there is a plan to import %z printf support
in gdb/gnulib/ but so far there wasn't a need for it.

Besides that you should describe what the two numbers mean otherwise they are
mostly useless. And after all when you already print the XML string the
numbers do not add any more info to it.

> + 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;

Already written before:
I am not completely sure warning cannot throw, better to do these cleanups
before the warning call (moreover when %zu/%zu will no longer be printed).

> + }
> + }
> + }
>
> *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);

Maybe the warning should by printed by svr4_validate stating the build-id does
not match (even possibly printing the both build-ids). And then sure one can
remove the warning here.

> + 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);
>

I already wrote:

The problem is there is:
xfree (so->build_id);
in free_so() but it should be in free_so_symbols instead. free_so_symbols is
called also from reload_shared_libraries_1 where so_list->abfd can change.
Then obviously one should also set so->build_id = NULL there.

I was thinking making the BUILD_ID data private so solib-svr4.c but that is
currently not possible, lm_info is private for solib-svr4.c but that has
lifetime of so_list, not the lifetime of so_list->abfd.

> @@ -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
[...]

Thanks,
Jan