This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] validate binary before use


On Fri, 22 Mar 2013 14:12:18 +0100, Aleksandar Ristovski wrote:
> (depends on the gdbserver changes:
> http://sourceware.org/ml/gdb-patches/2013-03/msg00838.html)
> 
> ChangeLog:
> 
> 
> 	* mips-linux-tdep.c (mips_linux_init_abi): Assign validate value.
> 	* ppc-linux-tdep.c (ppc_linux_init_abi): Ditto.
> 	* solib-darwin.c (_initialize_darwin_solib): Ditto.
> 	* 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 (lm_addr_check): Add const for 'so' type.
> 	(svr4_validate_build_id): New function.
> 	(svr4_validate): New function.
> 	(library_list_start_library): Parse 'build-id' attribute.
> 	(svr4_library_pattributes): Add 'build-id' attribute.
> 	(_initialize_svr4_solib): Assign validate value.
> 	* solib-svr4.h (DYNAMIC_NAME): New define.
> 	(NOTE_GNU_BUILD_ID_NAME): New define.
> 	* 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'.
> 
> 
> 
> Test ChangeLog:
> 	* gdb.base/solib-mismatch-lib.c: New file.
> 	* gdb.base/solib-mismatch-libmod.c: New file.
> 	* gdb.base/solib-mismatch.c: New file.
> 	* gdb.base/solib-mismatch.exp: New file.
> 
> 
> Thanks,
> 
> Aleksandar
> 

> diff --git a/gdb/mips-linux-tdep.c b/gdb/mips-linux-tdep.c
> index 2ebe2fe..2e950b6 100644
> --- a/gdb/mips-linux-tdep.c
> +++ b/gdb/mips-linux-tdep.c
> @@ -1495,6 +1495,8 @@ mips_linux_init_abi (struct gdbarch_info info,
>        mips_svr4_so_ops.in_dynsym_resolve_code
>  	= mips_linux_in_dynsym_resolve_code;
>      }
> +  if (mips_svr4_so_ops.validate == NULL)
> +    mips_svr4_so_ops.validate = solib_validate;

I do not see why this override should be done.  There is above:
      mips_svr4_so_ops = svr4_so_ops;

so unless you have a specific reason why the verification cannot work for MIPS
- and such reason should be put in a comment there - just remove these two
'+' linse.


>    set_solib_ops (gdbarch, &mips_svr4_so_ops);
>  
>    set_gdbarch_write_pc (gdbarch, mips_linux_write_pc);
> diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c
> index 0fc6fe0..92dc41a 100644
> --- a/gdb/ppc-linux-tdep.c
> +++ b/gdb/ppc-linux-tdep.c
> @@ -1324,6 +1324,8 @@ ppc_linux_init_abi (struct gdbarch_info info,
>  	  powerpc_so_ops.in_dynsym_resolve_code =
>  	    powerpc_linux_in_dynsym_resolve_code;
>  	}
> +      if (powerpc_so_ops.validate == NULL)
> +	powerpc_so_ops.validate = solib_validate;

Likewise.


>        set_solib_ops (gdbarch, &powerpc_so_ops);
>  
>        set_gdbarch_skip_solib_resolver (gdbarch, glibc_skip_solib_resolver);
> diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c
> index b9a4be1..b588f86 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 = solib_validate;
>  }
> diff --git a/gdb/solib-dsbt.c b/gdb/solib-dsbt.c
> index ea2acd1..c5f84f3 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 = 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..e8de6ed 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 = 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..b5d3b6b 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 = solib_validate;
>  
>    return ops;
>  }
> diff --git a/gdb/solib-irix.c b/gdb/solib-irix.c
> index af3e7d6..642e973 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 = solib_validate;
>  }
> diff --git a/gdb/solib-osf.c b/gdb/solib-osf.c
> index d05c5c1..f7298e3 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 = 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..06749d0 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 = solib_validate;
>  
>    memset (&dld_cache, 0, sizeof (dld_cache));
>  }
> diff --git a/gdb/solib-som.c b/gdb/solib-som.c
> index ff7fbaa..0d68aac 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 = solib_validate;
>  }
>  
>  void
> diff --git a/gdb/solib-spu.c b/gdb/solib-spu.c
> index 7be5232..94b9a36 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 = solib_validate;

In whis case the override of SVR4_SO_OPS.VALIDATE seems as a safe bet, Ulrich
Weigand could test if it works with SPU but for the initial check-in I find it
OK as you wrote it.


>      }
>  
>    set_solib_ops (gdbarch, &spu_so_ops);
> diff --git a/gdb/solib-sunos.c b/gdb/solib-sunos.c
> index 5863fc2..fec2e9a 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 = 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 0f70097..9dd9cab 100644
> --- a/gdb/solib-svr4.c
> +++ b/gdb/solib-svr4.c
> @@ -189,7 +189,7 @@ has_lm_dynamic_from_link_map (void)
>  }
>  
>  static CORE_ADDR
> -lm_addr_check (struct so_list *so, bfd *abfd)
> +lm_addr_check (const struct so_list *so, bfd *abfd)

Such change is OK although it needs to be checked in separately.


>  {
>    if (!so->lm_info->l_addr_p)
>      {
> @@ -871,6 +871,87 @@ svr4_keep_data_in_core (CORE_ADDR vaddr, unsigned long size)
>    return (name_lm >= vaddr && name_lm < vaddr + size);
>  }
>  
> +
> +/* If build-id exists, compare it with target in-memory contents.
> +   Return 1 if they match, 0 if they don't.
> +   If there was no build-id, return 1 (could not be verified).  */
> +
> +static int
> +svr4_validate_build_id (const struct so_list *const so)

If you are used to it then OK but for GDB one does not have to make 'const'
even the variables themselves ('so'), that does not bring much benefits.
This is everywhere in the patch.


> +{
> +  asection *asect;
> +  int res = 1;
> +  size_t size;

There are multiple sizes around, please call it "asect_size" or somehow
similar to see this "size" is for the section.


> +  const CORE_ADDR l_addr = lm_addr_check (so, so->abfd);

Move this variable to the inner block where it is used, its computation
possibly would not be used.


> +
> +  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;
> +
> +  asect = bfd_get_section_by_name (so->abfd, NOTE_GNU_BUILD_ID_NAME);
> +
> +  if (!asect || !so->lm_info->l_addr_p)

Coding style for pointer comparison according to doc/gdbinte.texinfo:
	if (asect == NULL || !so->lm_info->l_addr_p)

But that l_addr_p check is not needed, drop it, lm_addr_check cannot fail,
l_addr_p is only internal field for lm_addr_check.


> +    {
> +      if (info_verbose)
> +	  warning (_("Could not verify '%s' section.\n"),
> +		   NOTE_GNU_BUILD_ID_NAME);
> +      return 1;
> +    }
> +
> +  size = bfd_get_section_size (asect);
> +
> +  if ((asect->flags & SEC_LOAD) == SEC_LOAD && size != 0)
> +    {
> +      gdb_byte *build_id = so->build_id;
> +      size_t build_idsz = so->build_idsz;

You never set so->build_id anywhere so it will be always NULL.

But in fact I do not see why there exists so->build_id at all.  It is only
verified once when loading the library and then if it remains loaded one can
pick the build_id from elf_tdata (so->abfd)->build_id which has been already
verified as matching.

So just drop so->build_id, it is a duplcate to elf_tdata (so->abfd)->build_id.



> +      gdb_byte *const data = xmalloc (size);
> +      struct cleanup *const cleanups = make_cleanup (xfree, data);
> +      /* Zero based vma, after undoing link script non zero base
> +	 and/or prelinked rebasing.  */

I do not understand this comment but as the code will change I prefer
a removal of the comment or a new/different comment.


> +      const CORE_ADDR sect_lma = l_addr + bfd_section_vma (so->abfd, asect);

"lma" is irrelevant here, it needs to be VMA as you do runtime
target_read_memory for that address.

I won't argue if the expression is right or wrong but it at least looks as
a duplication of computation done for target_section->addr:

 * Move the 'ops->validate (se)' call in solib_map_sections after the loop
   for (p = so->sections; p < so->sections_end; p++)

 * Then just use here the VMS address from so->sections but one needs to find
   it first:
     struct target_section *p;
     for (p = solib->sections; p < solib->sections_end; p++)
       if (p->the_bfd_section == asect)
         break;
     use p->addr


> +
> +      /* Relocated or not, contents will be corectly loaded from
> +	 the file by bfd library.  */
> +      bfd_get_section_contents ((bfd *) so->abfd, asect, data, 0, size);

Remove the bfd_get_section_contents call completely,
use elf_tdata (so->abfd)->build_id , there it is already read in and parsed.


> +
> +      if (build_id == NULL)

Here could be a comment - if it was intended so:

/* Even if gdbserver failed to find the build-id read it by
   target_read_memory.  */


> +	{
> +	  build_idsz = size;
> +	  build_id = xmalloc (size);
> +	  make_cleanup (xfree, build_id);

If you start to store the build-id to so->build_id then the xfree is no longer
appropriate here.


> +	  if (target_read_memory (sect_lma, build_id, size) != 0)
> +	    {
> +	      build_id = NULL;
> +	      res = -1;

Return value -1 is not documented for this function; but see below.


> +	    }
> +	}
> +
> +      if (build_id != NULL)
> +	res = size == build_idsz
> +	      && memcmp (build_id, data, size) == 0;

The line does not need wrapping, it is under 80 columns:
	res = size == build_idsz && memcmp (build_id, data, size) == 0;


> +
> +      if (res == -1 && info_verbose)
> +	warning (_("Could not verify section %s\n"), NOTE_GNU_BUILD_ID_NAME);
> +
> +      do_cleanups (cleanups);
> +    }
> +
> +  return res != 0;

When the variable is called 'res' then one expects it will be returned as is,
therefore 'return res;'.


> +}
> +
> +/* Validate SO by checking whether opened file matches
> +   in-memory object.  */
> +
> +static int
> +svr4_validate (const struct so_list *const so)
> +{
> +  gdb_assert (so != NULL);
> +  gdb_assert (so->abfd != NULL);
> +
> +  return svr4_validate_build_id (so);
> +}

I do not see why there is this wrapper svr4_validate, you could just move
those two gdb_assert to svr4_validate_build_id and use directly
svr4_validate_build_id, it has no other caller anyway.


> +
>  /* Implement the "open_symbol_file_object" target_so_ops method.
>  
>     If no open symbol file, attempt to locate and open the main symbol
> @@ -999,7 +1080,11 @@ 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;
> +  size_t hex_build_id_len;

Sometimes you call it "build_idsz" and sometimes "build_id_len", unify it.
(I prefer the latter.)


>  
>    new_elem = XZALLOC (struct so_list);
>    new_elem->lm_info = XZALLOC (struct lm_info);
> @@ -1010,6 +1095,21 @@ 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 && (hex_build_id_len = strlen (hex_build_id)) > 0)

Do not use assignment this way, make it a separate line, see GNU Coding
Standards:
	Try to avoid assignments inside if-conditions


> +    {
> +      new_elem->build_id = xmalloc (hex_build_id_len / 2 + 1);

Why the '+ 1' here?  NEW_ELEM->BUILD_ID is in binary form so there is no '\0'
terminator.


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

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'"
                      gdbserver                              build-id

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

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;

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 +1144,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_NONE, NULL, NULL }, 

There has to be be GDB_XML_EF_OPTIONAL.


>    { NULL, GDB_XML_AF_NONE, NULL, NULL }
>  };
>  
> @@ -2458,4 +2559,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 do not see used it anywhere.


> +#define NOTE_GNU_BUILD_ID_NAME  ".note.gnu.build-id"

It is used only in solib-svr4.c so it should be in that file.


> +
>  struct objfile;
>  struct target_so_ops;
>  
> diff --git a/gdb/solib-target.c b/gdb/solib-target.c
> index d897bc0..f43037a 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 = 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..0d68373 100644
> --- a/gdb/solib.c
> +++ b/gdb/solib.c
> @@ -477,6 +477,17 @@ solib_map_sections (struct so_list *so)
>  	     bfd_get_filename (abfd), bfd_errmsg (bfd_get_error ()));
>      }
>  
> +  gdb_assert (ops->validate != NULL);
> +
> +  if (!ops->validate (so))
> +    {
> +      warning (_("Shared object \"%s\" could not be validated and will be ignored."),

Number of columns exceeds 80 characters.



And move it under 'for (p = so->sections; p < so->sections_end; p++)' with the
reasons described above.


> +	       so->so_name);
> +      gdb_bfd_unref (so->abfd);
> +      so->abfd = NULL;
> +      return 0;
> +    }
> +
>    for (p = so->sections; p < so->sections_end; p++)
>      {
>        /* Relocate the section binding addresses as recorded in the shared
> @@ -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
> +solib_validate (const struct so_list *const so)

Up to you but I would prefer "default_solib_validate", "solib_validate" name
seems as if it does some validation.


> +{
> +  return 1; /* No validation.  */

Formatting:
  /* No validation.  */
  return 1;


> +}
> +
>  extern initialize_file_ftype _initialize_solib; /* -Wmissing-prototypes */
>  
>  void
> diff --git a/gdb/solib.h b/gdb/solib.h
> index b811866..ae42e9d 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 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
          build-id

> +       .note.gnu.build-id including note header.

It really should not contain the note header, it should be exactly 20 bytes
with the standard build-id in place.

I wanted to check it with gdbserver but gdbserver does not work for me due to
those errors like:

gdbserver: Error parsing {s,}maps file '/usr/lib64/libdl-2.17.so'^M


> 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 *).  */
> -- 
> 1.7.10.4
> 

> diff --git a/gdb/testsuite/gdb.base/solib-mismatch-lib.c b/gdb/testsuite/gdb.base/solib-mismatch-lib.c
> new file mode 100644
> index 0000000..19f1545
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/solib-mismatch-lib.c
> @@ -0,0 +1,29 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2013 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +
> +int _bar = 42;
> +
> +int bar(void)
> +{
> +  return _bar + 21;
> +}
> +
> +int foo(void)
> +{
> +  return _bar;
> +}
> diff --git a/gdb/testsuite/gdb.base/solib-mismatch-libmod.c b/gdb/testsuite/gdb.base/solib-mismatch-libmod.c
> new file mode 100644
> index 0000000..3b025a8
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/solib-mismatch-libmod.c
> @@ -0,0 +1,29 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2013 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +
> +int _bar = 21;
> +
> +int bar(void)
> +{
> +  return 42 - _bar;
> +}
> +
> +int foo(void)
> +{
> +  return 24 + bar();
> +}
> diff --git a/gdb/testsuite/gdb.base/solib-mismatch.c b/gdb/testsuite/gdb.base/solib-mismatch.c
> new file mode 100644
> index 0000000..85a2784
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/solib-mismatch.c
> @@ -0,0 +1,59 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2013 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +
> +#include <dlfcn.h>
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <signal.h>
> +
> +#define lib "./solib-mismatch.so"

Macros should use uppercased names.


> +
> +
> +/* First argument is working directory. */
> +
> +int main(int argc, char *argv[])
> +{
> +  void *h;
> +  int (*foo)(void);
> +  char buff[1024];
> +
> +  if (argc < 2)
> +    {
> +      printf ("ERROR - CWD not provided\n");
> +      return 1;
> +    }
> +
> +  if (chdir (argv[1]) != 0)
> +    {
> +      printf ("ERROR - Could not cd to '%s'\n", argv[1]);
> +      return 1;
> +    }
> +
> +  h = dlopen(lib, RTLD_NOW);
> +
> +  if (h == NULL)
> +    {
> +      printf ("ERROR - could not open lib %s\n", lib);
> +      return 1;
> +    }
> +  foo = dlsym(h, "foo");
> +  printf ("foo: %p\n", foo); /* set breakpoint 1 here */
> +  dlclose(h);
> +  return 0;
> +}
> +
> diff --git a/gdb/testsuite/gdb.base/solib-mismatch.exp b/gdb/testsuite/gdb.base/solib-mismatch.exp
> new file mode 100644
> index 0000000..c2192a5
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/solib-mismatch.exp
> @@ -0,0 +1,186 @@
> +# Copyright 2013 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +# are we on a target board

I do not see why this comment is here.


> +standard_testfile
> +set executable $testfile
> +
> +# Test overview:
> +#  generate two shared objects. One that will be used by the process
> +#  and another, modified, that will be found by gdb. Gdb should
> +#  detect the mismatch and refuse to use mismatched shared object.
> +
> +if { [get_compiler_info] } {
> +  untested ${testfile}.exp

Use specific text, like:
     untested "get_compiler_info failed."

> +}
> +
> +# First version of the object, to be loaded by ld 
> +set srclibfilerun ${testfile}-lib.c

Please make empty lines before comments.


> +# Modified version of the object to be loaded by gdb
> +# Code in -libmod.c is tuned so it gives a mismatch but
> +# leaves .dynamic at the same point.
> +set srclibfilegdb ${testfile}-libmod.c
> +
> +# So file name:
> +set binlibfilebase ${testfile}.so
> +
> +# Setup run directory (where program is run from)
> +#   It contains executable and '-lib' version of the library.
> +set binlibfiledirrun ${objdir}/${subdir}/${testfile}_wd
> +set binlibfilerun ${binlibfiledirrun}/${binlibfilebase}

Use [standard_output_file ...], not ${objdir}.


> +
> +# Second solib version is in current directory, '-libmod' version.
> +set binlibfiledirgdb ${objdir}/${subdir}

Likewise.


> +set binlibfilegdb ${binlibfiledirgdb}/${binlibfilebase}
> +
> +# Executeable
> +set srcfile ${testfile}.c
> +set executable ${testfile}
> +set objfile ${objdir}/${subdir}/${executable}.o
Likewise.
> +set binfile ${objdir}/${subdir}/${executable}
Likewise.
> +
> +send_user "Current WD: [eval pwd]\r\n"

Testfiles must not print messages during normal run.  Use "verbose -log" if needed.

3 times more below.


> +
> +file mkdir "${binlibfiledirrun}"
> +
Initialize it by:
	set exec_opts {}
as IIRC regex testcases have persistent variables during one runtest run.

> +if { ![istarget "*-*-nto-*"] } {
> +  set exec_opts [list debug shlib_load]
> +}
> +
> +if { [prepare_for_testing $testfile.exp $executable $srcfile $exec_opts] != 0 } {
> +  untested ${testfile}.exp

untested is not needed, prepare_for_testing complains on its own.


> +  return -1
> +}
> +
> +if { [gdb_compile_shlib "${srcdir}/${subdir}/${srclibfilerun}" "${binlibfilerun}" [list debug ldflags=-Wl,-soname,${binlibfilebase},--build-id]] != ""
> +     || [gdb_gnu_strip_debug "${binlibfilerun}"]
> +     || [gdb_compile_shlib "${srcdir}/${subdir}/${srclibfilegdb}" "${binlibfilegdb}" [list debug ldflags=-Wl,-soname,${binlibfilebase},--build-id]] != "" } {
> +  untested ${testfile}.exp

Use specific message.


> +  return -1
> +}
> +
> +proc solib_matching_test { solibfile symsloaded } {
> +  global gdb_prompt
> +  global testfile
> +  global executable
> +  global srcdir
> +  global subdir
> +  global binlibfiledirrun
> +  global binlibfiledirgdb
> +  global srcfile
> +
> +  gdb_exit
> +  gdb_start
> +  gdb_reinitialize_dir $srcdir/$subdir

use clean_restart


> +  
> +  send_gdb "set verbose 1\n"

Never (only in some exceptional cases) use send_gdb, it creates races wrt
syncing on end of the commands.  Use gdb_test or gdb_test_no_output.


> +  send_gdb "file \"${binlibfiledirrun}/${executable}\"\n"

Do not use "file" by hand, clean_restart does it.


> +  send_gdb "set solib-search-path \"${binlibfiledirgdb}\"\n"
> +  send_gdb "cd ${binlibfiledirgdb}\n"
> +  send_gdb "show solib-search-path\n"

> +  send_gdb "set auto-solib-add off\n"

Why is this command here?  Could you put a comment there?


> +  send_gdb "set args \"${binlibfiledirrun}\"\n"
> +  send_gdb "set verbose 1\n"
> +
> +  set bp_location [gdb_get_line_number "set breakpoint 1 here"]
> +
> +  send_gdb "tbreak ${srcfile}:${bp_location}\n"

Do not use send_gdb and there is gdb_breakpoint function.


> +  gdb_expect {
> +    -re ".*Temporary breakpoint.*${gdb_prompt} $" {
> +    }
> +    default {
> +      untested "${testfile}: Failed to set temp. breakpoint at ${bp_location}"
> +      return -1
> +    }
> +  }
> +
> +  send_gdb "run\r\n"

Use runto_main.  And check its result code.

Currently gdbserver (in non-extended mode) is never tested as "run" will never
start gdbserver.

After I fixed it I get from gdbserver messages like:

gdbserver: Error parsing {s,}maps file '/home/jkratoch/redhat/gdb-qnx/gdb/testsuite/gdb.base/solib-mismatch'^M



> +  gdb_expect {
> +    -re "Starting program.*${gdb_prompt} $" {
> +    }
> +    default {
> +      untested "${testfile}: Failed to hit breakpoint at ${bp_location}"
> +      return -1
> +    }
> +  }
> +
> +  send_gdb "sharedlibrary\n"

Use gdb_test.

> +  gdb_expect {
> +    -re ".*${gdb_prompt} $" {
> +    }
> +    default {
> +      untested "${testfile}: sharedlibrary failure"
> +      return -1
> +    }
> +  }
> +
> +  gdb_test "info sharedlibrary ${solibfile}" \
> +    ".*From.*To.*Syms.*Read.*Shared.*\r\n.*${symsloaded}.*" \
        ^^
BTW leading .* is excessive, gdb_test regex does not have anchored its start.


> +    "Symbols for ${solibfile} loaded: expected '${symsloaded}'"

Protect ${symsloaded} by [string_to_regexp $string] as user
may have regex-unsafe characters there.


> +
> +  send_gdb "p/x foo\n"

Use gdb_test.


> +  gdb_expect {
> +    -re ".*${gdb_prompt} $" {
> +#Nothing, just drain the buffer
> +    }
> +    default {
> +      untested "${testfile}: Failed 'info symbol foo'"
> +      return -1
> +    }
> +  }
> +
> +  send_gdb "detach\n"

I do not see a need for this command, remove it.


> +  gdb_expect {
> +    -re ".*Detaching from program.*${executable}.*${gdb_prompt} $" {
> +#Nothing, just drain the buffer
> +    }
> +    default {
> +      untested "${testfile}: Could not detach from ${testpid}"
> +      return -1
> +    }
> +  }
> +  return 0
> +}
> +
> +# Copy binary to working dir so it pulls in the library from that dir
> +# (by the virtue of $ORIGIN).
> +file copy -force "${binlibfiledirgdb}/${executable}" \
> +		 "${binlibfiledirrun}/${executable}"
> +
> +# Test unstripped, .dynamic matching
> +send_user "test unstripped, .dynamic matching\r\n"
> +solib_matching_test "${binlibfilebase}" "No"
> +
> +# Test --only-keep-debug, .dynamic matching so
> +send_user "test --only-keep-debug\r\n"
> +# Keep original so for debugging purposes
> +file copy -force "${binlibfilegdb}" "${binlibfilegdb}-orig"
> +set objcopy_program [transform objcopy]
> +set result [catch "exec $objcopy_program --only-keep-debug ${binlibfilegdb}"]
> +if {$result != 0} {
> +  untested "${testfile} test --only-keep-debug"
> +  return -1
> +}
> +solib_matching_test "${binlibfilebase}" "No"
> +
> +# Now test it does not mis-invalidate matching libraries
> +send_user "test matching libraries\r\n"
> +# Keep previous so for debugging puroses 
> +file copy -force "${binlibfilegdb}" "${binlibfilegdb}-orig1"
> +# Copy loaded so over the one gdb will find 
> +file copy -force "${binlibfilerun}" "${binlibfilegdb}"
> +solib_matching_test "${binlibfilebase}" "Yes"


The testcase is not safe for gdbserver targets not sharing the filesystem with
the runtest machine.  There are functions like gdb_download for it although
AFAIK many GDB testcases do not work well with such targets.  I do not have
experience with it and/or such a gdbserver setup.


Thanks,
Jan


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]