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


New patch attached. Details inlined below.


On 13-03-28 02:37 PM, Jan Kratochvil wrote:
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.

[AR] Ok. Removed.




    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.

[AR] Ok.



        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.

[AR]. Ok. Change stays, but solib_validate is renamed to default_solib_validate.



      }

    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.

[AR] Ok, separated.



  {
    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.

[AR] I consider it good practice much the same as const-qualifying local vars.



+{
+  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.

[AR] Ok.



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

[AR] ok.



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

[AR] Ok.



+    {
+      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.


[AR] I believe it is clearer by now, we need so->build_id as so->abfd may be closed due to mismatch.





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

[AR] ok. Comment removed. It wanted to say that this is load address, as if the shared object was link-edited with base 0 (i.e. this is after applying displacement).



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

[AR] I read "lma" as load memory address, i.e. address where it is actually loaded. This is to differentiate load address from virtual address as set by the link-editor.


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.

[AR]




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

[AR] ok.



+
+
+/* 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.

[AR] copy-paste; removed



+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."

[AR] ok.


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

Please make empty lines before comments.


[AR] ok.


+# 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}.


[AR] ok.


+
+# 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.

[AR] Ok, informative message incorporated in the PASS/FAIL/UNTESTED entry.



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

[AR] ok.



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


[AR] ok.


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

[AR] I very much dislike using gdb_test unless I actually am doing a test. Otherwise, we end up with testcases that tend to have 30-40 passes but only 2-3 relevant. Thus, when these 2-3 relevant ones start to FAIL it is easy to neglect that due to false cozy feeling that, well, *most* are still passing.



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

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

[AR] ok.



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

[AR] so that we can have control over when is the output printed and match it correctly. I added the comment.



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

[AR] I am not testing setting breakpoints. I do not want these to show up as PASS-es. These passes are irrelevant. The assumption is that breakpoints do work; there are other tests for breakponts.



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

[AR] The same. I am not testing run to main. I am testing this particular feature. There are other tests that test runto_main.


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.

[AR] the same. Irrelevant PASS-es. I do not want them.


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

[AR] ok.



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

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

[AR] symsloaded is argument passed to solib_matching_test, and the test is the only user. Ther eare no other users, and the string may contain only 'Yes' or 'No'.



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



Thanks,


Aleksandar
>From b5796c9a30c81c35b9849094344f86d60a16e2c2 Mon Sep 17 00:00:00 2001
From: Aleksandar Ristovski <ARistovski@qnx.com>
Date: Wed, 27 Mar 2013 16:05:19 -0400
Subject: [PATCH 7/8] Validate symbol file using build-id.

	* 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 (svr4_validate): New function.
	(library_list_start_library): Parse 'build-id' attribute.
	(svr4_library_attributes): Add 'build-id' attribute.
	(svr4_relocate_section_addresses): Read build-id from target
	memory.
	(_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'.
---
 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      |   91 +++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/solib-svr4.h      |    3 ++
 gdb/solib-target.c    |    2 ++
 gdb/solib.c           |   20 +++++++++++
 gdb/solib.h           |    4 +++
 gdb/solist.h          |   14 ++++++++
 16 files changed, 144 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 601e34d..023927a 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -871,6 +871,29 @@ 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_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;
+
+  if (so->build_id != NULL)
+    return elf_tdata (so->abfd)->build_id->size == so->build_idsz
+	   && memcmp (so->build_id, elf_tdata (so->abfd)->build_id->data,
+		      elf_tdata (so->abfd)->build_id->size) == 0;
+  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 +1022,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 +1036,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 +1090,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 }, 
   { NULL, GDB_XML_AF_NONE, NULL, NULL }
 };
 
@@ -2274,10 +2321,53 @@ static void
 svr4_relocate_section_addresses (struct so_list *so,
                                  struct target_section *sec)
 {
+  ULONGEST bfd_sect_size;
+
   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)
+    {
+      /* 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)
+	{
+	  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;
+
+	  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;
+		  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"
+#define NOTE_GNU_BUILD_ID_NAME  ".note.gnu.build-id"
+
 struct objfile;
 struct target_so_ops;
 
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..89503ab 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -495,6 +495,17 @@ solib_map_sections (struct so_list *so)
 	}
     }
 
+  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;
+    }
+
   /* Add the shared object's sections to the current set of file
      section tables.  Do this immediately after mapping the object so
      that later nodes in the list can query this object, as is needed
@@ -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 *).  */
-- 
1.7.10.4

>From d2a051a24105cb0396ca4056235e933dffbdea67 Mon Sep 17 00:00:00 2001
From: Aleksandar Ristovski <ARistovski@qnx.com>
Date: Wed, 27 Mar 2013 16:06:26 -0400
Subject: [PATCH 8/8] Tests for validate symbol file using build-id.

	* 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.
---
 gdb/testsuite/gdb.base/solib-mismatch-lib.c    |   29 +++++
 gdb/testsuite/gdb.base/solib-mismatch-libmod.c |   29 +++++
 gdb/testsuite/gdb.base/solib-mismatch.c        |   68 ++++++++++
 gdb/testsuite/gdb.base/solib-mismatch.exp      |  165 ++++++++++++++++++++++++
 4 files changed, 291 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/solib-mismatch-lib.c
 create mode 100644 gdb/testsuite/gdb.base/solib-mismatch-libmod.c
 create mode 100644 gdb/testsuite/gdb.base/solib-mismatch.c
 create mode 100644 gdb/testsuite/gdb.base/solib-mismatch.exp

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..8a7f58f
--- /dev/null
+++ b/gdb/testsuite/gdb.base/solib-mismatch.c
@@ -0,0 +1,68 @@
+/* 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>
+#include <string.h>
+
+/* The following defines must correspond to solib-mismatch.exp */
+
+#define DIRNAME "solib-mismatch_wd"
+#define LIB "./solib-mismatch.so"
+
+int main(int argc, char *argv[])
+{
+  void *h;
+  int (*foo)(void);
+  char buff[1024];
+  char *p;
+
+  p = strstr (argv[0], DIRNAME);
+
+  if (p == NULL)
+    {
+      printf ("ERROR - %s could not be found in argv[0]\n", DIRNAME);
+      return 1;
+    }
+
+  p += strlen (DIRNAME);
+
+  memcpy (buff, argv[0], p - argv[0]);
+
+  buff[p - argv[0]] = '\0';
+
+  if (chdir (buff) != 0)
+    {
+      printf ("ERROR - Could not cd to %s\n", buff);
+      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"); /* 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..47316b7
--- /dev/null
+++ b/gdb/testsuite/gdb.base/solib-mismatch.exp
@@ -0,0 +1,165 @@
+# 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/>.  */
+
+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 "get_compiler_info failed."
+}
+
+# First version of the object, to be loaded by ld 
+set srclibfilerun ${testfile}-lib.c
+
+# 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 [standard_output_file ${testfile}_wd]
+set binlibfilerun ${binlibfiledirrun}/${binlibfilebase}
+
+# Second solib version is in current directory, '-libmod' version.
+set binlibfiledirgdb [standard_output_file ""]
+set binlibfilegdb ${binlibfiledirgdb}/${binlibfilebase}
+
+# Executeable
+set srcfile ${testfile}.c
+set executable ${testfile}
+set objfile [standard_output_file ${executable}.o]
+set binfile [standard_output_file ${executable}]
+
+send_user "Current WD: [eval pwd]\r\n"
+
+file mkdir "${binlibfiledirrun}"
+
+set exec_opts {}
+
+if { ![istarget "*-*-nto-*"] } {
+  set exec_opts [list debug shlib_load]
+}
+
+if { [prepare_for_testing $testfile.exp $executable $srcfile $exec_opts] != 0 } {
+  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 "gdb_compile_shlib failed."
+  return -1
+}
+
+
+proc solib_matching_test { solibfile symsloaded msg } {
+  global gdb_prompt
+  global testfile
+  global executable
+  global srcdir
+  global subdir
+  global binlibfiledirrun
+  global binlibfiledirgdb
+  global srcfile
+
+  clean_restart ${binlibfiledirrun}/${executable}
+ 
+  send_gdb "set solib-search-path \"${binlibfiledirgdb}\"\n"
+  send_gdb "cd ${binlibfiledirgdb}\n"
+# Do not auto load shared libraries, the test needs to have control
+# over when the relevant output gets printed
+  send_gdb "set auto-solib-add off\n"
+
+  set bp_location [gdb_get_line_number "set breakpoint 1 here"]
+
+  send_gdb "tbreak ${srcfile}:${bp_location}\n"
+  gdb_expect {
+    -re "Temporary breakpoint.*${gdb_prompt} $" {
+    }
+    default {
+      untested "${msg} - Failed to set temp. breakpoint at ${bp_location}"
+      return -1
+    }
+  }
+
+  gdb_run_cmd { ${binlibfiledirrun} }
+  gdb_expect {
+    -re "set breakpoint 1 here.*${gdb_prompt} $" {
+    }
+    default {
+      untested "${msg} - Failed to hit breakpoint at ${bp_location}"
+      return -1
+    }
+  }
+
+  send_gdb "sharedlibrary\n"
+  gdb_expect {
+    -re "${gdb_prompt} $" {
+    }
+    default {
+      untested "${msg} - sharedlibrary failure"
+      return -1
+    }
+  }
+
+  gdb_test "info sharedlibrary ${solibfile}" \
+    "From.*To.*Syms.*Read.*Shared.*\r\n.*${symsloaded}.*" \
+    "${msg} - Symbols for ${solibfile} loaded: expected '${symsloaded}'"
+  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
+solib_matching_test "${binlibfilebase}" "No" \
+  "test unstripped, .dynamic matching" 
+
+# 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 "test --only-keep-debug"
+  return -1
+}
+
+# Test --only-keep-debug, .dynamic matching so
+solib_matching_test "${binlibfilebase}" "No" \
+  "test --only-keep-debug"
+
+# 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}"
+
+# Now test it does not mis-invalidate matching libraries
+solib_matching_test "${binlibfilebase}" "Yes" \
+  "test matching libraries"
+
+
+
-- 
1.7.10.4


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