From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15248 invoked by alias); 10 Apr 2013 15:01:13 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 15194 invoked by uid 89); 10 Apr 2013 15:01:13 -0000 X-Spam-SWARE-Status: No, score=-2.7 required=5.0 tests=AWL,BAYES_00,FSL_HELO_BARE_IP_2,KHOP_THREADED,RCVD_IN_DNSWL_NONE,RCVD_NUMERIC_HELO,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_CP autolearn=ham version=3.3.1 Received: from plane.gmane.org (HELO plane.gmane.org) (80.91.229.3) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Wed, 10 Apr 2013 15:01:10 +0000 Received: from list by plane.gmane.org with local (Exim 4.69) (envelope-from ) id 1UPwWI-0003cb-9O for gdb-patches@sourceware.org; Wed, 10 Apr 2013 17:01:06 +0200 Received: from 209.226.137.106 ([209.226.137.106]) by main.gmane.org with esmtp (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Wed, 10 Apr 2013 17:01:06 +0200 Received: from aristovski by 209.226.137.106 with local (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Wed, 10 Apr 2013 17:01:06 +0200 To: gdb-patches@sourceware.org From: Aleksandar Ristovski Subject: Re: [PATCH 7/8] Validate symbol file using build-id. Date: Wed, 10 Apr 2013 22:35:00 -0000 Message-ID: <51657EAB.3090604@qnx.com> References: <1365521265-28870-1-git-send-email-ARistovski@qnx.com> <1365521265-28870-8-git-send-email-ARistovski@qnx.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: gdb-patches@sourceware.org, jan.kratochvil@redhat.com User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130308 Thunderbird/17.0.4 In-Reply-To: <1365521265-28870-8-git-send-email-ARistovski@qnx.com> X-SW-Source: 2013-04/txt/msg00278.txt.bz2 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 *). */ > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14778 invoked by alias); 10 Apr 2013 15:01:10 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 14768 invoked by uid 89); 10 Apr 2013 15:01:09 -0000 X-Spam-SWARE-Status: No, score=-4.6 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,TW_CP autolearn=ham version=3.3.1 Received: from na3sys009aog125.obsmtp.com (HELO na3sys009aog125.obsmtp.com) (74.125.149.153) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Wed, 10 Apr 2013 15:01:07 +0000 Received: from mx20.qnx.com ([72.1.200.103]) (using TLSv1) by na3sys009aob125.postini.com ([74.125.148.12]) with SMTP ID DSNKUWV+r5hqoPM8wn7y7na95juDCipAdWDG@postini.com; Wed, 10 Apr 2013 08:01:07 PDT Received: by mx20.qnx.com (Postfix, from userid 500) id 7564C20E45; Wed, 10 Apr 2013 11:01:02 -0400 (EDT) Received: from exhts.ott.qnx.com (exch1 [10.222.2.137]) (using TLSv1 with cipher AES128-SHA (128/128 bits)) (No client certificate requested) by mx20.qnx.com (Postfix) with ESMTPS id 6F14020A8B; Wed, 10 Apr 2013 11:01:00 -0400 (EDT) Received: from [10.222.96.215] (10.222.2.5) by EXCH1.ott.qnx.com (10.222.2.137) with Microsoft SMTP Server id 14.2.318.4; Wed, 10 Apr 2013 11:01:00 -0400 Message-ID: <51657EAB.3090604@qnx.com> Date: Wed, 10 Apr 2013 19:58:00 -0000 From: Aleksandar Ristovski User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130308 Thunderbird/17.0.4 MIME-Version: 1.0 Newsgroups: gmane.comp.gdb.patches To: Aleksandar Ristovski CC: , Subject: Re: [PATCH 7/8] Validate symbol file using build-id. References: <1365521265-28870-1-git-send-email-ARistovski@qnx.com> <1365521265-28870-8-git-send-email-ARistovski@qnx.com> In-Reply-To: <1365521265-28870-8-git-send-email-ARistovski@qnx.com> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2013-04/txt/msg00277.txt.bz2 Message-ID: <20130410195800.Yk3vtKmiJWNckYEdCWEu1oKAxNVJ0LzJBXRKITF5HJQ@z> 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 *). */ >