From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1288 invoked by alias); 8 Apr 2013 14:35:40 -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 1275 invoked by uid 89); 8 Apr 2013 14:35:40 -0000 X-Spam-SWARE-Status: No, score=-7.4 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_CP,TW_CS autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Mon, 08 Apr 2013 14:35:20 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r38EZGni013352 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 8 Apr 2013 10:35:17 -0400 Received: from host2.jankratochvil.net (ovpn-116-44.ams2.redhat.com [10.36.116.44]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r38EZB8c010820 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Mon, 8 Apr 2013 10:35:14 -0400 Date: Mon, 08 Apr 2013 18:32:00 -0000 From: Jan Kratochvil To: Aleksandar Ristovski Cc: gdb-patches@sourceware.org Subject: Re: [patch] validate binary before use Message-ID: <20130408143511.GA14652@host2.jankratochvil.net> References: <514C58B2.6090701@qnx.com> <20130328183727.GA14798@host2.jankratochvil.net> <515B0632.1040502@qnx.com> <20130402165306.GA9479@host2.jankratochvil.net> <515B12D1.7050505@qnx.com> <20130403180917.GA6102@host2.jankratochvil.net> <515CDF7F.5020403@qnx.com> <20130404081302.GA4099@host2.jankratochvil.net> <515D7A09.5010305@qnx.com> <515ECB77.60401@qnx.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <515ECB77.60401@qnx.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes X-SW-Source: 2013-04/txt/msg00189.txt.bz2 Hi Aleksandar, according to IRC you are going to send a new patchset so just replying with a review what I have now. On Fri, 05 Apr 2013 15:02:47 +0200, Aleksandar Ristovski wrote: > @@ -2274,10 +2321,53 @@ static void > svr4_relocate_section_addresses (struct so_list *so, > struct target_section *sec) > { > + ULONGEST bfd_sect_size; Move this variable to the inner block. > + > sec->addr = svr4_truncate_ptr (sec->addr + lm_addr_check (so, > sec->bfd)); > sec->endaddr = svr4_truncate_ptr (sec->endaddr + lm_addr_check (so, > sec->bfd)); > + > + if (so->build_id == NULL) If you did negative conditional here with 'return;' the block below would not get so right making it less readable. Not required, just suggested. > + { > + /* Get build_id from NOTE_GNU_BUILD_ID_NAME section. */ > + bfd_sect_size = bfd_get_section_size (sec->the_bfd_section); > + > + if ((sec->the_bfd_section->flags & SEC_LOAD) == SEC_LOAD > + && bfd_sect_size != 0 > + && strcmp (bfd_section_name (sec->bfd, sec->the_bfd_section), > + NOTE_GNU_BUILD_ID_NAME) == 0) If you did negative conditional here with 'return;' the block below would not get so right making it less readable. Not required, just suggested. > + { > + CORE_ADDR build_id_offs; > + const enum bfd_endian byte_order > + = gdbarch_byte_order (target_gdbarch ()); > + gdb_byte *const note_raw = xmalloc (bfd_sect_size); > + const Elf_External_Note *const note = (void *) note_raw; I would say this is a strict aliasing violation (char * -> type *). Or why it is not? > + > + if (target_read_memory (sec->addr, note_raw, bfd_sect_size) == 0) > + { > + const ULONGEST descsz > + = extract_unsigned_integer ((gdb_byte *) note->descsz, 4, > + byte_order); > + ULONGEST namesz; > + > + namesz = extract_unsigned_integer ((gdb_byte *) note->namesz, 4, > + byte_order); > + if (descsz == elf_tdata (so->abfd)->build_id->size) > + { > + /* Rounded to next sizeof (ElfXX_Word) which happens > + to be 4 for both Elf32 and Elf64. */ > + namesz = (namesz + 3) & ~((ULONGEST) 3); > + build_id_offs = sizeof (note->namesz) + sizeof (note->descsz) > + + sizeof (note->type) + namesz; You use 4 above but here you use sizeof (note->namesz) (and descsz). Choose one in both cases the same. Also the GNU Coding Standards require parentheses on multi-line expressions AFAIK to workaround Emacs bug, therefore: build_id_offs = (sizeof (note->namesz) + sizeof (note->descsz) + sizeof (note->type) + namesz); The note name is not verified here to be "GNU\0". > + so->build_id = xmalloc (descsz); > + so->build_idsz = descsz; > + memcpy (so->build_id, note_raw + build_id_offs, descsz); > + } > + } > + xfree (note_raw); > + } > + } > } > > > @@ -2458,4 +2548,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-svr4.h b/gdb/solib-svr4.h > index 66a06e1..ba17dbe 100644 > --- a/gdb/solib-svr4.h > +++ b/gdb/solib-svr4.h > @@ -20,6 +20,9 @@ > #ifndef SOLIB_SVR4_H > #define SOLIB_SVR4_H > > +#define DYNAMIC_NAME ".dynamic" I already wrote in previous mail: I do not see used it anywhere. > +#define NOTE_GNU_BUILD_ID_NAME ".note.gnu.build-id" I already wrote in previous mail: It is used only in solib-svr4.c so it should be in that file. This happens multiple times with your mails. If you do not agree with it then reply why. > + > struct objfile; > struct target_so_ops; > [...] > @@ -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. */ I already wrote in the last mail: Formatting: /* No validation. */ return 1; Thanks, Jan