Re: [patch] validate binary before use

Jan Kratochvil
2013-04-08T14:35:40Z

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