Re: vdso handling

Re: vdso handling

[Alan Modra]

On Fri, Mar 21, 2014 at 03:48:48PM +0000, Pedro Alves wrote:
> I just tried pointing add-symbol-file-from-memory at an already
> mapped DSO's elf header, but it doesn't work as is unfortunately:
> 
>  (gdb) info shared curses
>  0x000000324d006d20  0x000000324d01df58  Yes         /lib64/libncurses.so.5
>  (gdb) x /4b 0x000000324d000000
>  0x324d000000:   127     69      76      70
>  (gdb) add-symbol-file-from-memory 0x000000324d000000
>  Failed to read a valid object file image from memory.
> 
> I single stepped a little through
> bfd_elf_bfd_from_remote_memory - something goes wrong with the
> reading of the load segment contents, probably something wrong
> with the address computations.

readelf -a --wide on my x86_64 libncurses.so.5 shows

[snip]
  Start of section headers:          132144 (bytes into file)
[snip]
  [25] .shstrtab         STRTAB          0000000000000000 02034c 0000de 00      0   0  1
[snip]
  LOAD           0x000000 0x0000000000000000 0x0000000000000000 0x01efe4 0x01efe4 R E 0x200000
  LOAD           0x01fd50 0x000000000021fd50 0x000000000021fd50 0x0005e4 0x000770 RW  0x200000

So .shstrtab and the section headers might have been loaded by the
second PT_LOAD header, *but* the second PT_LOAD has a bss area.
Anything past 0x220334 will be cleared out by ld.so.  No chance of
getting at section headers then, and this will be true for most
in-memory images.

bfd_from_remote_memory should take note of p_memsz..  Hmm, and there
are quite a few other issues there too, most notably that p_align
on x86_64 these days tends to be *much* larger than the page size used
by ld.so.

Gah, I've been sucked into looking at this long enough that I may as
well fix it.  Does this look OK?

bfd/
	* elfcode.h (bfd_from_remote_memory): Add "size" parameter.
	Consolidate code handling possible section headers past end of
	segment.  Don't use p_align for page size guess, instead use
	minpagesize.  Take note of ld.so clearing section headers when
	p_memsz > p_filesz.  Handle file header specifying no section
	headers.  Handle zero p_align throughout.  Default loadbase to
	zero.  Add comments.  Rename contents_size to high_offset, and
	make it a bfd_vma.  Delete unnecessary bfd_set_error calls.
	* bfd-in.h (bfd_elf_bfd_from_remote_memory): Update prototpe.
	* elf-bfd.h (struct elf_backend_data <elf_backend_from_remote_memory>):
	Likewise.
	(_bfd_elf32_bfd_from_remote_memory): Likewise.
	(_bfd_elf64_bfd_from_remote_memory): Likewise.
	* elf.c (bfd_elf_bfd_from_remote_memory): Adjust.
	* bfd-in2.h: Regnerate.
gdb/
	* symfile-mem.c (symbol_file_add_from_memory): Add size parameter.
	Pass to bfd_elf_bfd_from_remote_memory.  Adjust all callers.
	(struct symbol_file_add_from_memory_args): Add size field.
	(find_vdso_size): New function.
	(add_vsyscall_page): Attempt to find vdso size.

diff --git a/bfd/bfd-in.h b/bfd/bfd-in.h
index da350a5..ddc8270 100644
--- a/bfd/bfd-in.h
+++ b/bfd/bfd-in.h
@@ -680,19 +680,21 @@ extern int bfd_get_elf_phdrs
   (bfd *abfd, void *phdrs);
 
 /* Create a new BFD as if by bfd_openr.  Rather than opening a file,
-   reconstruct an ELF file by reading the segments out of remote memory
-   based on the ELF file header at EHDR_VMA and the ELF program headers it
-   points to.  If not null, *LOADBASEP is filled in with the difference
-   between the VMAs from which the segments were read, and the VMAs the
-   file headers (and hence BFD's idea of each section's VMA) put them at.
-
-   The function TARGET_READ_MEMORY is called to copy LEN bytes from the
-   remote memory at target address VMA into the local buffer at MYADDR; it
-   should return zero on success or an `errno' code on failure.  TEMPL must
-   be a BFD for an ELF target with the word size and byte order found in
-   the remote memory.  */
+   reconstruct an ELF file by reading the segments out of remote
+   memory based on the ELF file header at EHDR_VMA and the ELF program
+   headers it points to.  If non-zero, SIZE is the known extent of the
+   object.  If not null, *LOADBASEP is filled in with the difference
+   between the VMAs from which the segments were read, and the VMAs
+   the file headers (and hence BFD's idea of each section's VMA) put
+   them at.
+
+   The function TARGET_READ_MEMORY is called to copy LEN bytes from
+   the remote memory at target address VMA into the local buffer at
+   MYADDR; it should return zero on success or an `errno' code on
+   failure.  TEMPL must be a BFD for a target with the word size and
+   byte order found in the remote memory.  */
 extern bfd *bfd_elf_bfd_from_remote_memory
-  (bfd *templ, bfd_vma ehdr_vma, bfd_vma *loadbasep,
+  (bfd *templ, bfd_vma ehdr_vma, size_t size, bfd_vma *loadbasep,
    int (*target_read_memory) (bfd_vma vma, bfd_byte *myaddr,
 			      bfd_size_type len));
 
diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
index ee6e6cb..6d07303 100644
--- a/bfd/elf-bfd.h
+++ b/bfd/elf-bfd.h
@@ -1191,9 +1191,9 @@ struct elf_backend_data
   /* This function implements `bfd_elf_bfd_from_remote_memory';
      see elf.c, elfcode.h.  */
   bfd *(*elf_backend_bfd_from_remote_memory)
-     (bfd *templ, bfd_vma ehdr_vma, bfd_vma *loadbasep,
-      int (*target_read_memory) (bfd_vma vma, bfd_byte *myaddr,
-				 bfd_size_type len));
+    (bfd *templ, bfd_vma ehdr_vma, size_t size, bfd_vma *loadbasep,
+     int (*target_read_memory) (bfd_vma vma, bfd_byte *myaddr,
+				bfd_size_type len));
 
   /* This function is used by `_bfd_elf_get_synthetic_symtab';
      see elf.c.  */
@@ -2334,10 +2334,10 @@ extern char *elfcore_write_ppc_linux_prpsinfo32
   (bfd *, char *, int *, const struct elf_internal_linux_prpsinfo *);
 
 extern bfd *_bfd_elf32_bfd_from_remote_memory
-  (bfd *templ, bfd_vma ehdr_vma, bfd_vma *loadbasep,
+  (bfd *templ, bfd_vma ehdr_vma, size_t size, bfd_vma *loadbasep,
    int (*target_read_memory) (bfd_vma, bfd_byte *, bfd_size_type));
 extern bfd *_bfd_elf64_bfd_from_remote_memory
-  (bfd *templ, bfd_vma ehdr_vma, bfd_vma *loadbasep,
+  (bfd *templ, bfd_vma ehdr_vma, size_t size, bfd_vma *loadbasep,
    int (*target_read_memory) (bfd_vma, bfd_byte *, bfd_size_type));
 
 extern bfd_vma bfd_elf_obj_attr_size (bfd *);
diff --git a/bfd/elf.c b/bfd/elf.c
index 3ded683..d67b917 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -9908,11 +9908,12 @@ bfd *
 bfd_elf_bfd_from_remote_memory
   (bfd *templ,
    bfd_vma ehdr_vma,
+   size_t size,
    bfd_vma *loadbasep,
    int (*target_read_memory) (bfd_vma, bfd_byte *, bfd_size_type))
 {
   return (*get_elf_backend_data (templ)->elf_backend_bfd_from_remote_memory)
-    (templ, ehdr_vma, loadbasep, target_read_memory);
+    (templ, ehdr_vma, size, loadbasep, target_read_memory);
 }
 \f
 long
diff --git a/bfd/elfcode.h b/bfd/elfcode.h
index 20101be..31f67a8 100644
--- a/bfd/elfcode.h
+++ b/bfd/elfcode.h
@@ -1587,22 +1587,25 @@ elf_debug_file (Elf_Internal_Ehdr *ehdrp)
 #endif
 \f
 /* Create a new BFD as if by bfd_openr.  Rather than opening a file,
-   reconstruct an ELF file by reading the segments out of remote memory
-   based on the ELF file header at EHDR_VMA and the ELF program headers it
-   points to.  If not null, *LOADBASEP is filled in with the difference
-   between the VMAs from which the segments were read, and the VMAs the
-   file headers (and hence BFD's idea of each section's VMA) put them at.
-
-   The function TARGET_READ_MEMORY is called to copy LEN bytes from the
-   remote memory at target address VMA into the local buffer at MYADDR; it
-   should return zero on success or an `errno' code on failure.  TEMPL must
-   be a BFD for a target with the word size and byte order found in the
-   remote memory.  */
+   reconstruct an ELF file by reading the segments out of remote
+   memory based on the ELF file header at EHDR_VMA and the ELF program
+   headers it points to.  If non-zero, SIZE is the known extent of the
+   object.  If not null, *LOADBASEP is filled in with the difference
+   between the VMAs from which the segments were read, and the VMAs
+   the file headers (and hence BFD's idea of each section's VMA) put
+   them at.
+
+   The function TARGET_READ_MEMORY is called to copy LEN bytes from
+   the remote memory at target address VMA into the local buffer at
+   MYADDR; it should return zero on success or an `errno' code on
+   failure.  TEMPL must be a BFD for a target with the word size and
+   byte order found in the remote memory.  */
 
 bfd *
 NAME(_bfd_elf,bfd_from_remote_memory)
   (bfd *templ,
    bfd_vma ehdr_vma,
+   size_t size,
    bfd_vma *loadbasep,
    int (*target_read_memory) (bfd_vma, bfd_byte *, bfd_size_type))
 {
@@ -1612,10 +1615,11 @@ NAME(_bfd_elf,bfd_from_remote_memory)
   Elf_Internal_Phdr *i_phdrs, *last_phdr;
   bfd *nbfd;
   struct bfd_in_memory *bim;
-  int contents_size;
   bfd_byte *contents;
   int err;
   unsigned int i;
+  bfd_vma high_offset;
+  bfd_vma shdr_end;
   bfd_vma loadbase;
   bfd_boolean loadbase_set;
 
@@ -1677,10 +1681,7 @@ NAME(_bfd_elf,bfd_from_remote_memory)
   x_phdrs = (Elf_External_Phdr *)
       bfd_malloc (i_ehdr.e_phnum * (sizeof *x_phdrs + sizeof *i_phdrs));
   if (x_phdrs == NULL)
-    {
-      bfd_set_error (bfd_error_no_memory);
-      return NULL;
-    }
+    return NULL;
   err = target_read_memory (ehdr_vma + i_ehdr.e_phoff, (bfd_byte *) x_phdrs,
 			    i_ehdr.e_phnum * sizeof x_phdrs[0]);
   if (err)
@@ -1692,34 +1693,44 @@ NAME(_bfd_elf,bfd_from_remote_memory)
     }
   i_phdrs = (Elf_Internal_Phdr *) &x_phdrs[i_ehdr.e_phnum];
 
-  contents_size = 0;
+  high_offset = 0;
   last_phdr = NULL;
-  loadbase = ehdr_vma;
+  loadbase = 0;
   loadbase_set = FALSE;
   for (i = 0; i < i_ehdr.e_phnum; ++i)
     {
       elf_swap_phdr_in (templ, &x_phdrs[i], &i_phdrs[i]);
       if (i_phdrs[i].p_type == PT_LOAD)
 	{
-	  bfd_vma segment_end;
-	  segment_end = (i_phdrs[i].p_offset + i_phdrs[i].p_filesz
-			 + i_phdrs[i].p_align - 1) & -i_phdrs[i].p_align;
-	  if (segment_end > (bfd_vma) contents_size)
-	    contents_size = segment_end;
-
-	  /* LOADADDR is the `Base address' from the gELF specification:
-	     `lowest p_vaddr value for a PT_LOAD segment' is P_VADDR from the
-	     first PT_LOAD as PT_LOADs are ordered by P_VADDR.  */
-	  if (!loadbase_set && (i_phdrs[i].p_offset & -i_phdrs[i].p_align) == 0)
+	  bfd_vma segment_end = i_phdrs[i].p_offset + i_phdrs[i].p_filesz;
+
+	  if (segment_end > high_offset)
 	    {
-	      loadbase = ehdr_vma - (i_phdrs[i].p_vaddr & -i_phdrs[i].p_align);
-	      loadbase_set = TRUE;
+	      high_offset = segment_end;
+	      last_phdr = &i_phdrs[i];
 	    }
 
-	  last_phdr = &i_phdrs[i];
+	  /* If this program header covers offset zero, where the file
+	     header sits, then we can figure out the loadbase.  */
+	  if (!loadbase_set)
+	    {
+	      bfd_vma p_offset = i_phdrs[i].p_offset;
+	      bfd_vma p_vaddr = i_phdrs[i].p_vaddr;
+
+	      if (i_phdrs[i].p_align > 1)
+		{
+		  p_offset &= -i_phdrs[i].p_align;
+		  p_vaddr &= -i_phdrs[i].p_align;
+		}
+	      if (p_offset == 0)
+		{
+		  loadbase = ehdr_vma - p_vaddr;
+		  loadbase_set = TRUE;
+		}
+	    }
 	}
     }
-  if (last_phdr == NULL)
+  if (high_offset == 0)
     {
       /* There were no PT_LOAD segments, so we don't have anything to read.  */
       free (x_phdrs);
@@ -1727,40 +1738,61 @@ NAME(_bfd_elf,bfd_from_remote_memory)
       return NULL;
     }
 
-  /* Trim the last segment so we don't bother with zeros in the last page
-     that are off the end of the file.  However, if the extra bit in that
-     page includes the section headers, keep them.  */
-  if ((bfd_vma) contents_size > last_phdr->p_offset + last_phdr->p_filesz
-      && (bfd_vma) contents_size >= (i_ehdr.e_shoff
-				     + i_ehdr.e_shnum * i_ehdr.e_shentsize))
+  shdr_end = 0;
+  if (i_ehdr.e_shoff != 0 && i_ehdr.e_shnum != 0 && i_ehdr.e_shentsize != 0)
     {
-      contents_size = last_phdr->p_offset + last_phdr->p_filesz;
-      if ((bfd_vma) contents_size < (i_ehdr.e_shoff
-				     + i_ehdr.e_shnum * i_ehdr.e_shentsize))
-	contents_size = i_ehdr.e_shoff + i_ehdr.e_shnum * i_ehdr.e_shentsize;
+      shdr_end = i_ehdr.e_shoff + i_ehdr.e_shnum * i_ehdr.e_shentsize;
+
+      if (last_phdr->p_filesz != last_phdr->p_memsz)
+	{
+	  /* If the last PT_LOAD header has a bss area then ld.so will
+	     have cleared anything past p_filesz, zapping the section
+	     headers.  */
+	}
+      else if (size >= shdr_end)
+	high_offset = shdr_end;
+      else
+	{
+	  bfd_vma page_size = get_elf_backend_data (templ)->minpagesize;
+	  bfd_vma segment_end = last_phdr->p_offset + last_phdr->p_filesz;
+
+	  /* Assume we loaded full pages, allowing us to sometimes see
+	     section headers.  */
+	  if (page_size > 1 && shdr_end > segment_end)
+	    {
+	      bfd_vma page_end = (segment_end + page_size - 1) & -page_size;
+
+	      if (page_end >= shdr_end)
+		/* Whee, section headers covered.  */
+		high_offset = shdr_end;
+	    }
+	}
     }
-  else
-    contents_size = last_phdr->p_offset + last_phdr->p_filesz;
 
   /* Now we know the size of the whole image we want read in.  */
-  contents = (bfd_byte *) bfd_zmalloc (contents_size);
+  contents = (bfd_byte *) bfd_zmalloc (high_offset);
   if (contents == NULL)
     {
       free (x_phdrs);
-      bfd_set_error (bfd_error_no_memory);
       return NULL;
     }
 
   for (i = 0; i < i_ehdr.e_phnum; ++i)
     if (i_phdrs[i].p_type == PT_LOAD)
       {
-	bfd_vma start = i_phdrs[i].p_offset & -i_phdrs[i].p_align;
-	bfd_vma end = (i_phdrs[i].p_offset + i_phdrs[i].p_filesz
-		       + i_phdrs[i].p_align - 1) & -i_phdrs[i].p_align;
-	if (end > (bfd_vma) contents_size)
-	  end = contents_size;
-	err = target_read_memory ((loadbase + i_phdrs[i].p_vaddr)
-				  & -i_phdrs[i].p_align,
+	bfd_vma start = i_phdrs[i].p_offset;
+	bfd_vma end = start + i_phdrs[i].p_filesz;
+	bfd_vma vaddr = i_phdrs[i].p_vaddr;
+
+	if (i_phdrs[i].p_align > 1)
+	  {
+	    start &= -i_phdrs[i].p_align;
+	    end = (end + i_phdrs[i].p_align - 1) & -i_phdrs[i].p_align;
+	    vaddr &= -i_phdrs[i].p_align;
+	  }
+	if (end > high_offset)
+	  end = high_offset;
+	err = target_read_memory (loadbase + vaddr,
 				  contents + start, end - start);
 	if (err)
 	  {
@@ -1775,8 +1807,7 @@ NAME(_bfd_elf,bfd_from_remote_memory)
 
   /* If the segments visible in memory didn't include the section headers,
      then clear them from the file header.  */
-  if ((bfd_vma) contents_size < (i_ehdr.e_shoff
-				 + i_ehdr.e_shnum * i_ehdr.e_shentsize))
+  if (high_offset < shdr_end)
     {
       memset (&x_ehdr.e_shoff, 0, sizeof x_ehdr.e_shoff);
       memset (&x_ehdr.e_shnum, 0, sizeof x_ehdr.e_shnum);
@@ -1792,7 +1823,6 @@ NAME(_bfd_elf,bfd_from_remote_memory)
   if (bim == NULL)
     {
       free (contents);
-      bfd_set_error (bfd_error_no_memory);
       return NULL;
     }
   nbfd = _bfd_new_bfd ();
@@ -1800,12 +1830,11 @@ NAME(_bfd_elf,bfd_from_remote_memory)
     {
       free (bim);
       free (contents);
-      bfd_set_error (bfd_error_no_memory);
       return NULL;
     }
   nbfd->filename = xstrdup ("<in-memory>");
   nbfd->xvec = templ->xvec;
-  bim->size = contents_size;
+  bim->size = high_offset;
   bim->buffer = contents;
   nbfd->iostream = bim;
   nbfd->flags = BFD_IN_MEMORY;
diff --git a/gdb/symfile-mem.c b/gdb/symfile-mem.c
index e3230de..98764cd 100644
--- a/gdb/symfile-mem.c
+++ b/gdb/symfile-mem.c
@@ -76,13 +76,14 @@ target_read_memory_bfd (bfd_vma memaddr, bfd_byte *myaddr, bfd_size_type len)
 }
 
 /* Read inferior memory at ADDR to find the header of a loaded object file
-   and read its in-core symbols out of inferior memory.  TEMPL is a bfd
+   and read its in-core symbols out of inferior memory.  SIZE, if
+   non-zero, is the known size of the object.  TEMPL is a bfd
    representing the target's format.  NAME is the name to use for this
    symbol file in messages; it can be NULL or a malloc-allocated string
    which will be attached to the BFD.  */
 static struct objfile *
-symbol_file_add_from_memory (struct bfd *templ, CORE_ADDR addr, char *name,
-			     int from_tty)
+symbol_file_add_from_memory (struct bfd *templ, CORE_ADDR addr,
+			     size_t size, char *name, int from_tty)
 {
   struct objfile *objf;
   struct bfd *nbfd;
@@ -95,7 +96,7 @@ symbol_file_add_from_memory (struct bfd *templ, CORE_ADDR addr, char *name,
   if (bfd_get_flavour (templ) != bfd_target_elf_flavour)
     error (_("add-symbol-file-from-memory not supported for this target"));
 
-  nbfd = bfd_elf_bfd_from_remote_memory (templ, addr, &loadbase,
+  nbfd = bfd_elf_bfd_from_remote_memory (templ, addr, size, &loadbase,
 					 target_read_memory_bfd);
   if (nbfd == NULL)
     error (_("Failed to read a valid object file image from memory."));
@@ -158,7 +159,7 @@ add_symbol_file_from_memory_command (char *args, int from_tty)
     error (_("Must use symbol-file or exec-file "
 	     "before add-symbol-file-from-memory."));
 
-  symbol_file_add_from_memory (templ, addr, NULL, from_tty);
+  symbol_file_add_from_memory (templ, addr, 0, NULL, from_tty);
 }
 
 /* Arguments for symbol_file_add_from_memory_wrapper.  */
@@ -167,6 +168,7 @@ struct symbol_file_add_from_memory_args
 {
   struct bfd *bfd;
   CORE_ADDR sysinfo_ehdr;
+  size_t size;
   char *name;
   int from_tty;
 };
@@ -179,8 +181,26 @@ symbol_file_add_from_memory_wrapper (struct ui_out *uiout, void *data)
 {
   struct symbol_file_add_from_memory_args *args = data;
 
-  symbol_file_add_from_memory (args->bfd, args->sysinfo_ehdr, args->name,
-			       args->from_tty);
+  symbol_file_add_from_memory (args->bfd, args->sysinfo_ehdr, args->size,
+			       args->name, args->from_tty);
+  return 0;
+}
+
+/* Rummage through mappings to find the vsyscall page size.  */
+
+static int
+find_vdso_size (CORE_ADDR vaddr, unsigned long size,
+		int read ATTRIBUTE_UNUSED, int write ATTRIBUTE_UNUSED,
+		int exec ATTRIBUTE_UNUSED, int modified ATTRIBUTE_UNUSED,
+		void *data)
+{
+  struct symbol_file_add_from_memory_args *args = data;
+
+  if (vaddr == args->sysinfo_ehdr)
+    {
+      args->size = size;
+      return 1;
+    }
   return 0;
 }
 
@@ -217,6 +237,11 @@ add_vsyscall_page (struct target_ops *target, int from_tty)
 	}
       args.bfd = bfd;
       args.sysinfo_ehdr = sysinfo_ehdr;
+      args.size = 0;
+      if (gdbarch_find_memory_regions_p (target_gdbarch ()))
+	(void) gdbarch_find_memory_regions (target_gdbarch (),
+					    find_vdso_size, &args);
+
       args.name = xstrprintf ("system-supplied DSO at %s",
 			      paddress (target_gdbarch (), sysinfo_ehdr));
       /* Pass zero for FROM_TTY, because the action of loading the


-- 
Alan Modra
Australia Development Lab, IBM

Re: vdso handling

[Pedro Alves]

On 03/28/2014 06:13 AM, Alan Modra wrote:
> On Fri, Mar 21, 2014 at 03:48:48PM +0000, Pedro Alves wrote:
>> I just tried pointing add-symbol-file-from-memory at an already
>> mapped DSO's elf header, but it doesn't work as is unfortunately:
>>
>>  (gdb) info shared curses
>>  0x000000324d006d20  0x000000324d01df58  Yes         /lib64/libncurses.so.5
>>  (gdb) x /4b 0x000000324d000000
>>  0x324d000000:   127     69      76      70
>>  (gdb) add-symbol-file-from-memory 0x000000324d000000
>>  Failed to read a valid object file image from memory.
>>
>> I single stepped a little through
>> bfd_elf_bfd_from_remote_memory - something goes wrong with the
>> reading of the load segment contents, probably something wrong
>> with the address computations.
> 
> readelf -a --wide on my x86_64 libncurses.so.5 shows
> 
> [snip]
>   Start of section headers:          132144 (bytes into file)
> [snip]
>   [25] .shstrtab         STRTAB          0000000000000000 02034c 0000de 00      0   0  1
> [snip]
>   LOAD           0x000000 0x0000000000000000 0x0000000000000000 0x01efe4 0x01efe4 R E 0x200000
>   LOAD           0x01fd50 0x000000000021fd50 0x000000000021fd50 0x0005e4 0x000770 RW  0x200000
> 
> So .shstrtab and the section headers might have been loaded by the
> second PT_LOAD header, *but* the second PT_LOAD has a bss area.
> Anything past 0x220334 will be cleared out by ld.so.  No chance of
> getting at section headers then, and this will be true for most
> in-memory images.

Indeed.

> bfd_from_remote_memory should take note of p_memsz..  Hmm, and there
> are quite a few other issues there too, most notably that p_align
> on x86_64 these days tends to be *much* larger than the page size used
> by ld.so.

Hmm.  Indeed.  With current mainline, and with your patch as is,
the command still fails for me.  In fact, it turns out
exactly related to p_align vs page size.

$ cat /proc/30669/maps | grep ncurses
324d000000-324d023000 r-xp 00000000 fd:01 315662                         /usr/lib64/libncurses.so.5.9
324d023000-324d222000 ---p 00023000 fd:01 315662                         /usr/lib64/libncurses.so.5.9
324d222000-324d223000 r--p 00022000 fd:01 315662                         /usr/lib64/libncurses.so.5.9
324d223000-324d224000 rw-p 00023000 fd:01 315662                         /usr/lib64/libncurses.so.5.9

So when trying to read the second PT_LOAD with p_vmaddr 324d222cf8
and p_vmaddr+p_filesz 324d2236b4, (the 3rd and 4th region above),
we'd end up reading from 324d200000 to 324d2236b4:

(top-gdb) p /x loadbase + vaddr
$5 = 0x324d200000
(top-gdb) p /x end
$6 = 0x236b4
(top-gdb) p /x loadbase + vaddr + end
$8 = 0x324d2236b4

which fails as it hits the (324d023000-324d222000) region,
which has no permissions.

This patch on top of yours makes things work for me:

---
 bfd/elfcode.h | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/bfd/elfcode.h b/bfd/elfcode.h
index 31f67a8..974c8b4 100644
--- a/bfd/elfcode.h
+++ b/bfd/elfcode.h
@@ -1622,6 +1622,7 @@ NAME(_bfd_elf,bfd_from_remote_memory)
   bfd_vma shdr_end;
   bfd_vma loadbase;
   bfd_boolean loadbase_set;
+  bfd_vma page_size;

   /* Read in the ELF header in external format.  */
   err = target_read_memory (ehdr_vma, (bfd_byte *) &x_ehdr, sizeof x_ehdr);
@@ -1693,6 +1694,7 @@ NAME(_bfd_elf,bfd_from_remote_memory)
     }
   i_phdrs = (Elf_Internal_Phdr *) &x_phdrs[i_ehdr.e_phnum];

+  page_size = get_elf_backend_data (templ)->minpagesize;
   high_offset = 0;
   last_phdr = NULL;
   loadbase = 0;
@@ -1753,7 +1755,6 @@ NAME(_bfd_elf,bfd_from_remote_memory)
 	high_offset = shdr_end;
       else
 	{
-	  bfd_vma page_size = get_elf_backend_data (templ)->minpagesize;
 	  bfd_vma segment_end = last_phdr->p_offset + last_phdr->p_filesz;

 	  /* Assume we loaded full pages, allowing us to sometimes see
@@ -1781,15 +1782,14 @@ NAME(_bfd_elf,bfd_from_remote_memory)
     if (i_phdrs[i].p_type == PT_LOAD)
       {
 	bfd_vma start = i_phdrs[i].p_offset;
-	bfd_vma end = start + i_phdrs[i].p_filesz;
 	bfd_vma vaddr = i_phdrs[i].p_vaddr;
+	bfd_vma end = start + i_phdrs[i].p_filesz;

-	if (i_phdrs[i].p_align > 1)
-	  {
-	    start &= -i_phdrs[i].p_align;
-	    end = (end + i_phdrs[i].p_align - 1) & -i_phdrs[i].p_align;
-	    vaddr &= -i_phdrs[i].p_align;
-	  }
+	/* Assume we loaded full pages, allowing us to sometimes see
+	   section headers.  */
+	start &= -page_size;
+	vaddr &= -page_size;
+	end = (end + page_size - 1) & -page_size;
 	if (end > high_offset)
 	  end = high_offset;
 	err = target_read_memory (loadbase + vaddr,
-- 
1.7.11.7


> Gah, I've been sucked into looking at this long enough that I may as
> well fix it.  Does this look OK?

It does to me.  Thanks!

-- 
Pedro Alves

Re: vdso handling

[Alan Modra]

On Fri, Mar 28, 2014 at 01:37:52PM +0000, Pedro Alves wrote:
> Hmm.  Indeed.  With current mainline, and with your patch as is,
> the command still fails for me.  In fact, it turns out
> exactly related to p_align vs page size.
> 
> $ cat /proc/30669/maps | grep ncurses
> 324d000000-324d023000 r-xp 00000000 fd:01 315662                         /usr/lib64/libncurses.so.5.9
> 324d023000-324d222000 ---p 00023000 fd:01 315662                         /usr/lib64/libncurses.so.5.9
> 324d222000-324d223000 r--p 00022000 fd:01 315662                         /usr/lib64/libncurses.so.5.9
> 324d223000-324d224000 rw-p 00023000 fd:01 315662                         /usr/lib64/libncurses.so.5.9
> 
> So when trying to read the second PT_LOAD with p_vmaddr 324d222cf8
> and p_vmaddr+p_filesz 324d2236b4, (the 3rd and 4th region above),
> we'd end up reading from 324d200000 to 324d2236b4:
> 
> (top-gdb) p /x loadbase + vaddr
> $5 = 0x324d200000
> (top-gdb) p /x end
> $6 = 0x236b4
> (top-gdb) p /x loadbase + vaddr + end
> $8 = 0x324d2236b4
> 
> which fails as it hits the (324d023000-324d222000) region,
> which has no permissions.

Ah ha!  What's more, if the read did happen to succeed you'd overwrite
contents written when processing the first PT_LOAD.  I believe that
will still happen with your fixup patch.  Not that it's a problem,
since ld.so reads the page holding the end of the first PT_LOAD and
the beginning of the second PT_LOAD twice, but I think it would be
better if BFD didn't rely on this ld.so behaviour (and an exact match
between BFD and ld.so's page size).

I believe the intent of rounding to a page was to pick up the file
and program headers at the start of a file and section headers at the
end, so let's do just that.  On top of my last patch:

diff --git a/bfd/elfcode.h b/bfd/elfcode.h
index 31f67a8..a005948 100644
--- a/bfd/elfcode.h
+++ b/bfd/elfcode.h
@@ -1612,7 +1612,7 @@ NAME(_bfd_elf,bfd_from_remote_memory)
   Elf_External_Ehdr x_ehdr;	/* Elf file header, external form */
   Elf_Internal_Ehdr i_ehdr;	/* Elf file header, internal form */
   Elf_External_Phdr *x_phdrs;
-  Elf_Internal_Phdr *i_phdrs, *last_phdr;
+  Elf_Internal_Phdr *i_phdrs, *last_phdr, *first_phdr;
   bfd *nbfd;
   struct bfd_in_memory *bim;
   bfd_byte *contents;
@@ -1621,7 +1621,6 @@ NAME(_bfd_elf,bfd_from_remote_memory)
   bfd_vma high_offset;
   bfd_vma shdr_end;
   bfd_vma loadbase;
-  bfd_boolean loadbase_set;
 
   /* Read in the ELF header in external format.  */
   err = target_read_memory (ehdr_vma, (bfd_byte *) &x_ehdr, sizeof x_ehdr);
@@ -1694,9 +1693,9 @@ NAME(_bfd_elf,bfd_from_remote_memory)
   i_phdrs = (Elf_Internal_Phdr *) &x_phdrs[i_ehdr.e_phnum];
 
   high_offset = 0;
-  last_phdr = NULL;
   loadbase = 0;
-  loadbase_set = FALSE;
+  first_phdr = NULL;
+  last_phdr = NULL;
   for (i = 0; i < i_ehdr.e_phnum; ++i)
     {
       elf_swap_phdr_in (templ, &x_phdrs[i], &i_phdrs[i]);
@@ -1712,7 +1711,7 @@ NAME(_bfd_elf,bfd_from_remote_memory)
 
 	  /* If this program header covers offset zero, where the file
 	     header sits, then we can figure out the loadbase.  */
-	  if (!loadbase_set)
+	  if (first_phdr == NULL)
 	    {
 	      bfd_vma p_offset = i_phdrs[i].p_offset;
 	      bfd_vma p_vaddr = i_phdrs[i].p_vaddr;
@@ -1725,7 +1724,7 @@ NAME(_bfd_elf,bfd_from_remote_memory)
 	      if (p_offset == 0)
 		{
 		  loadbase = ehdr_vma - p_vaddr;
-		  loadbase_set = TRUE;
+		  first_phdr = &i_phdrs[i];
 		}
 	    }
 	}
@@ -1784,13 +1783,15 @@ NAME(_bfd_elf,bfd_from_remote_memory)
 	bfd_vma end = start + i_phdrs[i].p_filesz;
 	bfd_vma vaddr = i_phdrs[i].p_vaddr;
 
-	if (i_phdrs[i].p_align > 1)
+	/* Extend the beginning of the first pt_load to cover file
+	   header and program headers.  */
+	if (first_phdr == &i_phdrs[i])
 	  {
-	    start &= -i_phdrs[i].p_align;
-	    end = (end + i_phdrs[i].p_align - 1) & -i_phdrs[i].p_align;
-	    vaddr &= -i_phdrs[i].p_align;
+	    vaddr -= start;
+	    start = 0;
 	  }
-	if (end > high_offset)
+	/* Extend the end of the last pt_load to cover section headers.  */
+	if (last_phdr == &i_phdrs[i])
 	  end = high_offset;
 	err = target_read_memory (loadbase + vaddr,
 				  contents + start, end - start);

-- 
Alan Modra
Australia Development Lab, IBM

Re: vdso handling

[Pedro Alves]

On 03/28/2014 11:00 PM, Alan Modra wrote:

> I believe the intent of rounding to a page was to pick up the file
> and program headers at the start of a file and section headers at the
> end, so let's do just that.  On top of my last patch:

Agreed.  This works for me.  Thanks!

> -	if (i_phdrs[i].p_align > 1)
> +	/* Extend the beginning of the first pt_load to cover file
> +	   header and program headers.  */
> +	if (first_phdr == &i_phdrs[i])

Minor nit:  Perhaps the comment could say "first pt_load
if it covers offset 0"?  The computation below confused me a little
until I scrolled up and realized that first_phdr is only set if
the first segment covers offset 0, not whatever the first segment
is.  (I'd even consider renaming it to zero_phdr or
zero_offset_phdr, but with the comment I'd already be
super happy).


On the GDB patch, sorry for not noticing earlier, but:

> +static int
> +find_vdso_size (CORE_ADDR vaddr, unsigned long size,
> +		int read ATTRIBUTE_UNUSED, int write ATTRIBUTE_UNUSED,
> +		int exec ATTRIBUTE_UNUSED, int modified ATTRIBUTE_UNUSED,
> +		void *data)
> +{

Please don't use ATTRIBUTE_UNUSED under gdb/, it'd be flagged by the
ARI as a regression:

gdb/contrib/gdb_ari.sh:

BEGIN { doc["ATTRIBUTE_UNUSED"] = "\
Do not use ATTRIBUTE_UNUSED, do not bother (GDB is compiled with -Werror and, \
consequently, is not able to tolerate false warnings.  Since -Wunused-param \
produces such warnings, neither that warning flag nor ATTRIBUTE_UNUSED \
are used by GDB"
    category["ATTRIBUTE_UNUSED"] = ari_regression
}
/(^|[^_[:alnum:]])ATTRIBUTE_UNUSED([^_[:alnum:]]|$)/ {
    fail("ATTRIBUTE_UNUSED")
}

-- 
Pedro Alves

Re: vdso handling

[Alan Modra]

On Tue, Apr 01, 2014 at 02:46:38PM +0100, Pedro Alves wrote:
> On 03/28/2014 11:00 PM, Alan Modra wrote:
> 
> > I believe the intent of rounding to a page was to pick up the file
> > and program headers at the start of a file and section headers at the
> > end, so let's do just that.  On top of my last patch:
> 
> Agreed.  This works for me.  Thanks!
> 
> > -	if (i_phdrs[i].p_align > 1)
> > +	/* Extend the beginning of the first pt_load to cover file
> > +	   header and program headers.  */
> > +	if (first_phdr == &i_phdrs[i])
> 
> Minor nit:  Perhaps the comment could say "first pt_load
> if it covers offset 0"?

Fixed.

	/* Extend the beginning of the first pt_load to cover file
	   header and program headers, if we proved earlier that its
	   aligned offset is 0.  */

> Please don't use ATTRIBUTE_UNUSED under gdb/, it'd be flagged by the
> ARI as a regression:

Hmm, OK.  Fixed and pushed.

-- 
Alan Modra
Australia Development Lab, IBM

RE: vdso handling

[Metzger, Markus T]

> -----Original Message-----
> From: Alan Modra [mailto:amodra@gmail.com]
> Sent: Wednesday, April 02, 2014 3:50 AM


> Hmm, OK.  Fixed and pushed.

Thanks,
Markus.
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052

Re: vdso handling

[Hans-Peter Nilsson]

On Fri, 28 Mar 2014, Alan Modra wrote:
>
> bfd_from_remote_memory should take note of p_memsz..  Hmm, and there
> are quite a few other issues there too, most notably that p_align
> on x86_64 these days tends to be *much* larger than the page size used
> by ld.so.
>
> Gah, I've been sucked into looking at this long enough that I may as
> well fix it.  Does this look OK?

The new size parameter uses size_t in bfd headers, breaking some
simulators like cris-elf, frv-elf, h8300-elf, iq2000-elf,
m32r-elf, mips-elf, mn10300-elf.

The obvious change is to instead use bfd_size_type, like
everything else in BFD headers.  Any reason not to do that here?

> 	* elfcode.h (bfd_from_remote_memory): Add "size" parameter.

brgds, H-P

Re: vdso handling

[Alan Modra]

On Wed, Apr 02, 2014 at 04:04:24AM -0400, Hans-Peter Nilsson wrote:
> The new size parameter uses size_t in bfd headers, breaking some
> simulators like cris-elf, frv-elf, h8300-elf, iq2000-elf,
> m32r-elf, mips-elf, mn10300-elf.
> 
> The obvious change is to instead use bfd_size_type, like
> everything else in BFD headers.  Any reason not to do that here?

I originally had bfd_size_type and wondered if we could use size_t,
which is the natural type to use here.  (bfd_size_type might be 64-bit
and it doesn't really make sense to use that when describing a memory
area on a 32-bit host.)  gdb and binutils built without trouble on a
few targets, so I went with size_t.  Sorry about the sim breakage.

	* elf-bfd.h (struct elf_backend_data
	<elf_backend_bfd_from_remote_memory>): Replace "size_t size"
	with "bfd_size_type size".
	(_bfd_elf32_bfd_from_remote_memory): Likewise.
	(_bfd_elf64_bfd_from_remote_memory): Likewise.
	* elf.c (bfd_elf_bfd_from_remote_memory): Likewise.
	* elfcode.h (bfd_from_remote_memory): Likewise.

diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
index 6d07303..f58a3b7 100644
--- a/bfd/elf-bfd.h
+++ b/bfd/elf-bfd.h
@@ -1191,7 +1191,7 @@ struct elf_backend_data
   /* This function implements `bfd_elf_bfd_from_remote_memory';
      see elf.c, elfcode.h.  */
   bfd *(*elf_backend_bfd_from_remote_memory)
-    (bfd *templ, bfd_vma ehdr_vma, size_t size, bfd_vma *loadbasep,
+    (bfd *templ, bfd_vma ehdr_vma, bfd_size_type size, bfd_vma *loadbasep,
      int (*target_read_memory) (bfd_vma vma, bfd_byte *myaddr,
 				bfd_size_type len));
 
@@ -2334,10 +2334,10 @@ extern char *elfcore_write_ppc_linux_prpsinfo32
   (bfd *, char *, int *, const struct elf_internal_linux_prpsinfo *);
 
 extern bfd *_bfd_elf32_bfd_from_remote_memory
-  (bfd *templ, bfd_vma ehdr_vma, size_t size, bfd_vma *loadbasep,
+  (bfd *templ, bfd_vma ehdr_vma, bfd_size_type size, bfd_vma *loadbasep,
    int (*target_read_memory) (bfd_vma, bfd_byte *, bfd_size_type));
 extern bfd *_bfd_elf64_bfd_from_remote_memory
-  (bfd *templ, bfd_vma ehdr_vma, size_t size, bfd_vma *loadbasep,
+  (bfd *templ, bfd_vma ehdr_vma, bfd_size_type size, bfd_vma *loadbasep,
    int (*target_read_memory) (bfd_vma, bfd_byte *, bfd_size_type));
 
 extern bfd_vma bfd_elf_obj_attr_size (bfd *);
diff --git a/bfd/elf.c b/bfd/elf.c
index d67b917..9e46f7c 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -9908,7 +9908,7 @@ bfd *
 bfd_elf_bfd_from_remote_memory
   (bfd *templ,
    bfd_vma ehdr_vma,
-   size_t size,
+   bfd_size_type size,
    bfd_vma *loadbasep,
    int (*target_read_memory) (bfd_vma, bfd_byte *, bfd_size_type))
 {
diff --git a/bfd/elfcode.h b/bfd/elfcode.h
index f840065..a49a708 100644
--- a/bfd/elfcode.h
+++ b/bfd/elfcode.h
@@ -1605,7 +1605,7 @@ bfd *
 NAME(_bfd_elf,bfd_from_remote_memory)
   (bfd *templ,
    bfd_vma ehdr_vma,
-   size_t size,
+   bfd_size_type size,
    bfd_vma *loadbasep,
    int (*target_read_memory) (bfd_vma, bfd_byte *, bfd_size_type))
 {


-- 
Alan Modra
Australia Development Lab, IBM

Re: vdso handling

[Alan Modra]

On Thu, Apr 03, 2014 at 11:36:44AM +1030, Alan Modra wrote:
> 	* elf-bfd.h (struct elf_backend_data
> 	<elf_backend_bfd_from_remote_memory>): Replace "size_t size"
> 	with "bfd_size_type size".
> 	(_bfd_elf32_bfd_from_remote_memory): Likewise.
> 	(_bfd_elf64_bfd_from_remote_memory): Likewise.
> 	* elf.c (bfd_elf_bfd_from_remote_memory): Likewise.
> 	* elfcode.h (bfd_from_remote_memory): Likewise.

Sigh.  Here too.

	* bfd-in.h (bfd_elf_bfd_from_remote_memory): Likewise.
	* bfd-in2.h: Regenerate.

-- 
Alan Modra
Australia Development Lab, IBM