From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <gdb-patches-return-98095-listarch-gdb-patches=sources.redhat.com@sourceware.org>
Received: (qmail 16849 invoked by alias); 30 Jan 2013 19:17:19 -0000
Received: (qmail 16839 invoked by uid 22791); 30 Jan 2013 19:17:18 -0000
X-SWARE-Spam-Status: No, hits=-6.4 required=5.0	tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_BJ,TW_JC
X-Spam-Check-By: sourceware.org
Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28)    by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 30 Jan 2013 19:17:03 +0000
Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25])	by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r0UJH0nD000971	(version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK);	Wed, 30 Jan 2013 14:17:00 -0500
Received: from host2.jankratochvil.net (ovpn-116-88.ams2.redhat.com [10.36.116.88])	by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r0UJGkKw001190	(version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO);	Wed, 30 Jan 2013 14:16:49 -0500
Date: Wed, 30 Jan 2013 19:17:00 -0000
From: Jan Kratochvil <jan.kratochvil@redhat.com>
To: Aleksandar Ristovski <aristovski@qnx.com>
Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
Subject: Re: [patch] validate binary before use
Message-ID: <20130130191646.GA1034@host2.jankratochvil.net>
References: <50D4C49A.6040502@qnx.com> <50D8B37A.20001@qnx.com> <20121225073709.GA11349@host2.jankratochvil.net> <50DCAA5C.3000301@qnx.com> <20121227205924.GA5109@host2.jankratochvil.net> <50DCB787.6020601@qnx.com> <20121227211328.GA5739@host2.jankratochvil.net> <50DCBBD1.7000707@qnx.com> <5107F591.304@qnx.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <5107F591.304@qnx.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
X-IsSubscribed: yes
Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm
Precedence: bulk
List-Id: <gdb-patches.sourceware.org>
List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org>
List-Archive: <http://sourceware.org/ml/gdb-patches/>
List-Post: <mailto:gdb-patches@sourceware.org>
List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs>
Sender: gdb-patches-owner@sourceware.org
X-SW-Source: 2013-01/txt/msg00725.txt.bz2

On Tue, 29 Jan 2013 17:15:13 +0100, Aleksandar Ristovski wrote:
> Index: gdb/mips-linux-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/mips-linux-tdep.c,v
> retrieving revision 1.97
> diff -u -p -r1.97 mips-linux-tdep.c
> --- gdb/mips-linux-tdep.c	1 Jan 2013 06:32:47 -0000	1.97
> +++ gdb/mips-linux-tdep.c	29 Jan 2013 15:46:39 -0000
> @@ -1495,6 +1495,8 @@ mips_linux_init_abi (struct gdbarch_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;
>    set_solib_ops (gdbarch, &mips_svr4_so_ops);
>  
>    set_gdbarch_write_pc (gdbarch, mips_linux_write_pc);
> Index: gdb/ppc-linux-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/ppc-linux-tdep.c,v
> retrieving revision 1.141
> diff -u -p -r1.141 ppc-linux-tdep.c
> --- gdb/ppc-linux-tdep.c	1 Jan 2013 06:32:49 -0000	1.141
> +++ gdb/ppc-linux-tdep.c	29 Jan 2013 15:46:39 -0000
> @@ -1732,6 +1732,8 @@ ppc_linux_init_abi (struct gdbarch_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;
>        set_solib_ops (gdbarch, &powerpc_so_ops);
>  
>        set_gdbarch_skip_solib_resolver (gdbarch, glibc_skip_solib_resolver);
> Index: gdb/solib-darwin.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/solib-darwin.c,v
> retrieving revision 1.35
> diff -u -p -r1.35 solib-darwin.c
> --- gdb/solib-darwin.c	1 Jan 2013 06:32:50 -0000	1.35
> +++ gdb/solib-darwin.c	29 Jan 2013 15:46:39 -0000
> @@ -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;
>  }
> Index: gdb/solib-dsbt.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/solib-dsbt.c,v
> retrieving revision 1.9
> diff -u -p -r1.9 solib-dsbt.c
> --- gdb/solib-dsbt.c	1 Jan 2013 06:32:50 -0000	1.9
> +++ gdb/solib-dsbt.c	29 Jan 2013 15:46:39 -0000
> @@ -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,
> Index: gdb/solib-frv.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/solib-frv.c,v
> retrieving revision 1.49
> diff -u -p -r1.49 solib-frv.c
> --- gdb/solib-frv.c	1 Jan 2013 06:32:50 -0000	1.49
> +++ gdb/solib-frv.c	29 Jan 2013 15:46:39 -0000
> @@ -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,
> Index: gdb/solib-ia64-hpux.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/solib-ia64-hpux.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 solib-ia64-hpux.c
> --- gdb/solib-ia64-hpux.c	1 Jan 2013 06:32:50 -0000	1.6
> +++ gdb/solib-ia64-hpux.c	29 Jan 2013 15:46:39 -0000
> @@ -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;
>  }
> Index: gdb/solib-irix.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/solib-irix.c,v
> retrieving revision 1.46
> diff -u -p -r1.46 solib-irix.c
> --- gdb/solib-irix.c	1 Jan 2013 06:32:50 -0000	1.46
> +++ gdb/solib-irix.c	29 Jan 2013 15:46:39 -0000
> @@ -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;
>  }
> Index: gdb/solib-osf.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/solib-osf.c,v
> retrieving revision 1.38
> diff -u -p -r1.38 solib-osf.c
> --- gdb/solib-osf.c	1 Jan 2013 06:32:50 -0000	1.38
> +++ gdb/solib-osf.c	29 Jan 2013 15:46:39 -0000
> @@ -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;
> Index: gdb/solib-pa64.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/solib-pa64.c,v
> retrieving revision 1.40
> diff -u -p -r1.40 solib-pa64.c
> --- gdb/solib-pa64.c	1 Jan 2013 06:32:51 -0000	1.40
> +++ gdb/solib-pa64.c	29 Jan 2013 15:46:39 -0000
> @@ -623,6 +623,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));
>  }
> Index: gdb/solib-som.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/solib-som.c,v
> retrieving revision 1.39
> diff -u -p -r1.39 solib-som.c
> --- gdb/solib-som.c	1 Jan 2013 06:32:51 -0000	1.39
> +++ gdb/solib-som.c	29 Jan 2013 15:46:39 -0000
> @@ -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
> Index: gdb/solib-spu.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/solib-spu.c,v
> retrieving revision 1.24
> diff -u -p -r1.24 solib-spu.c
> --- gdb/solib-spu.c	1 Jan 2013 06:32:51 -0000	1.24
> +++ gdb/solib-spu.c	29 Jan 2013 15:46:39 -0000
> @@ -519,6 +519,7 @@ set_spu_solib_ops (struct gdbarch *gdbar
>        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;
>      }
>  
>    set_solib_ops (gdbarch, &spu_so_ops);
> Index: gdb/solib-sunos.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/solib-sunos.c,v
> retrieving revision 1.51
> diff -u -p -r1.51 solib-sunos.c
> --- gdb/solib-sunos.c	1 Jan 2013 06:32:51 -0000	1.51
> +++ gdb/solib-sunos.c	29 Jan 2013 15:46:39 -0000
> @@ -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;
> Index: gdb/solib-svr4.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/solib-svr4.c,v
> retrieving revision 1.172
> diff -u -p -r1.172 solib-svr4.c
> --- gdb/solib-svr4.c	1 Jan 2013 06:32:51 -0000	1.172
> +++ gdb/solib-svr4.c	29 Jan 2013 15:46:39 -0000

OK, I see currently target_so_ops methods are always valid and there is no
common initializer.


> @@ -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)
>  {
>    if (!so->lm_info->l_addr_p)
>      {
> @@ -848,7 +848,6 @@ svr4_keep_data_in_core (CORE_ADDR vaddr,
>    CORE_ADDR ldsomap;
>    struct so_list *new;
>    struct cleanup *old_chain;
> -  struct link_map_offsets *lmo;
>    CORE_ADDR name_lm;
>  
>    info = get_svr4_info ();
> @@ -862,7 +861,6 @@ svr4_keep_data_in_core (CORE_ADDR vaddr,
>    if (!ldsomap)
>      return 0;
>  
> -  lmo = svr4_fetch_link_map_offsets ();
>    new = XZALLOC (struct so_list);
>    old_chain = make_cleanup (xfree, new);
>    new->lm_info = lm_info_read (ldsomap);

This part is OK as a "Code cleanup" but it should be checked in separately, it
is unrelated.


> @@ -873,6 +871,110 @@ svr4_keep_data_in_core (CORE_ADDR vaddr,
>    return (name_lm >= vaddr && name_lm < vaddr + size);
>  }
>  
> +/* Find containing segment for the section and calculate section's
> +   offset.  */

Empty line after function comment.

> +static CORE_ADDR
> +svr4_unrelocated_vma (const bfd *const abfd, const asection *const asect)
> +{
> +  const Elf_Internal_Ehdr *const ehdr = elf_tdata (abfd)->elf_header;
> +  const Elf_Internal_Phdr *const phdr = elf_tdata (abfd)->phdr;
> +  CORE_ADDR phdr_base = 0;
> +  int phdr_base_p = 0;
> +  int i;
> +
> +  for (i = 0; i < ehdr->e_phnum; ++i)
> +    {
> +      if (phdr[i].p_type == PT_LOAD)
> +	{
> +	  if (!phdr_base_p)
> +	    {
> +	      /* First PT_LOAD serves as the base.  */
> +	      phdr_base = phdr[i].p_vaddr;
> +	      phdr_base_p = 1;
> +	    }
> +	  if (phdr[i].p_vaddr <= asect->vma
> +	      && (phdr[i].p_vaddr + phdr[i].p_memsz) > asect->vma)
> +	    {
> +	      if (phdr_base_p)
> +		return (asect->vma - phdr_base);
> +	      else
> +		return (asect->vma - phdr[i].p_vaddr);
> +	    }
> +	}
> +    }
> +
> +  return 0;
> +}

This function is not needed, see below.


> +
> +/* 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)
> +{
> +  const asection *asect;
> +  int i, res = 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;
> +
> +  for (asect = so->abfd->sections; asect != NULL; asect = asect->next)
> +    {
> +      if ((asect->flags & SEC_LOAD) == SEC_LOAD)
> +	{
> +	  const char *const sectname = bfd_get_section_name (so->abfd, asect);
> +	  const bfd_size_type size = bfd_get_section_size (asect);
> +
> +          if (size != 0 && strcmp (sectname, NOTE_GNU_BUILD_ID_NAME) == 0)

spaces->tab.

> +	    {
> +	      gdb_byte *const data = xmalloc (size);
> +	      /* Real load base, not the offset from a possibly rebased
> +	         object.  */
> +	      const CORE_ADDR so_base_addr = so->lm_info->l_addr_inferior;
> +	      /* Zero based vma, after undoing link script non zero base
> +	         and/or prelinked rebasing.  */
> +	      const CORE_ADDR sect_vma_offset
> +		= svr4_unrelocated_vma (so->abfd, asect);
> +
> +	      /* Relocated or not, contents will be corectly loaded from
> +	         the file by bfd library.  */
> +
> +	      bfd_get_section_contents ((bfd *) so->abfd,
> +					(asection *)asect, data, 0,

Just do not make 'asect' so const, then you will not need that '(asection *)'
cast.


> +					size);
> +	      /* Section vma is unrelocated.  If SO_BASE_ADDR is zero, then
> +	         use ASECT->VMA as-is.  If not, then use offset + base addr.  */
> +	      res = target_verify_memory (data, (so_base_addr > 0)?

I do not see why to use target_verify_memory in this case.

target_verify_memory is there for large sections to compare only their 32-bit
checksum.  But build-id is already only 20 bytes long, with the protocol
overhead the 4 vs. 20 bytes do not make a difference.  And it needlessly
weakens the check, it also does some patching of target_verify_memory.
Just use target_read_memory and memcmp.


> +					   so_base_addr + sect_vma_offset
> +					   : asect->vma,
> +					  size);

so->abfd is not properly relocated (not sure why but it is so) but you can
iterate so->sections..so->sections_end which contains relocated ADDR (=target
VMA).  Then you can drop the svr4_unrelocated_vma and other calculations
around.


> +	      xfree (data);
> +
> +	      if (res == -1 && info_verbose)
> +		warning (_("Could not verify section %s\n"),
> +			 NOTE_GNU_BUILD_ID_NAME);

Here could be break;.

> +	    }
> +	}
> +    }
> +
> +  return (res != 0);

Just:
  return res != 0;


> +}
> +
> +/* 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);
> +}
> +
>  /* Implement the "open_symbol_file_object" target_so_ops method.
>  
>     If no open symbol file, attempt to locate and open the main symbol


> @@ -1175,7 +1277,6 @@ svr4_read_so_list (CORE_ADDR lm, struct 
>  
>    for (; lm != 0; prev_lm = lm, lm = next_lm)
>      {
> -      struct link_map_offsets *lmo = svr4_fetch_link_map_offsets ();
>        struct so_list *new;
>        struct cleanup *old_chain;
>        int errcode;

This is also unrelated "Code cleanup" to be checked in separately.


> @@ -2461,4 +2562,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;
>  }
> Index: gdb/solib-svr4.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/solib-svr4.h,v
> retrieving revision 1.24
> diff -u -p -r1.24 solib-svr4.h
> --- gdb/solib-svr4.h	1 Jan 2013 06:32:51 -0000	1.24
> +++ gdb/solib-svr4.h	29 Jan 2013 15:46:39 -0000
> @@ -84,4 +84,7 @@ extern struct link_map_offsets *svr4_lp6
>     SVR4 run time loader.  */
>  int svr4_in_dynsym_resolve_code (CORE_ADDR pc);
>  
> +
> +#define NOTE_GNU_BUILD_ID_NAME  ".note.gnu.build-id"

It is more common to place #define somewhere at the top of the file, not at
the bottom or between function declarations.


> +
>  #endif /* solib-svr4.h */
> Index: gdb/solib-target.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/solib-target.c,v
> retrieving revision 1.26
> diff -u -p -r1.26 solib-target.c
> --- gdb/solib-target.c	1 Jan 2013 06:32:51 -0000	1.26
> +++ gdb/solib-target.c	29 Jan 2013 15:46:39 -0000
> @@ -25,6 +25,7 @@
>  #include "target.h"
>  #include "vec.h"
>  #include "solib-target.h"
> +#include "solib.h"
>  
>  #include "gdb_string.h"
>  
> @@ -501,6 +502,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.  */
> Index: gdb/solib.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/solib.c,v
> retrieving revision 1.169
> diff -u -p -r1.169 solib.c
> --- gdb/solib.c	1 Jan 2013 06:32:51 -0000	1.169
> +++ gdb/solib.c	29 Jan 2013 15:46:39 -0000
> @@ -495,6 +495,17 @@ solib_map_sections (struct so_list *so)
>  	}
>      }
>  
> +  gdb_assert (ops->validate != NULL);
> +
> +  if (!ops->validate (so))
> +    {
> +      warning (_("Shared object could not be validated and will be ignored: %s."),

Either without the trailing dot or the more common case:
	_("Shared object '%s' could not be validated and will be ignored."),


> +	       so->abfd->filename);

There would be rather bfd_get_filename (so->abfd).

But GDB prints for shared libraries their so->so_name, it is also guaraneed to
be the same here.


> +      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
> @@ -1448,6 +1459,14 @@ gdb_bfd_lookup_symbol (bfd *abfd,
>    return symaddr;
>  }
>  
> +/* Default implementation does not perform any validation.  */

You provide two similar function comments, one in solib.c and one in solib.h.
There is s risk they become out of sync in the future.  As solib.h already
contains comments at its functions I would keep the solib.h as is and replace
this solib.c comment by:
	/* See solib.h for the function comment.  */


> +
> +int
> +solib_validate (const struct so_list *const so)
> +{
> +  return 1; /* No validation.  */

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


> +}
> +
>  extern initialize_file_ftype _initialize_solib; /* -Wmissing-prototypes */
>  
>  void
> Index: gdb/solib.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/solib.h,v
> retrieving revision 1.34
> diff -u -p -r1.34 solib.h
> --- gdb/solib.h	1 Jan 2013 06:32:51 -0000	1.34
> +++ gdb/solib.h	29 Jan 2013 15:46:39 -0000
> @@ -90,4 +90,8 @@ extern CORE_ADDR gdb_bfd_lookup_symbol_f
>  								      void *),
>  						    void *data);
>  
> +/* Default validation always returns 1.  */
> +
> +extern int solib_validate (const struct so_list *so);
> +
>  #endif /* SOLIB_H */
> Index: gdb/solist.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/solist.h,v
> retrieving revision 1.38
> diff -u -p -r1.38 solist.h
> --- gdb/solist.h	1 Jan 2013 06:32:51 -0000	1.38
> +++ gdb/solist.h	29 Jan 2013 15:46:39 -0000
> @@ -148,6 +148,11 @@ struct target_so_ops
>         core file (in particular, for readonly sections).  */
>      int (*keep_data_in_core) (CORE_ADDR vaddr,
>  			      unsigned long size);
> +
> +

Single empty line only, not two.

> +    /* 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 *).  */
> Index: gdb/target.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/target.c,v
> retrieving revision 1.318
> diff -u -p -r1.318 target.c
> --- gdb/target.c	1 Jan 2013 06:32:52 -0000	1.318
> +++ gdb/target.c	29 Jan 2013 15:46:39 -0000
> @@ -4045,12 +4045,14 @@ int
>  target_verify_memory (const gdb_byte *data, CORE_ADDR memaddr, ULONGEST size)
>  {
>    struct target_ops *t;
> +  gdb_byte *buff;
> +  int retval = -1;
>  
>    for (t = current_target.beneath; t != NULL; t = t->beneath)
>      {
>        if (t->to_verify_memory != NULL)
>  	{
> -	  int retval = t->to_verify_memory (t, data, memaddr, size);
> +	  retval = t->to_verify_memory (t, data, memaddr, size);
>  
>  	  if (targetdebug)
>  	    fprintf_unfiltered (gdb_stdlog,
> @@ -4062,7 +4064,23 @@ target_verify_memory (const gdb_byte *da
>  	}
>      }
>  
> -  tcomplain ();
> +  if (targetdebug)
> +    fprintf_unfiltered (gdb_stdlog,
> +			"target_verify_memory (%s, %s) - use target_read_memory\n",
> +			paddress (target_gdbarch (), memaddr),
> +			pulongest (size));
> +
> +  /* Default: use memory reads and compare raw memory */
> +  buff = xmalloc (size);
> +
> +  if (target_read_memory (memaddr, buff, size) == 0)
> +    retval = (memcmp (buff, data, size) == 0);
> +  else
> +    retval = -1;
> +
> +  xfree (buff);
> +
> +  return retval;

As discussed at its call I do not find this change useful, to be dropped.


>  }
>  
>  /* The documentation for this function is in its prototype declaration in
> Index: gdb/testsuite/gdb.base/solib-mismatch-lib.c
> ===================================================================
> RCS file: gdb/testsuite/gdb.base/solib-mismatch-lib.c
> diff -N gdb/testsuite/gdb.base/solib-mismatch-lib.c
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ gdb/testsuite/gdb.base/solib-mismatch-lib.c	29 Jan 2013 15:46:40 -0000
> @@ -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;
> +}
> Index: gdb/testsuite/gdb.base/solib-mismatch-libmod.c
> ===================================================================
> RCS file: gdb/testsuite/gdb.base/solib-mismatch-libmod.c
> diff -N gdb/testsuite/gdb.base/solib-mismatch-libmod.c
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ gdb/testsuite/gdb.base/solib-mismatch-libmod.c	29 Jan 2013 15:46:40 -0000
> @@ -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();
> +}
> Index: gdb/testsuite/gdb.base/solib-mismatch.c
> ===================================================================
> RCS file: gdb/testsuite/gdb.base/solib-mismatch.c
> diff -N gdb/testsuite/gdb.base/solib-mismatch.c
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ gdb/testsuite/gdb.base/solib-mismatch.c	29 Jan 2013 15:46:40 -0000
> @@ -0,0 +1,43 @@
> +/* 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"
> +
> +
> +int main(int argc, char *argv[])
> +{
> +  void *h = dlopen(lib, RTLD_NOW);
> +  int (*foo)(void);
> +
> +  if (h == NULL)
> +	{
> +	  printf("ERROR - could not open lib %s\n", lib);
> +	  return 1;
> +	}
> +  foo = dlsym(h, "foo");
> +  printf("foo says: %d\n", (*foo)());
> +  raise(SIGSTOP); /* Let gdb attach */

No testcase should remain running indefinitely, there should be for example
sleep (60); otherwise there remain during various crashes stale processes on
the system.

But that should not be needed with process spawned from GDB.


> +  dlclose(h);
> +  return 0;
> +}
> +
> Index: gdb/testsuite/gdb.base/solib-mismatch.exp
> ===================================================================
> RCS file: gdb/testsuite/gdb.base/solib-mismatch.exp
> diff -N gdb/testsuite/gdb.base/solib-mismatch.exp
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ gdb/testsuite/gdb.base/solib-mismatch.exp	29 Jan 2013 15:46:40 -0000
> @@ -0,0 +1,175 @@
> +# 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
> +set test "solib-mismatch"
> +set testfile "solib-mismatch"

Use the new macro standard_testfile instead.


> +
> +# 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.
> +
> +# 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 ${objdir}/${subdir}/lib${testfile}

Please keep the same prefix of all the files as ${testfile}...
(not lib${testfile}...)


> +set binlibfilerun ${binlibfiledirrun}/${binlibfilebase}

Use the new macro standard_output_file.


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

Use the new macro standard_output_file.


> +
> +# Executeable
> +set srcfile ${testfile}.c
> +set executable ${testfile}
> +set objfile ${objdir}/${subdir}/${executable}.o
> +set binfile ${objdir}/${subdir}/${executable}

These get set by standard_testfile (except for $executable).


> +
> +file mkdir "${binlibfiledirrun}"
> +
> +set exec_opts [list debug additional_flags=-ldl]
> +
> +if [istarget "*-*-nto-*"] {
> +  set exec_opts {} 
> +}
> +
> +# build the first test case
> +if { [get_compiler_info]
> +     || [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]] != ""
> +     || [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${objfile}" object {debug}] != ""
> +     || [gdb_compile "${objfile}" "${binfile}" executable ${exec_opts}] != "" } {
> +    untested ${testfile}.exp

${testfile}.exp does not tell anything, use for example:
    untested "Compilation failed.


> +    return -1
> +}
> +
> +# Start with a fresh gdb

Remove the comment, there is no command for it here.


> +
> +# Start the exe. It will raise SIGSTOP on itself.
> +send_user "Exec: $executable\r\n"

dejagnu uses: verbose -log "Exec: $executable"

send_user also prints messages on screen during runtest, it should not (at
least it is not done in GDB testsuite).


> +set curdir [eval pwd]

Normal
	set curdir [pwd]
works for me.


> +set dummy [eval cd "${binlibfiledirrun}"]

Normal
	cd ${binlibfiledirrun}
works for me.


> +set testpid [eval exec "../${executable}" &]

The primary problem I have with the testcase is that it needlessly uses
separately spawned process for attach/detach.  This makes the testcase
complicated, racy with testing system under load (I do not see any sleep here,
${executable} may be for example still in shell spawning ${executable} when
GDb attaches to it) and also fragile for leftover running processes.

I find it possible to test the feature just with normally "start"-ed process
from GDB.

In general always use only "cd" command in GDB, not in dejagnu.

And then you can:
 * start the inferior, stop before dlopen
 * "cd" in GDB to the other directory (libsolib-mismatch/).
 * next "dlopen".

Now the inferior will use different current directory than GDB, while both
will load "./solib-mismatch.so".


> +send_user "You can now attach to $testpid\r\n"

verbose -log

> +
> +set dummy [eval cd "${binlibfiledirgdb}"]

Again, to be simplified.

But this leaves dejagnu in non-standard directory, current directory should be
always switched only temporarily as otherwise one forgets to restore it in
this or that exit path.


> +
> +proc solib_matching_test { solibfile symsloaded } {
> +  global gdb_prompt
> +  global testpid
> +  global test
> +  global executable
> +  global srcdir
> +  global subdir
> +
> +  gdb_exit
> +  gdb_start
> +  gdb_reinitialize_dir $srcdir/$subdir
> +  gdb_load ${executable}

This block can be replaced by:
	clean_restart ${executable}


> +
> +  send_gdb "set auto-solib-add off\r\n"
> +    gdb_expect {
> +      -re "${gdb_prompt} $" {
> +# Nothing, just drain the buffer
> +      }
> +    }

send_gdb never needs to be used in general, it does not handle various corner
cases.  In this case it is enough:
	gdb_test_no_output "set auto-solib-add off"

The same applies to any send_gdb commands below.


> +
> +# Test unstripped, .dynamic matching so
> +  send_gdb "attach ${testpid}\r\n"
> +    gdb_expect {
> +      -re "Attaching to program:.*${executable}, process ${testpid}.*${gdb_prompt} $" {
> +# Nothing
> +      }
> +      default {
> +	untested "${test}: Could not attach to ${testpid}"
> +	return -1
> +      }
> +    }
> +
> +  gdb_test_multiple "sharedlibrary" $test {
> +    -re ".*${gdb_prompt} $" {
> +      pass "Validate library detects mismatch"
> +    }
> +    default {
> +      fail "${test}: sharedlibrary failure"
> +    }
> +  }
> +
> +  gdb_test "info sharedlibrary ${solibfile}" \
> +    ".*From.*To.*Syms.*Read.*Shared.*\r\n0x\[0-9a-f\]+.*0x\[0-9a-f\]+.*${symsloaded}.*" \
> +    "Symbols for ${solibfile} loaded: expected '${symsloaded}'"
> +
> +    send_gdb "detach\r\n"
> +    gdb_expect {
> +      -re ".*Detaching from program.*${executable}.*${gdb_prompt} $" {
> +#Nothing, just drain the buffer
> +      }
> +      default {
> +	untested "${test}: Could not detach from ${testpid}"
> +	return -1
> +      }
> +    }
> +  return 0
> +}
> +
> +proc teardown {} {
> +  global testpid
> +
> +  send_user "send SIGCONT to ${testpid}\r\n"
> +  send_gdb "attach ${testpid}\r\n"
> +  send_gdb "signal SIGCONT\r\n"
> +  send_gdb "detach\r\n"
> +  gdb_expect {
> +    -re ".*" {
> +#Nothing, just drain the buffer
> +    }
> +  }
> +}

Hopefully no longer needed without attachments.


> +
> +# 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"
> +set objcopy_program [transform objcopy]
> +set result [catch "exec $objcopy_program --only-keep-debug ${binlibfilegdb}"]
> +if {$result != 0} {
> +  untested "${test} test --only-keep-debug"
> +  set dummy [eval cd "${curdir}"]
> +  return -1
> +}
> +solib_matching_test "${binlibfilebase}" "No"
> +
> +# Now test it does not mis-invalidate matching libraries
> +send_user "test matching libraries\r\n"
> +file copy -force "${binlibfilerun}" "${binlibfilegdb}"

Here is ${binlibfilegdb} and it is nowhere left.  It makes troubleshooting
difficult, one has to modify the .exp file first to get the library for manual
testing.  Keep some copy of it around.


> +solib_matching_test "${binlibfilebase}" "Yes"
> +
> +send_user "cleanup\r\n"
> +teardown
> +set dummy [eval cd "${curdir}"]
> +


Thanks,
Jan