diff mbox

vdso handling

Message ID 20140328230037.GW18201@bubble.grove.modra.org
State Committed
Headers show

Commit Message

Alan Modra March 28, 2014, 11 p.m. UTC
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:

Comments

Pedro Alves April 1, 2014, 1:46 p.m. UTC | #1
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")
}
Alan Modra April 2, 2014, 1:49 a.m. UTC | #2
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.
Metzger, Markus T April 2, 2014, 8:03 a.m. UTC | #3
> -----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
diff mbox

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