Re: [patch] validate binary before use

[Jan Kratochvil <jan.kratochvil@redhat.com>]

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);
> +	}
> +    }
>  }
>  \f
>  
> @@ -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