[patch] Fix bfd_elf_bfd_from_remote_memory() loadbase

[patch] Fix bfd_elf_bfd_from_remote_memory() loadbase

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 1693 bytes --]

Hi,

bfd_elf_bfd_from_remote_memory() is currently in use only for vDSOs where it
works fine.  It fails for real memory-mapped ELF files, though.

On the other hand I do not need this fix as my build-id GDB patch no longer
uses bfd_elf_bfd_from_remote_memory() (as it was performance-ineffective).


My memory-mapped dummy test library has:
  Type           Offset   VirtAddr           PhysAddr           FileSiz  MemSiz   Flg Align
  LOAD           0x000000 0x0000000000000000 0x0000000000000000 0x0005dc 0x0005dc R E 0x200000
  LOAD           0x0005e0 0x00000000002005e0 0x00000000002005e0 0x0001e8 0x0001f8 RW  0x200000

which sets the LOADBASE variable by the current BFD CVS HEAD for both for the
first and for the second PT_LOAD segment by

        if (loadbase == ehdr_vma)
                loadbase = ehdr_vma - (i_phdrs[i].p_vaddr & -i_phdrs[i].p_align);

This command has no effect on the first PT_LOAD there (as P_VADDR is zero) and
so even the second PT_LOAD gets processed.

I believe a simple rule "just the first PT_LOAD segment" implemented in my
patch should be enough.

The current PF_R condition there targets ia64 vDSO which has:
  Type           Offset   VirtAddr           PhysAddr           FileSiz  MemSiz   Flg Align
  LOAD           0x000000 0xa000000000000000 0xa000000000000000 0x000610 0x000610 R   0x10000
  LOAD           0x000000 0xa000000000010000 0xa000000000010000 0x0009d0 0x0009d0   E 0x10000
so the (i_phdrs[i].p_flags & PF_R) condition picked the first one (the right
one).  Still this condition is needed there as the memory needs to be readable.


Tested on x86_64 on GDB testsuite and on some IA64 vDSO GDB accesses with no
regressions.


Regards,
Jan

[-- Attachment #2: bfd-loadbase.patch --]
[-- Type: text/plain, Size: 2590 bytes --]

2007-08-10  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* elfcode.h (NAME(_bfd_elf,bfd_from_remote_memory)): LOADBASE is now
	initialized only on the first PT_LOAD.  New variable LOADBASE_SET.
	Moved the IA-64 vDSO handling comments to the LOADBASE assignment.

--- bfd/elfcode.h	4 Aug 2007 16:31:00 -0000	1.85
+++ bfd/elfcode.h	10 Aug 2007 14:46:27 -0000
@@ -1635,6 +1635,7 @@ NAME(_bfd_elf,bfd_from_remote_memory)
   int err;
   unsigned int i;
   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);
@@ -1711,12 +1712,11 @@ NAME(_bfd_elf,bfd_from_remote_memory)
   contents_size = 0;
   last_phdr = NULL;
   loadbase = ehdr_vma;
+  loadbase_set = FALSE;
   for (i = 0; i < i_ehdr.e_phnum; ++i)
     {
       elf_swap_phdr_in (templ, &x_phdrs[i], &i_phdrs[i]);
-      /* IA-64 vDSO may have two mappings for one segment, where one mapping
-	 is executable only, and one is read only.  We must not use the
-	 executable one.  */
+      /* PF_R as we need to read the segment memory.  */
       if (i_phdrs[i].p_type == PT_LOAD && (i_phdrs[i].p_flags & PF_R))
 	{
 	  bfd_vma segment_end;
@@ -1725,8 +1725,18 @@ NAME(_bfd_elf,bfd_from_remote_memory)
 	  if (segment_end > (bfd_vma) contents_size)
 	    contents_size = segment_end;
 
-	  if ((i_phdrs[i].p_offset & -i_phdrs[i].p_align) == 0)
-	    loadbase = ehdr_vma - (i_phdrs[i].p_vaddr & -i_phdrs[i].p_align);
+	  /* Only the first PT_LOAD segment indicates the file bias.
+	     Next segments may have P_VADDR arbitrarily higher.
+	     If the first segment has P_VADDR zero any next segment must not
+	     confuse us, the first one sets LOADBASE certainly enough.
+	     IA-64 vDSO may have two mappings for one segment, where the first
+	     mapping is executable only, and the second one is read only.
+	     We must use the first one.  */
+	  if (!loadbase_set && i_phdrs[i].p_offset == 0)
+	    {
+	      loadbase = ehdr_vma - i_phdrs[i].p_vaddr;
+	      loadbase_set = TRUE;
+	    }
 
 	  last_phdr = &i_phdrs[i];
 	}
@@ -1764,9 +1774,7 @@ NAME(_bfd_elf,bfd_from_remote_memory)
     }
 
   for (i = 0; i < i_ehdr.e_phnum; ++i)
-    /* IA-64 vDSO may have two mappings for one segment, where one mapping
-       is executable only, and one is read only.  We must not use the
-       executable one.  */
+    /* PF_R as we need to read the segment memory.  */
     if (i_phdrs[i].p_type == PT_LOAD && (i_phdrs[i].p_flags & PF_R))
       {
 	bfd_vma start = i_phdrs[i].p_offset & -i_phdrs[i].p_align;

Re: [patch] Fix bfd_elf_bfd_from_remote_memory() loadbase

[Alan Modra]

On Fri, Aug 10, 2007 at 05:15:46PM +0200, Jan Kratochvil wrote:
> I believe a simple rule "just the first PT_LOAD segment" implemented in my
> patch should be enough.

No.  Please fix this properly as per the ELF gABI.

"An executable or shared object file's base address (on platforms that
support the concept) is calculated during execution from three values:
the virtual memory load address, the maximum page size, and the lowest
virtual address of a program's loadable segment. To compute the base
address, one determines the memory address associated with the lowest
p_vaddr value for a PT_LOAD segment. This address is truncated to the
nearest multiple of the maximum page size. The corresponding p_vaddr
value itself is also truncated to the nearest multiple of the maximum
page size. The base address is the difference between the truncated
memory address and the truncated p_vaddr value."

-- 
Alan Modra
Australia Development Lab, IBM

Re: [patch] Fix bfd_elf_bfd_from_remote_memory() loadbase

[Roland McGrath]

> On Fri, Aug 10, 2007 at 05:15:46PM +0200, Jan Kratochvil wrote:
> > I believe a simple rule "just the first PT_LOAD segment" implemented in my
> > patch should be enough.
> 
> No.  Please fix this properly as per the ELF gABI.

Sorry, reading your citation from the spec did not help me have any idea
what you think "fix this properly" means in this context.

The situation is that we have some phdrs, and we know an address that is
the lowest virtual address corresponding to file offset 0.  The task at
hand is to calculate the base address from that knowledge.

In a correct object per the spec, "PT_LOAD with lower p_vaddr" is the
same as "PT_LOAD with lower index".  Ergo, the first PT_LOAD with
(p_offset & -maxpage) == 0 is the one whose (p_vaddr & -maxpage)
describes the lowest-addresse mapping of file offset 0.  Ergo, the
difference between this and the known address we started with is the
base address we are looking for.  

The file itself doesn't tell you what "maxpage" is, but p_align is
supposed to match it.  You can't presume the actual memory addresses
used will be so aligned (it's the maximum page size, not the minimum).
However, in a correct ELF object's phdrs, each p_vaddr and p_offset
must be congruent to that alignment.  So (p_offset & -p_align) == 0 is
enough to tell you that the system could have mapped that segment such
that (p_vaddr & -p_align) came from file offset 0.  From there, it's a
mild assumption of normal practice to guess that the first (thus
lowest-addressed) such segment is the (first) one actually mapped from
offset 0.  Technically, a valid object could have:

	PT_LOAD p_vaddr=0x201000 p_offset=0x1000 p_align=0x200000
	PT_LOAD p_vaddr=0x400000 p_offset=0	 p_align=0x200000

If mapped with an actual page size of 0x1000, then the assumption is
wrong.  So to answer the general question "does this PT_LOAD segment
include file offset 0 in memory", you need to know the actual page
size.  bfd_elf_bfd_from_remote_memory doesn't know this.  (The
debugger might know it by some external means.  While there is no
detailed specification for ET_CORE files, the de facto standard is
that p_align in core file PT_LOAD segments gives the actual page size.
For live debugging, there are OS-specific ways to find out.)

However, for the task at hand you are starting from an address that is
a priori both the lowest address mapped and the one mapped at offset 0.  
So the assumption is completely safe for this purpose.

Jan's change to includes only segments with p_offset==0 is incorrect.
In practice today, those are the phdrs we see.  But they could very
well be p_vaddr=0x123 p_offset=0x123 instead.

Jan's change to use only the first segment that maps file offset 0 is
correct.  The old change to ignore segments without PF_R was incorrect
for the general case, though handled the only real-world case where the
function was used on phdrs of which more than one maps file offset 0
(the Linux/IA64 vDSO, whose first such mapping has PF_R).

Jan's new comment explaining the PF_R check is incorrect.  It may once
have been the case in Linux that the debugger was unable to read memory
mapped without read permission, but that has certainly not been true
recently.  I think this check may have been introduced solely as a
heuristic to handle the ia64 vDSO.  This is IMO much better addressed
by correcting the base address calculation to use the first PT_LOAD
mapping file offset 0 rather than the last, as Jan's change does.



Thanks,
Roland

Re: [patch] Fix bfd_elf_bfd_from_remote_memory() loadbase

[Alan Modra]

On Sun, Aug 12, 2007 at 06:15:44PM -0700, Roland McGrath wrote:
> > On Fri, Aug 10, 2007 at 05:15:46PM +0200, Jan Kratochvil wrote:
> > > I believe a simple rule "just the first PT_LOAD segment" implemented in my
> > > patch should be enough.
> > 
> > No.  Please fix this properly as per the ELF gABI.
> 
> Sorry, reading your citation from the spec did not help me have any idea
> what you think "fix this properly" means in this context.
> 
> The situation is that we have some phdrs, and we know an address that is
> the lowest virtual address corresponding to file offset 0.  The task at
> hand is to calculate the base address from that knowledge.
> 
> In a correct object per the spec, "PT_LOAD with lower p_vaddr" is the
> same as "PT_LOAD with lower index".  Ergo, the first PT_LOAD with
> (p_offset & -maxpage) == 0 is the one whose (p_vaddr & -maxpage)
> describes the lowest-addresse mapping of file offset 0.

The test on p_offset was the thing that made me reject Jan's patch.
The spec doesn't even mention p_offset entering the equation for
calculating the base address.  What I missed, due to not looking at
this function's parameters, was that in this case we know one of the
program headers loaded the ELF header into memory, and indeed the
memory address we have is that for the ELF header.

What I'd like to see is:
- remove the nonsense PF_R tests.
- set loadbase using the first header with (p_offset & -p_align) == 0

-- 
Alan Modra
Australia Development Lab, IBM

Re: [patch] Fix bfd_elf_bfd_from_remote_memory() loadbase

[Roland McGrath]

> What I'd like to see is:
> - remove the nonsense PF_R tests.
> - set loadbase using the first header with (p_offset & -p_align) == 0

I'd agree with those changes.  It is probably also appropriate to change
how the later part of the function works (the actual data reading).
Removing the PF_R check from the second loop in the existing code will
lead to a redundant second read of the first page in the ia64 vdso case.
But that is a further refinement.


Thanks,
Roland

Re: [patch] Fix bfd_elf_bfd_from_remote_memory() loadbase

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 3488 bytes --]

On Mon, 13 Aug 2007 05:56:54 +0200, Alan Modra wrote:
...
> - remove the nonsense PF_R tests.
> - set loadbase using the first header with (p_offset & -p_align) == 0

On Mon, 13 Aug 2007 06:14:30 +0200, Roland McGrath wrote:
...
> It is probably also appropriate to change how the later part of the function
> works (the actual data reading).  Removing the PF_R check from the second
> loop in the existing code

[attached]
GDB testsuite ran on x86_64; vDSO manually tested on IA-64.


On Mon, 13 Aug 2007 03:15:44 +0200, Roland McGrath wrote:
...
> Jan's new comment explaining the PF_R check is incorrect.  It may once
> have been the case in Linux that the debugger was unable to read memory
> mapped without read permission, but that has certainly not been true
> recently.

While it is Linux kernel dependent (kernel-2.6.21-1.3228.fc7.x86_64) I have
made a test [attached] of this behavior with the results you describe.

Still some segments / segments parts may be missing if using a core file
backend but it is the responsibility of the TARGET_READ_MEMORY callback to
never report error - or possibly a fix out of scope of this patch.


Regards,
Jan


prot: @address self-Read(requested)->(permitted) self-Write self-eXecute ptrace-Read(permitted) ptrace-Write
00: @0x2aaaaaaae000 R(-)->(-) W(-)->(-) X(-)->(-) pR(+) pW(+)
01: @0x2aaaaaab0000 R(-)->(+) W(-)->(-) X(+)->(-) pR(+) pW(+)
02: @0x2aaaaaab2000 R(-)->(+) W(+)->(+) X(-)->(-) pR(+) pW(+)
03: @0x2aaaaaab4000 R(-)->(+) W(+)->(+) X(+)->(-) pR(+) pW(+)
04: @0x2aaaaaab6000 R(+)->(+) W(-)->(-) X(-)->(+) pR(+) pW(+)
05: @0x2aaaaaab8000 R(+)->(+) W(-)->(-) X(+)->(+) pR(+) pW(+)
06: @0x2aaaaaaba000 R(+)->(+) W(+)->(+) X(-)->(+) pR(+) pW(+)
07: @0x2aaaaaabc000 R(+)->(+) W(+)->(+) X(+)->(+) pR(+) pW(+)
/proc/PID/maps:
2aaaaaaae000-2aaaaaaaf000 ---p 2aaaaaaae000 00:00 0
2aaaaaab0000-2aaaaaab1000 r--p 2aaaaaab0000 00:00 0
2aaaaaab2000-2aaaaaab3000 -w-p 2aaaaaab2000 00:00 0
2aaaaaab4000-2aaaaaab5000 rw-p 2aaaaaab4000 00:00 0
2aaaaaab6000-2aaaaaab7000 --xp 2aaaaaab6000 00:00 0
2aaaaaab8000-2aaaaaab9000 r-xp 2aaaaaab8000 00:00 0
2aaaaaaba000-2aaaaaabb000 -wxp 2aaaaaaba000 00:00 0
2aaaaaabc000-2aaaaaabd000 rwxp 2aaaaaabc000 00:00 0
core:
Program Headers:
  Type           Offset             VirtAddr           PhysAddr
                 FileSiz            MemSiz              Flags  Align
  LOAD           0x0000000000010000 0x00002aaaaaaae000 0x0000000000000000
                 0x0000000000001000 0x0000000000001000         1000
  LOAD           0x0000000000011000 0x00002aaaaaab0000 0x0000000000000000
                 0x0000000000001000 0x0000000000001000  R      1000
  LOAD           0x0000000000012000 0x00002aaaaaab2000 0x0000000000000000
                 0x0000000000001000 0x0000000000001000   W     1000
  LOAD           0x0000000000013000 0x00002aaaaaab4000 0x0000000000000000
                 0x0000000000001000 0x0000000000001000  RW     1000
  LOAD           0x0000000000014000 0x00002aaaaaab6000 0x0000000000000000
                 0x0000000000001000 0x0000000000001000    E    1000
  LOAD           0x0000000000015000 0x00002aaaaaab8000 0x0000000000000000
                 0x0000000000001000 0x0000000000001000  R E    1000
  LOAD           0x0000000000016000 0x00002aaaaaaba000 0x0000000000000000
                 0x0000000000001000 0x0000000000001000   WE    1000
  LOAD           0x0000000000017000 0x00002aaaaaabc000 0x0000000000000000
                 0x0000000000001000 0x0000000000001000  RWE    1000

[-- Attachment #2: bfd-loadbase2.patch --]
[-- Type: text/plain, Size: 2508 bytes --]

2007-08-13  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* elfcode.h (NAME(_bfd_elf,bfd_from_remote_memory)): LOADBASE is now
	initialized only on the first PT_LOAD.  New variable LOADBASE_SET.
	Removed PF_R checking for IA-64 vDSOs as redundant now.
	Code advisory: Roland McGrath

--- bfd/elfcode.h	4 Aug 2007 16:31:00 -0000	1.85
+++ bfd/elfcode.h	13 Aug 2007 20:52:25 -0000
@@ -1635,6 +1635,7 @@ NAME(_bfd_elf,bfd_from_remote_memory)
   int err;
   unsigned int i;
   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);
@@ -1711,13 +1712,11 @@ NAME(_bfd_elf,bfd_from_remote_memory)
   contents_size = 0;
   last_phdr = NULL;
   loadbase = ehdr_vma;
+  loadbase_set = FALSE;
   for (i = 0; i < i_ehdr.e_phnum; ++i)
     {
       elf_swap_phdr_in (templ, &x_phdrs[i], &i_phdrs[i]);
-      /* IA-64 vDSO may have two mappings for one segment, where one mapping
-	 is executable only, and one is read only.  We must not use the
-	 executable one.  */
-      if (i_phdrs[i].p_type == PT_LOAD && (i_phdrs[i].p_flags & PF_R))
+      if (i_phdrs[i].p_type == PT_LOAD)
 	{
 	  bfd_vma segment_end;
 	  segment_end = (i_phdrs[i].p_offset + i_phdrs[i].p_filesz
@@ -1725,8 +1724,14 @@ NAME(_bfd_elf,bfd_from_remote_memory)
 	  if (segment_end > (bfd_vma) contents_size)
 	    contents_size = segment_end;
 
-	  if ((i_phdrs[i].p_offset & -i_phdrs[i].p_align) == 0)
-	    loadbase = ehdr_vma - (i_phdrs[i].p_vaddr & -i_phdrs[i].p_align);
+	  /* 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)
+	    {
+	      loadbase = ehdr_vma - (i_phdrs[i].p_vaddr & -i_phdrs[i].p_align);
+	      loadbase_set = TRUE;
+	    }
 
 	  last_phdr = &i_phdrs[i];
 	}
@@ -1764,10 +1769,7 @@ NAME(_bfd_elf,bfd_from_remote_memory)
     }
 
   for (i = 0; i < i_ehdr.e_phnum; ++i)
-    /* IA-64 vDSO may have two mappings for one segment, where one mapping
-       is executable only, and one is read only.  We must not use the
-       executable one.  */
-    if (i_phdrs[i].p_type == PT_LOAD && (i_phdrs[i].p_flags & PF_R))
+    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

[-- Attachment #3: mmapprot.c --]
[-- Type: text/plain, Size: 3171 bytes --]

#include <sys/mman.h>
#include <stdlib.h>
#include <assert.h>
#include <asm/page.h>
#include <stdio.h>
#include <unistd.h>
#include <setjmp.h>
#include <signal.h>
#include <string.h>
#include <sys/ptrace.h>
#include <sys/wait.h>
#include <errno.h>
#include <limits.h>

static sigjmp_buf env;

static void sigsegv (int signo)
{
  assert (signo == SIGSEGV);
  siglongjmp (env, 1);
  assert (0);
}

static void funcret (void)
{
}

static void funcafter (void)
{
}

int main (void)
{
  unsigned prot;
  char command[LINE_MAX];
  int status;
  
  puts ("prot: @address self-Read(requested)->(permitted) self-Write self-eXecute ptrace-Read(permitted) ptrace-Write");
  signal (SIGSEGV, sigsegv);

  for (prot = 0; prot < 010; prot++)
    {
      void *p;
      volatile int prep;
      pid_t child;
      int i;

      p = mmap (NULL, 2 * PAGE_SIZE,
		0
		  | (prot & 04 ? PROT_READ  : 0)
		  | (prot & 02 ? PROT_WRITE : 0)
		  | (prot & 01 ? PROT_EXEC  : 0),
		MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
      if (p == MAP_FAILED)
        {
	  printf ("0%o failed\n", prot);
	  continue;
	}
      i = munmap (p, PAGE_SIZE);
      assert (i == 0);
      p += PAGE_SIZE;
      printf ("0%o: @%p", prot, p);

      printf (" R(%c)->", prot & 04 ? '+' : '-');
      if (!sigsetjmp (env, 1))
	{
	  volatile long v = *((volatile long *) p);
	  v;
	  fputs ("(+)", stdout);
	}
      else
	fputs ("(-)", stdout);

      printf (" W(%c)->", prot & 02 ? '+' : '-');
      if (!sigsetjmp (env, 1))
	{
	  *((volatile long *) p) = 1;
	  fputs ("(+)", stdout);
	}
      else
	fputs ("(-)", stdout);

      printf (" X(%c)->", prot & 01 ? '+' : '-');
      if (!sigsetjmp (env, 1))
	{
	  prep = 1;
	  i = mprotect (p, PAGE_SIZE, PROT_WRITE);
	  assert (i == 0);
	  memcpy (p, funcret, (char *) funcafter - (char *) funcret);
	  i = mprotect (p, PAGE_SIZE, prot);
	  assert (i == 0);
	  prep = 0;
	  ((void (*) (void)) p) ();
	  fputs ("(+)", stdout);
	}
      else
	{
	  if (prep)
	    fputs ("(*)", stdout);
	  else
	    fputs ("(-)", stdout);
	}

      child = fork ();
      switch (child)
	{
	  case -1: assert (0);
	  case 0:
	    {
	      long l;

	      l = ptrace (PTRACE_TRACEME, 0, NULL, NULL);
	      assert (l == 0);
	      raise (SIGSTOP);
	      _exit (0);
	    }
	  default:
	    {
	      long l;
	      pid_t pid_got;

	      pid_got = waitpid (child, &status, 0);
	      assert (pid_got == child);
	      assert (WIFSTOPPED (status));
	      assert (WSTOPSIG (status) == SIGSTOP);

	      errno = 0;
	      l = ptrace (PTRACE_PEEKDATA, child, p, NULL);
	      printf (" pR(%c)", errno == 0 ? '+' : '-');

	      l = ptrace (PTRACE_POKEDATA, child, p, 1);
	      printf (" pW(%c)", l == 0 ? '+' : '-');

	      l = ptrace (PTRACE_DETACH, child, NULL, NULL);
	      assert (l == 0);

	      pid_got = waitpid (child, &status, 0);
	      assert (pid_got == child);
	      assert (WIFEXITED (status));
	      assert (WEXITSTATUS (status) == 0);
	    }
	}

      putchar ('\n');
    }
  snprintf (command, sizeof command, "cat /proc/%d/maps", (int) getpid ());
  status = system (command);
  assert (WIFEXITED (status));
  assert (WEXITSTATUS (status) == 0);
  abort ();
  return 0;
}

Re: [patch] Fix bfd_elf_bfd_from_remote_memory() loadbase

[Alan Modra]

On Mon, Aug 13, 2007 at 11:27:52PM +0200, Jan Kratochvil wrote:
> 	* elfcode.h (NAME(_bfd_elf,bfd_from_remote_memory)): LOADBASE is now
> 	initialized only on the first PT_LOAD.  New variable LOADBASE_SET.
> 	Removed PF_R checking for IA-64 vDSOs as redundant now.
> 	Code advisory: Roland McGrath

OK.

-- 
Alan Modra
Australia Development Lab, IBM

loadbase alignment - ld.so/prelink/kernel or bfd_elf_bfd_from_remote_memory() bug?

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 4775 bytes --]

Hi,

bfd_elf_bfd_from_remote_memory() would almost work for general ELF files but it
fails (I have no simple testcase) now due to the ELF `Base address' alignment.
BFD uses `& -i_phdrs[i].p_align' for the VMA addresses but the real mapping
is only PAGE_SIZE aligned (illustrated in the `BEFORE' dumps below).

For x86_64:

* bfd_elf_bfd_from_remote_memory() expects `loadbase' is P_ALIGN aligned.

  * GCC on x86_64 produces P_ALIGN 0x200000 == 2MB.

* ld.so loads ELF to `l_map_start' being only PAGE_SIZE aligned.
  (The same problem applies to prelink and Linux kernel elf loading.)

* gELF standard in its `Base Address' computation expects the `base address' to
  be `maximum page size' aligned, according to the last sentence:
  	http://x86.ddj.com/ftp/manuals/tools/elf.pdf
  	This address is truncated to the nearest multiple of the maximum page
  	size.  The corresponding p_vaddr value itself is also truncated to the
  	nearest multiple of the maximum page size. The base address is the
  	difference between the truncated memory address and the truncated
  	p_vaddr value.

  * x86_64 ELF standard in its `3.3.3 Page Size' talks about maximum page size
    64KB (I would expect it should scale up to 2MB for the 2MB PSE pages).
    	http://www.x86-64.org/documentation/abi.pdf
    	Systems are permitted to use any power-of-two page size between 4KB and
    	64KB, inclusive.

According to the Roland's mail below (p_align is supposed to match "maxpage")
IMO ld.so + prelink + kernel violate the gELF standard.

Proper ld.so P_ALIGN-compliant mapping would also make possible a kernel 2MB
PSE pages optimized mapping for very large .so libraries.

Removing P_ALIGN masking from bfd_elf_bfd_from_remote_memory() workarounds it
well but I expect this problem should move to glibc + prelink + kernel, right?


Proposing ld.so patch which fixes the bfd_elf_bfd_from_remote_memory() problem.

Program Headers:
  Type           Offset             VirtAddr           PhysAddr
                 FileSiz            MemSiz              Flags  Align
  LOAD           0x0000000000000000 0x0000000000000000 0x0000000000000000
                 0x00000000000004bc 0x00000000000004bc  R E    200000
  LOAD           0x00000000000004c0 0x00000000002004c0 0x00000000002004c0
                 0x00000000000001e8 0x00000000000001f8  RW     200000

BEFORE:
00400000-00401000 r-xp 00000000 08:01 4717502                            /tmp/alignmain
00600000-00601000 rw-p 00000000 08:01 4717502                            /tmp/alignmain
...
2aaaaaaad000-2aaaaaaae000 r-xp 00000000 08:01 4717503                    /tmp/alignlib.so
2aaaaaaae000-2aaaaacad000 ---p 00001000 08:01 4717503                    /tmp/alignlib.so
2aaaaacad000-2aaaaacae000 rw-p 00000000 08:01 4717503                    /tmp/alignlib.so

open("./alignlib.so", O_RDONLY)         = 3
...
mmap(NULL, 2098872, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = 0x2aaaaaaad000
mprotect(0x2aaaaaaae000, 2093056, PROT_NONE) = 0
mmap(0x2aaaaacad000, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0) = 0x2aaaaacad000
close(3)                                = 0


AFTER:
00400000-00401000 r-xp 00000000 08:01 4717502                            /tmp/alignmain
00600000-00601000 rw-p 00000000 08:01 4717502                            /tmp/alignmain
...
2aaaaac00000-2aaaaac01000 r-xp 00000000 08:01 4717503                    /tmp/alignlib.so
2aaaaac01000-2aaaaae00000 ---p 2aaaaac01000 00:00 0 
2aaaaae00000-2aaaaae01000 rw-p 00000000 08:01 4717503                    /tmp/alignlib.so

open("./alignlib.so", O_RDONLY)         = 3
...
mmap(NULL, 4194304, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_ANONYMOUS|MAP_DENYWRITE, -1, 0) = 0x2aaaaaaad000
munmap(0x2aaaaaaad000, 1388544)         = 0
munmap(0x2aaaaae01000, 704512)          = 0
mprotect(0x2aaaaac01000, 2093056, PROT_NONE) = 0
mmap(0x2aaaaac00000, 4096, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0) = 0x2aaaaac00000
mmap(0x2aaaaae00000, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0) = 0x2aaaaae00000
close(3)                                = 0



Regards,
Jan


http://sourceware.org/ml/binutils/2007-08/msg00184.html

On Mon, 13 Aug 2007 03:15:44 +0200, Roland McGrath wrote:
...
> The file itself doesn't tell you what "maxpage" is, but p_align is
> supposed to match it.

> You can't presume the actual memory addresses
> used will be so aligned (it's the maximum page size, not the minimum).

This sentence contradicts my deduction above.


> However, in a correct ELF object's phdrs, each p_vaddr and p_offset
> must be congruent to that alignment.
...
> you need to know the actual page size.  bfd_elf_bfd_from_remote_memory
> doesn't know this.  (The debugger might know it by some external means.

[-- Attachment #2: glibc-dl-load-aligned.patch --]
[-- Type: text/plain, Size: 4982 bytes --]

2007-08-15  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* elf/dl-load.c (_dl_map_object_from_fd): New variable ALIGNMAX.
	New sanity check if P_ALIGN is a power of two.
	New variables ALIGNEDSTART, ALIGNEDMAPPEDEND and ALIGNEDMAPLENGTH.
	Map ET_DYN ELFs with the P_ALIGN compliant ELF Base address.

--- glibc-20070810T2152-orig/elf/dl-load.c	2007-08-03 17:50:24.000000000 +0200
+++ glibc-20070810T2152/elf/dl-load.c	2007-08-15 00:41:03.000000000 +0200
@@ -1012,6 +1012,7 @@ _dl_map_object_from_fd (const char *name
 	int prot;
       } loadcmds[l->l_phnum], *c;
     size_t nloadcmds = 0;
+    ElfW(Addr) alignmax = GLRO(dl_pagesize);
     bool has_holes = false;
 
     /* The struct is initialized to zero so this is not necessary:
@@ -1036,6 +1037,12 @@ _dl_map_object_from_fd (const char *name
 	case PT_LOAD:
 	  /* A load command tells us to map in part of the file.
 	     We record the load commands and process them all later.  */
+	  if (__builtin_expect ((ph->p_align & (ph->p_align - 1)) != 0,
+				0))
+	    {
+	      errstring = N_("ELF load command alignment not a power of two");
+	      goto call_lose;
+	    }
 	  if (__builtin_expect ((ph->p_align & (GLRO(dl_pagesize) - 1)) != 0,
 				0))
 	    {
@@ -1049,6 +1056,8 @@ _dl_map_object_from_fd (const char *name
 		= N_("ELF load command address/offset not properly aligned");
 	      goto call_lose;
 	    }
+	  if (ph->p_align > alignmax)
+	    alignmax = ph->p_align;
 
 	  c = &loadcmds[nloadcmds++];
 	  c->mapstart = ph->p_vaddr & ~(GLRO(dl_pagesize) - 1);
@@ -1195,22 +1204,42 @@ cannot allocate TLS data structures for 
 	   prefer to map such objects at; but this is only a preference,
 	   the OS can do whatever it likes. */
 	ElfW(Addr) mappref;
+	ElfW(Addr) alignedstart, alignedmappedend;
+	ElfW(Addr) alignedmaplength = (maplength + GLRO(dl_pagesize) - 1)
+				      & -GLRO(dl_pagesize);
 	mappref = (ELF_PREFERRED_ADDRESS (loader, maplength,
 					  c->mapstart & GLRO(dl_use_load_bias))
 		   - MAP_BASE_ADDR (l));
 
 	/* Remember which part of the address space this object uses.  */
-	l->l_map_start = (ElfW(Addr)) __mmap ((void *) mappref, maplength,
+	l->l_map_start = (ElfW(Addr)) __mmap ((void *) mappref,
+					      alignedmaplength + alignmax
+							    - GLRO(dl_pagesize),
 					      c->prot,
-					      MAP_COPY|MAP_FILE,
-					      fd, c->mapoff);
+					      MAP_COPY|MAP_ANON,
+					      ANONFD, 0);
 	if (__builtin_expect ((void *) l->l_map_start == MAP_FAILED, 0))
 	  {
 	  map_error:
 	    errstring = N_("failed to map segment from shared object");
 	    goto call_lose_errno;
 	  }
-
+	/* Set the ELF `Base address' complying with all the P_ALIGNs.  */
+	alignedstart = (l->l_map_start + alignmax - 1) & -alignmax;
+	alignedmappedend = l->l_map_start + alignedmaplength + alignmax
+			   - GLRO(dl_pagesize);
+	if ((alignedstart != l->l_map_start
+	     && __munmap ((void *) l->l_map_start,
+			  alignedstart - l->l_map_start) != 0)
+	    || (alignedstart + alignedmaplength != alignedmappedend
+	        && __munmap ((void *) (alignedstart + alignedmaplength),
+			     alignedmappedend
+			     - (alignedstart + alignedmaplength)) != 0))
+	  {
+	    errstring = N_("failed to unmap excessive alignment memory");
+	    goto call_lose_errno;
+	  }
+	l->l_map_start = alignedstart;
 	l->l_map_end = l->l_map_start + maplength;
 	l->l_addr = l->l_map_start - c->mapstart;
 
@@ -1225,27 +1254,27 @@ cannot allocate TLS data structures for 
 		      PROT_NONE);
 
 	l->l_contiguous = 1;
-
-	goto postmap;
       }
-
-    /* This object is loaded at a fixed address.  This must never
-       happen for objects loaded with dlopen().  */
-    if (__builtin_expect ((mode & __RTLD_OPENEXEC) == 0, 0))
+    else
       {
-	errstring = N_("cannot dynamically load executable");
-	goto call_lose;
-      }
+	/* This object is loaded at a fixed address.  This must never
+	   happen for objects loaded with dlopen().  */
+	if (__builtin_expect ((mode & __RTLD_OPENEXEC) == 0, 0))
+	  {
+	    errstring = N_("cannot dynamically load executable");
+	    goto call_lose;
+	  }
 
-    /* Notify ELF_PREFERRED_ADDRESS that we have to load this one
-       fixed.  */
-    ELF_FIXED_ADDRESS (loader, c->mapstart);
+	/* Notify ELF_PREFERRED_ADDRESS that we have to load this one
+	   fixed.  */
+	ELF_FIXED_ADDRESS (loader, c->mapstart);
 
 
-    /* Remember which part of the address space this object uses.  */
-    l->l_map_start = c->mapstart + l->l_addr;
-    l->l_map_end = l->l_map_start + maplength;
-    l->l_contiguous = !has_holes;
+	/* Remember which part of the address space this object uses.  */
+	l->l_map_start = c->mapstart + l->l_addr;
+	l->l_map_end = l->l_map_start + maplength;
+	l->l_contiguous = !has_holes;
+      }
 
     while (c < &loadcmds[nloadcmds])
       {
@@ -1258,7 +1287,6 @@ cannot allocate TLS data structures for 
 		== MAP_FAILED))
 	  goto map_error;
 
-      postmap:
 	if (c->prot & PROT_EXEC)
 	  l->l_text_end = l->l_addr + c->mapend;
 

Re: loadbase alignment - ld.so/prelink/kernel or bfd_elf_bfd_from_remote_memory() bug?

[H.J. Lu]

On Wed, Aug 15, 2007 at 03:56:10PM +0200, Jan Kratochvil wrote:
> Hi,
> 
> bfd_elf_bfd_from_remote_memory() would almost work for general ELF files but it
> fails (I have no simple testcase) now due to the ELF `Base address' alignment.
> BFD uses `& -i_phdrs[i].p_align' for the VMA addresses but the real mapping
> is only PAGE_SIZE aligned (illustrated in the `BEFORE' dumps below).
> 
> For x86_64:
> 
> * bfd_elf_bfd_from_remote_memory() expects `loadbase' is P_ALIGN aligned.
> 
>   * GCC on x86_64 produces P_ALIGN 0x200000 == 2MB.
> 
> * ld.so loads ELF to `l_map_start' being only PAGE_SIZE aligned.
>   (The same problem applies to prelink and Linux kernel elf loading.)
> 
> * gELF standard in its `Base Address' computation expects the `base address' to
>   be `maximum page size' aligned, according to the last sentence:
>   	http://x86.ddj.com/ftp/manuals/tools/elf.pdf
>   	This address is truncated to the nearest multiple of the maximum page
>   	size.  The corresponding p_vaddr value itself is also truncated to the
>   	nearest multiple of the maximum page size. The base address is the
>   	difference between the truncated memory address and the truncated
>   	p_vaddr value.
> 
>   * x86_64 ELF standard in its `3.3.3 Page Size' talks about maximum page size
>     64KB (I would expect it should scale up to 2MB for the 2MB PSE pages).
>     	http://www.x86-64.org/documentation/abi.pdf
>     	Systems are permitted to use any power-of-two page size between 4KB and
>     	64KB, inclusive.
> 
> According to the Roland's mail below (p_align is supposed to match "maxpage")
> IMO ld.so + prelink + kernel violate the gELF standard.
> 
> Proper ld.so P_ALIGN-compliant mapping would also make possible a kernel 2MB
> PSE pages optimized mapping for very large .so libraries.
> 
> Removing P_ALIGN masking from bfd_elf_bfd_from_remote_memory() workarounds it
> well but I expect this problem should move to glibc + prelink + kernel, right?
> 
> 
> Proposing ld.so patch which fixes the bfd_elf_bfd_from_remote_memory() problem.
> 
> Program Headers:
>   Type           Offset             VirtAddr           PhysAddr
>                  FileSiz            MemSiz              Flags  Align
>   LOAD           0x0000000000000000 0x0000000000000000 0x0000000000000000
>                  0x00000000000004bc 0x00000000000004bc  R E    200000
>   LOAD           0x00000000000004c0 0x00000000002004c0 0x00000000002004c0
>                  0x00000000000001e8 0x00000000000001f8  RW     200000
> 
> BEFORE:
> 00400000-00401000 r-xp 00000000 08:01 4717502                            /tmp/alignmain
> 00600000-00601000 rw-p 00000000 08:01 4717502                            /tmp/alignmain
> ...
> 2aaaaaaad000-2aaaaaaae000 r-xp 00000000 08:01 4717503                    /tmp/alignlib.so
> 2aaaaaaae000-2aaaaacad000 ---p 00001000 08:01 4717503                    /tmp/alignlib.so
> 2aaaaacad000-2aaaaacae000 rw-p 00000000 08:01 4717503                    /tmp/alignlib.so
> 
> open("./alignlib.so", O_RDONLY)         = 3
> ...
> mmap(NULL, 2098872, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = 0x2aaaaaaad000
> mprotect(0x2aaaaaaae000, 2093056, PROT_NONE) = 0
> mmap(0x2aaaaacad000, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0) = 0x2aaaaacad000
> close(3)                                = 0
> 
> 
> AFTER:
> 00400000-00401000 r-xp 00000000 08:01 4717502                            /tmp/alignmain
> 00600000-00601000 rw-p 00000000 08:01 4717502                            /tmp/alignmain
> ...
> 2aaaaac00000-2aaaaac01000 r-xp 00000000 08:01 4717503                    /tmp/alignlib.so
> 2aaaaac01000-2aaaaae00000 ---p 2aaaaac01000 00:00 0 
> 2aaaaae00000-2aaaaae01000 rw-p 00000000 08:01 4717503                    /tmp/alignlib.so
> 
> open("./alignlib.so", O_RDONLY)         = 3
> ...
> mmap(NULL, 4194304, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_ANONYMOUS|MAP_DENYWRITE, -1, 0) = 0x2aaaaaaad000
> munmap(0x2aaaaaaad000, 1388544)         = 0
> munmap(0x2aaaaae01000, 704512)          = 0
> mprotect(0x2aaaaac01000, 2093056, PROT_NONE) = 0
> mmap(0x2aaaaac00000, 4096, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0) = 0x2aaaaac00000
> mmap(0x2aaaaae00000, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0) = 0x2aaaaae00000
> close(3)                                = 0
> 
> 
> 
> Regards,
> Jan
> 
> 
> http://sourceware.org/ml/binutils/2007-08/msg00184.html
> 
> On Mon, 13 Aug 2007 03:15:44 +0200, Roland McGrath wrote:
> ...
> > The file itself doesn't tell you what "maxpage" is, but p_align is
> > supposed to match it.
> 
> > You can't presume the actual memory addresses
> > used will be so aligned (it's the maximum page size, not the minimum).
> 
> This sentence contradicts my deduction above.
> 
> 
> > However, in a correct ELF object's phdrs, each p_vaddr and p_offset
> > must be congruent to that alignment.
> ...
> > you need to know the actual page size.  bfd_elf_bfd_from_remote_memory
> > doesn't know this.  (The debugger might know it by some external means.

> 2007-08-15  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* elf/dl-load.c (_dl_map_object_from_fd): New variable ALIGNMAX.
> 	New sanity check if P_ALIGN is a power of two.
> 	New variables ALIGNEDSTART, ALIGNEDMAPPEDEND and ALIGNEDMAPLENGTH.
> 	Map ET_DYN ELFs with the P_ALIGN compliant ELF Base address.
> 
> --- glibc-20070810T2152-orig/elf/dl-load.c	2007-08-03 17:50:24.000000000 +0200
> +++ glibc-20070810T2152/elf/dl-load.c	2007-08-15 00:41:03.000000000 +0200
> @@ -1012,6 +1012,7 @@ _dl_map_object_from_fd (const char *name
>  	int prot;
>        } loadcmds[l->l_phnum], *c;
>      size_t nloadcmds = 0;
> +    ElfW(Addr) alignmax = GLRO(dl_pagesize);

Why not replace all GLRO(dl_pagesize) with alignmax?


H.J.

Re: loadbase alignment - ld.so/prelink/kernel or bfd_elf_bfd_from_remote_memory() bug?

[Jan Kratochvil]

On Wed, 15 Aug 2007 16:03:02 +0200, H.J. Lu wrote:
> On Wed, Aug 15, 2007 at 03:56:10PM +0200, Jan Kratochvil wrote:
...
> > --- glibc-20070810T2152-orig/elf/dl-load.c	2007-08-03 17:50:24.000000000 +0200
> > +++ glibc-20070810T2152/elf/dl-load.c	2007-08-15 00:41:03.000000000 +0200
> > @@ -1012,6 +1012,7 @@ _dl_map_object_from_fd (const char *name
> >  	int prot;
> >        } loadcmds[l->l_phnum], *c;
> >      size_t nloadcmds = 0;
> > +    ElfW(Addr) alignmax = GLRO(dl_pagesize);
> 
> Why not replace all GLRO(dl_pagesize) with alignmax?

The final ELF mapping start address is decided by the call:
	l->l_map_start = (ElfW(Addr)) __mmap ((void *) mappref, maplength,
					      c->prot,
					      MAP_COPY|MAP_FILE,
					      fd, c->mapoff);

Even if you have MAPPREF properly aligned kernel still may choose any address
>= MAPPREF being only PAGE_SIZE aligned.  There should be a kernel mmap()
syscall alignment parameter which does not exist, therefore it must be emulated
by hand as my patch does.

The current use of `GLRO(dl_pagesize)' inside `case PT_LOAD' has meaning only
for ET_EXEC files as the address gets recalculated for ET_DYN files later.
ET_EXEC files base address cannot be changed so this patch should not change
their behavior.

Sorry if I did not understand your suggestion.


Regards,
Jan

Re: loadbase alignment - ld.so/prelink/kernel or bfd_elf_bfd_from_remote_memory() bug?

[H.J. Lu]

On Wed, Aug 15, 2007 at 04:40:55PM +0200, Jan Kratochvil wrote:
> On Wed, 15 Aug 2007 16:03:02 +0200, H.J. Lu wrote:
> > On Wed, Aug 15, 2007 at 03:56:10PM +0200, Jan Kratochvil wrote:
> ...
> > > --- glibc-20070810T2152-orig/elf/dl-load.c	2007-08-03 17:50:24.000000000 +0200
> > > +++ glibc-20070810T2152/elf/dl-load.c	2007-08-15 00:41:03.000000000 +0200
> > > @@ -1012,6 +1012,7 @@ _dl_map_object_from_fd (const char *name
> > >  	int prot;
> > >        } loadcmds[l->l_phnum], *c;
> > >      size_t nloadcmds = 0;
> > > +    ElfW(Addr) alignmax = GLRO(dl_pagesize);
> > 
> > Why not replace all GLRO(dl_pagesize) with alignmax?
> 
> Sorry if I did not understand your suggestion.
> 

You introduced alignmax for GLRO(dl_pagesize). But you still use
GLRO(dl_pagesize) in some places instead of alignmax.


H.J.

Re: loadbase alignment - ld.so/prelink/kernel or bfd_elf_bfd_from_remote_memory() bug?

[H.J. Lu]

On Wed, Aug 15, 2007 at 07:45:04AM -0700, H.J. Lu wrote:
> On Wed, Aug 15, 2007 at 04:40:55PM +0200, Jan Kratochvil wrote:
> > On Wed, 15 Aug 2007 16:03:02 +0200, H.J. Lu wrote:
> > > On Wed, Aug 15, 2007 at 03:56:10PM +0200, Jan Kratochvil wrote:
> > ...
> > > > --- glibc-20070810T2152-orig/elf/dl-load.c	2007-08-03 17:50:24.000000000 +0200
> > > > +++ glibc-20070810T2152/elf/dl-load.c	2007-08-15 00:41:03.000000000 +0200
> > > > @@ -1012,6 +1012,7 @@ _dl_map_object_from_fd (const char *name
> > > >  	int prot;
> > > >        } loadcmds[l->l_phnum], *c;
> > > >      size_t nloadcmds = 0;
> > > > +    ElfW(Addr) alignmax = GLRO(dl_pagesize);
> > > 
> > > Why not replace all GLRO(dl_pagesize) with alignmax?
> > 
> > Sorry if I did not understand your suggestion.
> > 
> 
> You introduced alignmax for GLRO(dl_pagesize). But you still use
> GLRO(dl_pagesize) in some places instead of alignmax.

Never mind.


H.J.

Re: loadbase alignment - ld.so/prelink/kernel or bfd_elf_bfd_from_remote_memory() bug?

[Jakub Jelinek]

On Wed, Aug 15, 2007 at 03:56:10PM +0200, Jan Kratochvil wrote:
> bfd_elf_bfd_from_remote_memory() would almost work for general ELF files but it
> fails (I have no simple testcase) now due to the ELF `Base address' alignment.
> BFD uses `& -i_phdrs[i].p_align' for the VMA addresses but the real mapping
> is only PAGE_SIZE aligned (illustrated in the `BEFORE' dumps below).
> 
> For x86_64:
> 
> * bfd_elf_bfd_from_remote_memory() expects `loadbase' is P_ALIGN aligned.
> 
>   * GCC on x86_64 produces P_ALIGN 0x200000 == 2MB.

If bfd_elf_bfd_from_remote_memory() doesn't handle it, then it should be
fixed.  The current ld.so/prelink/kernel behavior is desirable.

	Jakub

Re: loadbase alignment - ld.so/prelink/kernel or bfd_elf_bfd_from_remote_memory() bug?

[H.J. Lu]

On Wed, Aug 15, 2007 at 05:13:48PM +0200, Jakub Jelinek wrote:
> On Wed, Aug 15, 2007 at 03:56:10PM +0200, Jan Kratochvil wrote:
> > bfd_elf_bfd_from_remote_memory() would almost work for general ELF files but it
> > fails (I have no simple testcase) now due to the ELF `Base address' alignment.
> > BFD uses `& -i_phdrs[i].p_align' for the VMA addresses but the real mapping
> > is only PAGE_SIZE aligned (illustrated in the `BEFORE' dumps below).
> > 
> > For x86_64:
> > 
> > * bfd_elf_bfd_from_remote_memory() expects `loadbase' is P_ALIGN aligned.
> > 
> >   * GCC on x86_64 produces P_ALIGN 0x200000 == 2MB.
> 
> If bfd_elf_bfd_from_remote_memory() doesn't handle it, then it should be
> fixed.  The current ld.so/prelink/kernel behavior is desirable.
> 

I can see both approaches make senses. On one hand, BFD doesn't
know what the run-time page size is. BFD has to use a constant like
the maximum page size. On the other hand, we may not want to waste
address space at run-time. But it does violate gABI. We hadn't
run into any problems up to now since only ld.so cares about base
address so far. It won't work when BFD also needs base address. If we
want to keep the current ld.so behavior and also support base address
in BFD, I suggest we should

1. Modify the gABI to use the maximum run-time page size when
computing base address, instead of the maximum page size.
2. Modify BFD to take the maximum run-time page size, which can
be set at run-time, and use it to compute base address.


H.J.

[patch] bfd_elf_bfd_from_remote_memory() workaround for the ELF misalignment [Re: loadbase alignment - ld.so/prelink/kernel or bfd_elf_bfd_from_remote_memory() bug?]

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 1946 bytes --]

On Wed, 15 Aug 2007 18:00:43 +0200, H.J. Lu wrote:
> On Wed, Aug 15, 2007 at 05:13:48PM +0200, Jakub Jelinek wrote:
> > On Wed, Aug 15, 2007 at 03:56:10PM +0200, Jan Kratochvil wrote:
...
> > > BFD uses `& -i_phdrs[i].p_align' for the VMA addresses but the real mapping
> > > is only PAGE_SIZE aligned (illustrated in the `BEFORE' dumps below).
> > > 
> > > For x86_64:
> > > 
> > > * bfd_elf_bfd_from_remote_memory() expects `loadbase' is P_ALIGN aligned.
> > > 
> > >   * GCC on x86_64 produces P_ALIGN 0x200000 == 2MB.
> > 
> > If bfd_elf_bfd_from_remote_memory() doesn't handle it, then it should be
> > fixed.  The current ld.so/prelink/kernel behavior is desirable.
> 
> I can see both approaches make senses. On one hand, BFD doesn't
> know what the run-time page size is.
...
> 2. Modify BFD to take the maximum run-time page size, which can
> be set at run-time, and use it to compute base address.

You were suggesting a new parameter for bfd_elf_bfd_from_remote_memory()?
Attached a patch using TEMPL's MINPAGESIZE value instead, isn't that sufficient?

It may have a regression due to a smaller alignment on these arches:
$ egrep -l 'ELF_MAXPAGESIZE.*\<(0x)?1\>' *.c
elf32-avr.c elf32-cr16.c elf32-cr16c.c elf32-crx.c elf32-dlx.c elf32-gen.c
elf32-h8300.c elf32-i960.c elf32-ip2k.c elf32-m32r.c elf32-m88k.c
elf32-msp430.c elf32-mt.c elf32-xtensa.c elf64-gen.c

Is there a chance these arches would use bfd_elf_bfd_from_remote_memory()?
They should fix their MINPAGESIZE, in many cases there is even a FIXME note.


Tried to remove the P_ALIGN use there completely - besides missing PHDRs read
(EHDR is already read there) there should be no alignment requirements.
Unfortunately IA64 Linux kernel vDSO provides debug symbols after the last
segment ends but still in the same page - no symbols without the aligning:
contents_size  == 2512
i_ehdr.e_shoff == 2640
ia64 minpagesize == 16384
i_phdrs[i].p_align == 0x10000



Regards,
Jan

[-- Attachment #2: bfd-frommemory-unaligned.patch --]
[-- Type: text/plain, Size: 2206 bytes --]

2007-08-20  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* elfcode.h (NAME(_bfd_elf,bfd_from_remote_memory)): New variable
	PAGESIZE.  Replaced P_ALIGN by backend's MINPAGESIZE.  Fixed LOADBASE
	computation for segments without P_ALIGN alignment.

--- bfd/elfcode.h	14 Aug 2007 08:04:47 -0000	1.86
+++ bfd/elfcode.h	17 Aug 2007 18:09:00 -0000
@@ -1636,6 +1636,7 @@ NAME(_bfd_elf,bfd_from_remote_memory)
   unsigned int i;
   bfd_vma loadbase;
   bfd_boolean loadbase_set;
+  bfd_vma pagesize = get_elf_backend_data (templ)->minpagesize;
 
   /* Read in the ELF header in external format.  */
   err = target_read_memory (ehdr_vma, (bfd_byte *) &x_ehdr, sizeof x_ehdr);
@@ -1720,7 +1721,7 @@ NAME(_bfd_elf,bfd_from_remote_memory)
 	{
 	  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;
+			 + pagesize - 1) & -pagesize;
 	  if (segment_end > (bfd_vma) contents_size)
 	    contents_size = segment_end;
 
@@ -1729,7 +1730,9 @@ NAME(_bfd_elf,bfd_from_remote_memory)
 	     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)
 	    {
-	      loadbase = ehdr_vma - (i_phdrs[i].p_vaddr & -i_phdrs[i].p_align);
+	      /* LOADBASE may not be P_ALIGN aligned.  It violates the gELF
+	         standard but it is a common practice.  */
+	      loadbase = ehdr_vma - (i_phdrs[i].p_vaddr - i_phdrs[i].p_offset);
 	      loadbase_set = TRUE;
 	    }
 
@@ -1771,13 +1775,12 @@ NAME(_bfd_elf,bfd_from_remote_memory)
   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 start = i_phdrs[i].p_offset & -pagesize;
 	bfd_vma end = (i_phdrs[i].p_offset + i_phdrs[i].p_filesz
-		       + i_phdrs[i].p_align - 1) & -i_phdrs[i].p_align;
+		       + pagesize - 1) & -pagesize;
 	if (end > (bfd_vma) contents_size)
 	  end = contents_size;
-	err = target_read_memory ((loadbase + i_phdrs[i].p_vaddr)
-				  & -i_phdrs[i].p_align,
+	err = target_read_memory ((loadbase + i_phdrs[i].p_vaddr) & -pagesize,
 				  contents + start, end - start);
 	if (err)
 	  {

Re: [patch] bfd_elf_bfd_from_remote_memory() workaround for the ELF misalignment [Re: loadbase alignment - ld.so/prelink/kernel or bfd_elf_bfd_from_remote_memory() bug?]

[Roland McGrath]

> You were suggesting a new parameter for bfd_elf_bfd_from_remote_memory()?
> Attached a patch using TEMPL's MINPAGESIZE value instead, isn't that sufficient?

I don't know what the BFD minpagesize value means exactly.

It may well matter to use the actual page size.  

When dealing with a core file, the de facto standard is that the p_align of
the core file's PT_LOAD phdrs gives the real page size.  (This is in phdrs
of the core file itself, not to be confused with the phdrs inside the
embedded ELF image of interest.)  Note however that IIUC gdb's gcore does
not produce correct p_align values in its core files (it uses 1).  If the
core PT_LOAD segment you are looking at has a p_align that is unreasonably
small, you could instead look for an NT_AUXV note and AT_PAGESZ inside it.

For a live process, the debugger might already have determined this.
One thing it can do is look in /proc/pid/auxv for AT_PAGESZ.

> Tried to remove the P_ALIGN use there completely - besides missing PHDRs read
> (EHDR is already read there) there should be no alignment requirements.
> Unfortunately IA64 Linux kernel vDSO provides debug symbols after the last
> segment ends but still in the same page - no symbols without the aligning:
> contents_size  == 2512
> i_ehdr.e_shoff == 2640
> ia64 minpagesize == 16384
> i_phdrs[i].p_align == 0x10000

This is the practical example today.  But it demonstrates the worth of
having the function follow the general principle I always intended for it.
That is, it should try to recover exactly everything that came from the ELF
file and is visible in the process memory.  This means rounding segments to
include all the whole pages that include the [p_vaddr, p_vaddr+p_size) range.


Thanks,
Roland

Re: [patch] bfd_elf_bfd_from_remote_memory() workaround for the ELF misalignment [Re: loadbase alignment - ld.so/prelink/kernel or bfd_elf_bfd_from_remote_memory() bug?]

[H.J. Lu]

On Mon, Aug 20, 2007 at 09:54:28PM +0200, Jan Kratochvil wrote:
> On Wed, 15 Aug 2007 18:00:43 +0200, H.J. Lu wrote:
> > On Wed, Aug 15, 2007 at 05:13:48PM +0200, Jakub Jelinek wrote:
> > > On Wed, Aug 15, 2007 at 03:56:10PM +0200, Jan Kratochvil wrote:
> ...
> > > > BFD uses `& -i_phdrs[i].p_align' for the VMA addresses but the real mapping
> > > > is only PAGE_SIZE aligned (illustrated in the `BEFORE' dumps below).
> > > > 
> > > > For x86_64:
> > > > 
> > > > * bfd_elf_bfd_from_remote_memory() expects `loadbase' is P_ALIGN aligned.
> > > > 
> > > >   * GCC on x86_64 produces P_ALIGN 0x200000 == 2MB.
> > > 
> > > If bfd_elf_bfd_from_remote_memory() doesn't handle it, then it should be
> > > fixed.  The current ld.so/prelink/kernel behavior is desirable.
> > 
> > I can see both approaches make senses. On one hand, BFD doesn't
> > know what the run-time page size is.
> ...
> > 2. Modify BFD to take the maximum run-time page size, which can
> > be set at run-time, and use it to compute base address.
> 
> You were suggesting a new parameter for bfd_elf_bfd_from_remote_memory()?
> Attached a patch using TEMPL's MINPAGESIZE value instead, isn't that sufficient?
> 

No. The new parameter should be the maximum page size at run-time,
which is >= MINPAGESIZE and may be default to MINPAGESIZE.


H.J.

Re: [patch] bfd_elf_bfd_from_remote_memory() workaround for the ELF misalignment [Re: loadbase alignment - ld.so/prelink/kernel or bfd_elf_bfd_from_remote_memory() bug?]

[Daniel Jacobowitz]

On Tue, Aug 21, 2007 at 06:50:12AM -0700, H.J. Lu wrote:
> No. The new parameter should be the maximum page size at run-time,
> which is >= MINPAGESIZE and may be default to MINPAGESIZE.

What practical consequence does it have to use a too small value here?
The segment's size should be large enough to cover all the memory it
uses - in deliberately malicious cases that may not be true but I
don't think it's a problem in this context since more values in the
header could be wrong than just this one.

So if you align to the architecturally minimum page size you should
find the whole library.

But then, I'm not even clear on why we need to honor p_align.  We're
trying to create an _ELF file_ from memory, not a loaded image.  We
only need the bits that would have been in the file!

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] bfd_elf_bfd_from_remote_memory() workaround for the ELF misalignment [Re: loadbase alignment - ld.so/prelink/kernel or bfd_elf_bfd_from_remote_memory() bug?]

[Roland McGrath]

> What practical consequence does it have to use a too small value here?
[...]
> We're trying to create an _ELF file_ from memory, not a loaded image.
> We only need the bits that would have been in the file!

That's precisely the issue.  The actual page size used by the process in
question is what tells you the true facts about how much of the original
ELF file is in the memory that you can read now.

The page alignment probably doesn't come up in practice for getting the
beginning of the file, because even without it you are usually sure from
phdr arithmetic what vaddr corresponds to file offset 0.  It is relevant
in practice to finding the end of the file.  

The only case where this function is used today is the Linux vDSO.  That
is a DSO with one segment, or two segments that are both read-only and
map the same part of the file (IA64).  The part of the file after the
p_vaddr+p_filesz (=p_memsz) of the last segment will be in the memory
image, up to the next page boundary.  This is where the section headers
and .shstrtab reside (or other nonallocated sections for an unstripped
file, but this is not the case with the vDSO).  Of course, the zeros seen
in memory for the tail of the page past the end of the file are not part
of the file, so it misrepresents the original file to just always include
the whole last page.  Hence the function has the special heuristic to
truncate to the shorter of the file portion available in memory and the
offset at the end of the section headers as indicated by the ELF header.

It is pleasant to have the section headers and names when they are
available.  At least at some point in the past, I think it may have been
required for gdb to find everything it should, because it didn't find
everything from PT_GNU_EH_FRAME, PT_DYNAMIC, DT_*, etc.  (I don't know if
that is an issue with today's code.)

It so happens that in all the Linux vDSO images today, there is enough of
the partial page after the segments to include all the section headers.
(In point of fact, the way Linux loads the vDSO is just a flat mapping of
the whole file, not driven by its phdrs.  So, for example, as things
stand if the ppc/ppc64 vDSO grew so its segments exactly filled out n
pages, an n+1'th page of .shstrtab and section headers would be there in
memory.  But there is no way to determine this from a dump/process, short
of assuming it as a special case for the vDSO that the maximum size the
ELF header suggests will in fact be there.)

This is the only case used by gdb today.  There is another case relevant
to my thinking, that I do support in the equivalent code in the elfutils
libraries.  A normal DSO usually has a data segment with some bss,
i.e. p_memsz > p_filesz.  In that case, the end of the file is never
available in memory.  The part of the file you can get intact (or close)
from memory is truncated at the p_filesz of the data segment.  But, you
can usually see the whole text segment, which includes .eh_frame and for
DSOs includes the dynamic symbol table.  So, it really is worthwhile to
get as much as you can get, and you do need to know the page size to
reliably know what you can get.  (This is probably only of interest when
dealing with core dumps when using the option to dump all segments in
their entirety rather than suppressing text segments as usual.)


Thanks,
Roland

Re: [patch] bfd_elf_bfd_from_remote_memory() workaround for the ELF misalignment [Re: loadbase alignment - ld.so/prelink/kernel or bfd_elf_bfd_from_remote_memory() bug?]

[Daniel Jacobowitz]

On Tue, Aug 21, 2007 at 01:43:53PM -0700, Roland McGrath wrote:
> The only case where this function is used today is the Linux vDSO.  That
> is a DSO with one segment, or two segments that are both read-only and
> map the same part of the file (IA64).  The part of the file after the
> p_vaddr+p_filesz (=p_memsz) of the last segment will be in the memory
> image, up to the next page boundary.  This is where the section headers
> and .shstrtab reside (or other nonallocated sections for an unstripped
> file, but this is not the case with the vDSO).  Of course, the zeros seen
> in memory for the tail of the page past the end of the file are not part
> of the file, so it misrepresents the original file to just always include
> the whole last page.  Hence the function has the special heuristic to
> truncate to the shorter of the file portion available in memory and the
> offset at the end of the section headers as indicated by the ELF header.

So what we are doing here is guessing from the program headers where
to find other bits that would have been in the disk file, but are not
part of the ELF object's memory image.  Then we try to decide whether
what we have in memory is likely to include them or not.  Is that
about right?

> It is pleasant to have the section headers and names when they are
> available.  At least at some point in the past, I think it may have been
> required for gdb to find everything it should, because it didn't find
> everything from PT_GNU_EH_FRAME, PT_DYNAMIC, DT_*, etc.  (I don't know if
> that is an issue with today's code.)

If it is still an issue, IMHO we should fix that instead of attempting
to recover the section headers.  The corner cases are too cornered.

> This is the only case used by gdb today.  There is another case relevant
> to my thinking, that I do support in the equivalent code in the elfutils
> libraries.  A normal DSO usually has a data segment with some bss,
> i.e. p_memsz > p_filesz.  In that case, the end of the file is never
> available in memory.  The part of the file you can get intact (or close)
> from memory is truncated at the p_filesz of the data segment.  But, you
> can usually see the whole text segment, which includes .eh_frame and for
> DSOs includes the dynamic symbol table.  So, it really is worthwhile to
> get as much as you can get, and you do need to know the page size to
> reliably know what you can get.  (This is probably only of interest when
> dealing with core dumps when using the option to dump all segments in
> their entirety rather than suppressing text segments as usual.)

What I'm missing - I'm sure it's there somewhere - is how you got from
"can usually see the whole text segment, including [some loaded
sections]" to "need to know the page size".  .eh_frame will be found
via the program headers, .dynsym will be indicated by .dynamic, and
all three are included in both p_filesz and p_memsz.  I don't see why
we need any heurestics for this case.

I think we can effectively ignore p_align.  We know the actual load
address of the program headers.  We can rely on at least one PT_LOAD
segment including the file offset of the program headers - this is not
the case on some targets but we won't be able to do what we're trying
in that case anyway.  So we know the file's link time virtual address
for the program headers.  The difference is the base address, no
truncation at all.  Obviously incomplete patch:

Index: elfcode.h
===================================================================
RCS file: /cvs/src/src/bfd/elfcode.h,v
retrieving revision 1.86
diff -u -p -r1.86 elfcode.h
--- elfcode.h	14 Aug 2007 08:04:47 -0000	1.86
+++ elfcode.h	21 Aug 2007 21:27:16 -0000
@@ -1627,7 +1627,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, *phdrs_phdr;
   bfd *nbfd;
   struct bfd_in_memory *bim;
   int contents_size;
@@ -1635,7 +1635,6 @@ NAME(_bfd_elf,bfd_from_remote_memory)
   int err;
   unsigned int i;
   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);
@@ -1711,8 +1710,8 @@ NAME(_bfd_elf,bfd_from_remote_memory)
 
   contents_size = 0;
   last_phdr = NULL;
+  phdrs_phdr = NULL;
   loadbase = ehdr_vma;
-  loadbase_set = FALSE;
   for (i = 0; i < i_ehdr.e_phnum; ++i)
     {
       elf_swap_phdr_in (templ, &x_phdrs[i], &i_phdrs[i]);
@@ -1724,14 +1723,11 @@ NAME(_bfd_elf,bfd_from_remote_memory)
 	  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)
-	    {
-	      loadbase = ehdr_vma - (i_phdrs[i].p_vaddr & -i_phdrs[i].p_align);
-	      loadbase_set = TRUE;
-	    }
+	  if (i_phdrs[i].p_offset < i_ehdr.e_phoff
+	      && i_phdrs[i].p_offset + i_phdrs[i].p_filesz > i_ehdr.e_phoff
+	      && (phdrs_phdr == NULL
+		  || i_phdrs[i].p_vaddr < phdrs_phdr->p_vaddr))
+	    phdrs_phdr = &i_phdrs[i];
 
 	  last_phdr = &i_phdrs[i];
 	}
@@ -1744,6 +1740,13 @@ NAME(_bfd_elf,bfd_from_remote_memory)
       return NULL;
     }
 
+  /* LOADADDR is the `Base address' from the gELF specification.  */
+  if (phdrs_phdr != NULL)
+    {
+      loadbase = ehdr_vma + i_ehdr.e_phoff;
+      loadbase -= phdrs_phdr->p_vaddr - phdrs_phdr->p_offset + i_ehdr.e_phoff;
+    }
+
   /* 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.  */


-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] bfd_elf_bfd_from_remote_memory() workaround for the ELF misalignment

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 1262 bytes --]

On Tue, 21 Aug 2007 23:27:35 +0200, Daniel Jacobowitz wrote:
...
> > It is pleasant to have the section headers and names when they are
> > available.  At least at some point in the past, I think it may have been
> > required for gdb to find everything it should, because it didn't find
> > everything from PT_GNU_EH_FRAME, PT_DYNAMIC, DT_*, etc.  (I don't know if
> > that is an issue with today's code.)
> 
> If it is still an issue, IMHO we should fix that instead of attempting
> to recover the section headers.  The corner cases are too cornered.

Attaching the the patch I had prepared before.

You are tight it would work if GDB (IMO BFD) has to be fixed to parse PHDRs
instead.  Attaching IA64 VDSO + its variant as it gets cut+cleared by
bfd_elf_bfd_from_remote_memory().

...
> What I'm missing - I'm sure it's there somewhere - is how you got from
> "can usually see the whole text segment, including [some loaded
> sections]" to "need to know the page size".  .eh_frame will be found
> via the program headers, .dynsym will be indicated by .dynamic, and
> all three are included in both p_filesz and p_memsz.  I don't see why
> we need any heurestics for this case.

The section headers table itself is behind the last loaded segment.


Regards,
Jan

[-- Attachment #2: bfd-remotememory-unaligned.patch --]
[-- Type: text/plain, Size: 3007 bytes --]

2007-08-20  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* elfcode.h (NAME(_bfd_elf,bfd_from_remote_memory)): Removed all the
	P_ALIGN alignments.  New P_ALIGN validity check.  Fixed LOADBASE
	computation for segments without P_ALIGN alignment.  Explicitly read
	PHDRs into the returned BUFFER.

--- bfd/elfcode.h	14 Aug 2007 08:04:47 -0000	1.86
+++ bfd/elfcode.h	20 Aug 2007 16:56:49 -0000
@@ -1709,7 +1709,8 @@ NAME(_bfd_elf,bfd_from_remote_memory)
     }
   i_phdrs = (Elf_Internal_Phdr *) &x_phdrs[i_ehdr.e_phnum];
 
-  contents_size = 0;
+  /* Also cover `sizeof x_ehdr'.  */
+  contents_size = i_ehdr.e_phoff + i_ehdr.e_phnum * sizeof x_phdrs[0];
   last_phdr = NULL;
   loadbase = ehdr_vma;
   loadbase_set = FALSE;
@@ -1719,17 +1720,27 @@ NAME(_bfd_elf,bfd_from_remote_memory)
       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;
+	  segment_end = i_phdrs[i].p_offset + i_phdrs[i].p_filesz;
 	  if (segment_end > (bfd_vma) contents_size)
 	    contents_size = segment_end;
 
+	  /* ELF file sanity check for valid ELF-compliant P_ALIGN.  */
+	  if ((i_phdrs[i].p_vaddr - i_phdrs[i].p_offset) % i_phdrs[i].p_align
+	      != 0)
+	    {
+	      free (x_phdrs);
+	      bfd_set_error (bfd_error_wrong_format);
+	      return NULL;
+	    }
+
 	  /* 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)
 	    {
-	      loadbase = ehdr_vma - (i_phdrs[i].p_vaddr & -i_phdrs[i].p_align);
+	      /* LOADBASE may not be P_ALIGN aligned.  It violates the gELF
+		 standard but it is a common practice.  */
+	      loadbase = ehdr_vma - (i_phdrs[i].p_vaddr - i_phdrs[i].p_offset);
 	      loadbase_set = TRUE;
 	    }
 
@@ -1771,13 +1782,11 @@ NAME(_bfd_elf,bfd_from_remote_memory)
   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;
+	bfd_vma start = i_phdrs[i].p_offset;
+	bfd_vma end = i_phdrs[i].p_offset + i_phdrs[i].p_filesz;
 	if (end > (bfd_vma) contents_size)
 	  end = contents_size;
-	err = target_read_memory ((loadbase + i_phdrs[i].p_vaddr)
-				  & -i_phdrs[i].p_align,
+	err = target_read_memory (loadbase + i_phdrs[i].p_vaddr,
 				  contents + start, end - start);
 	if (err)
 	  {
@@ -1788,6 +1797,9 @@ NAME(_bfd_elf,bfd_from_remote_memory)
 	    return NULL;
 	  }
       }
+  /* This will normally have been in the first PT_LOAD segment.  */
+  memcpy (contents + i_ehdr.e_phoff, x_phdrs,
+	  i_ehdr.e_phnum * sizeof x_phdrs[0]);
   free (x_phdrs);
 
   /* If the segments visible in memory didn't include the section headers,

[-- Attachment #3: vdso-ia64-orig.so.gz --]
[-- Type: application/x-gzip, Size: 1544 bytes --]

[-- Attachment #4: vdso-ia64-by-bfd_elf_bfd_from_remote_memory.so.gz --]
[-- Type: application/x-gzip, Size: 1119 bytes --]

Re: [patch] bfd_elf_bfd_from_remote_memory() workaround for the ELF misalignment [Re: loadbase alignment - ld.so/prelink/kernel or bfd_elf_bfd_from_remote_memory() bug?]

[Roland McGrath]

> So what we are doing here is guessing from the program headers where
> to find other bits that would have been in the disk file, but are not
> part of the ELF object's memory image.  Then we try to decide whether
> what we have in memory is likely to include them or not.  Is that
> about right?

Exactly so.

> If it is still an issue, IMHO we should fix that instead of attempting
> to recover the section headers.  The corner cases are too cornered.

I agree that gdb should be fixed to do the best possible with no sections.
My tendencies lead me to want the function in question always to do the
best it can on the terms it describes for itself, regardless.  That is
especially attractive given that in the only real-world uses right now, it
can in fact win.

> What I'm missing - I'm sure it's there somewhere - is how you got from
> "can usually see the whole text segment, including [some loaded
> sections]" to "need to know the page size".  .eh_frame will be found
> via the program headers, .dynsym will be indicated by .dynamic, and
> all three are included in both p_filesz and p_memsz.  I don't see why
> we need any heurestics for this case.

No, you're right to miss it because it's not really there.  I just leapt to
"want as much as you can get", and from there the page size is inevitable.
But for all those uses, p_filesz is plenty.  It's just those tendencies of mine.

Honestly, I don't care what BFD does.  
I was just airing everything I could think of on the subject.


Thanks,
Roland

Re: [patch] bfd_elf_bfd_from_remote_memory() workaround for the ELF misalignment

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 1254 bytes --]

On Wed, 22 Aug 2007 00:22:36 +0200, Jan Kratochvil wrote:
> On Tue, 21 Aug 2007 23:27:35 +0200, Daniel Jacobowitz wrote:
> > On Tue, 21 Aug 2007 22:43:53 +0200, Roland McGrath wrote:
> > > At least at some point in the past, I think it may have been
> > > required for gdb to find everything it should, because it didn't find
> > > everything from PT_GNU_EH_FRAME, PT_DYNAMIC, DT_*, etc.  (I don't know if
> > > that is an issue with today's code.)
> > 
> > If it is still an issue, IMHO we should fix that instead of attempting
> > to recover the section headers.  The corner cases are too cornered.
...
> You are tight it would work if GDB (IMO BFD) has to be fixed to parse PHDRs
> instead.  Attaching IA64 VDSO + its variant as it gets cut+cleared by
> bfd_elf_bfd_from_remote_memory().

Patch attached.  It cannot be placed only to GDB as GDB uses the symbols
parsing code from BFD and if BFD cannot find those symbols...  GDB could also
patch the ELF_OBJ_TDATA structure itself before calling BFD symbols retrieval.

It works on IA64 together with aligning-less bfd_elf_bfd_from_remote_memory():
	http://sourceware.org/ml/binutils/2007-08/msg00321.html

Not sure if it is worth it as the current code works for any real-world cases.


Regards,
Jan

[-- Attachment #2: bfd-exec-load-phdrs.patch --]
[-- Type: text/plain, Size: 12288 bytes --]

2007-08-24  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* Makefile.am (elf32.lo, elf64.lo): Updated dependencies.
	* Makefile.in: Likewise.
	* elf-bfd.h (struct elf_obj_tdata): New field CODE_SEGMENT_SECTION.
	* elf.c (bfd_elf_get_elf_syms): Optionally patch ST_SHNDX according to
	CODE_SEGMENT_SECTION.
	* elfcode.h: Include "safe-ctype.h".
	(find_code_segment): New function.
	(elf_object_p): Create sections from PHDRs if no SHDRs were found.
	If no dynamic symbols were found create them from PT_DYNAMIC pointers.

--- bfd/Makefile.am	23 Aug 2007 16:29:49 -0000	1.204
+++ bfd/Makefile.am	24 Aug 2007 08:50:29 -0000
@@ -1559,7 +1559,8 @@ elf32-xc16x.lo: elf32-xc16x.c $(INCDIR)/
   $(INCDIR)/elf/dwarf2.h $(INCDIR)/libiberty.h elf32-target.h
 elf32.lo: elf32.c elfcode.h $(INCDIR)/filenames.h $(INCDIR)/libiberty.h \
   $(INCDIR)/bfdlink.h $(INCDIR)/hashtab.h elf-bfd.h $(INCDIR)/elf/common.h \
-  $(INCDIR)/elf/internal.h $(INCDIR)/elf/external.h elfcore.h
+  $(INCDIR)/elf/internal.h $(INCDIR)/elf/external.h elfcore.h \
+  $(INCDIR)/safe-ctype.h
 elflink.lo: elflink.c $(INCDIR)/filenames.h $(INCDIR)/bfdlink.h \
   $(INCDIR)/hashtab.h elf-bfd.h $(INCDIR)/elf/common.h \
   $(INCDIR)/elf/internal.h $(INCDIR)/elf/external.h $(INCDIR)/safe-ctype.h \
@@ -1894,7 +1895,8 @@ elf64-sparc.lo: elf64-sparc.c $(INCDIR)/
   $(INCDIR)/opcode/sparc.h elfxx-sparc.h elf64-target.h
 elf64.lo: elf64.c elfcode.h $(INCDIR)/filenames.h $(INCDIR)/libiberty.h \
   $(INCDIR)/bfdlink.h $(INCDIR)/hashtab.h elf-bfd.h $(INCDIR)/elf/common.h \
-  $(INCDIR)/elf/internal.h $(INCDIR)/elf/external.h elfcore.h
+  $(INCDIR)/elf/internal.h $(INCDIR)/elf/external.h elfcore.h \
+  $(INCDIR)/safe-ctype.h
 mmo.lo: mmo.c $(INCDIR)/filenames.h $(INCDIR)/hashtab.h \
   $(INCDIR)/libiberty.h $(INCDIR)/elf/mmix.h $(INCDIR)/elf/reloc-macros.h \
   $(INCDIR)/opcode/mmix.h
--- bfd/Makefile.in	23 Aug 2007 16:29:49 -0000	1.224
+++ bfd/Makefile.in	24 Aug 2007 08:50:32 -0000
@@ -2141,7 +2141,8 @@ elf32-xc16x.lo: elf32-xc16x.c $(INCDIR)/
   $(INCDIR)/elf/dwarf2.h $(INCDIR)/libiberty.h elf32-target.h
 elf32.lo: elf32.c elfcode.h $(INCDIR)/filenames.h $(INCDIR)/libiberty.h \
   $(INCDIR)/bfdlink.h $(INCDIR)/hashtab.h elf-bfd.h $(INCDIR)/elf/common.h \
-  $(INCDIR)/elf/internal.h $(INCDIR)/elf/external.h elfcore.h
+  $(INCDIR)/elf/internal.h $(INCDIR)/elf/external.h elfcore.h \
+  $(INCDIR)/safe-ctype.h
 elflink.lo: elflink.c $(INCDIR)/filenames.h $(INCDIR)/bfdlink.h \
   $(INCDIR)/hashtab.h elf-bfd.h $(INCDIR)/elf/common.h \
   $(INCDIR)/elf/internal.h $(INCDIR)/elf/external.h $(INCDIR)/safe-ctype.h \
@@ -2476,7 +2477,8 @@ elf64-sparc.lo: elf64-sparc.c $(INCDIR)/
   $(INCDIR)/opcode/sparc.h elfxx-sparc.h elf64-target.h
 elf64.lo: elf64.c elfcode.h $(INCDIR)/filenames.h $(INCDIR)/libiberty.h \
   $(INCDIR)/bfdlink.h $(INCDIR)/hashtab.h elf-bfd.h $(INCDIR)/elf/common.h \
-  $(INCDIR)/elf/internal.h $(INCDIR)/elf/external.h elfcore.h
+  $(INCDIR)/elf/internal.h $(INCDIR)/elf/external.h elfcore.h \
+  $(INCDIR)/safe-ctype.h
 mmo.lo: mmo.c $(INCDIR)/filenames.h $(INCDIR)/hashtab.h \
   $(INCDIR)/libiberty.h $(INCDIR)/elf/mmix.h $(INCDIR)/elf/reloc-macros.h \
   $(INCDIR)/opcode/mmix.h
--- bfd/elf-bfd.h	4 Aug 2007 16:31:00 -0000	1.239
+++ bfd/elf-bfd.h	24 Aug 2007 08:50:32 -0000
@@ -1352,6 +1352,8 @@ struct elf_obj_tdata
   unsigned int strtab_section, dynsymtab_section;
   unsigned int symtab_shndx_section;
   unsigned int dynversym_section, dynverdef_section, dynverref_section;
+  /* The first `loadX' section from PT_LOAD PHDR using SEC_CODE.  */
+  unsigned int code_segment_section;
   file_ptr next_file_pos;
   bfd_vma gp;				/* The gp value */
   unsigned int gp_size;			/* The gp size */
--- bfd/elf.c	16 Aug 2007 18:49:42 -0000	1.410
+++ bfd/elf.c	24 Aug 2007 08:50:35 -0000
@@ -408,15 +408,20 @@ bfd_elf_get_elf_syms (bfd *ibfd,
   for (esym = extsym_buf, isym = intsym_buf, shndx = extshndx_buf;
        isym < isymend;
        esym += extsym_size, isym++, shndx = shndx != NULL ? shndx + 1 : NULL)
-    if (!(*bed->s->swap_symbol_in) (ibfd, esym, shndx, isym))
-      {
-	symoffset += (esym - (bfd_byte *) extsym_buf) / extsym_size;
-	(*_bfd_error_handler) (_("%B symbol number %lu references "
-				 "nonexistent SHT_SYMTAB_SHNDX section"),
-			       ibfd, (unsigned long) symoffset);
-	intsym_buf = NULL;
-	goto out;
-      }
+    {
+      if (!(*bed->s->swap_symbol_in) (ibfd, esym, shndx, isym))
+	{
+	  symoffset += (esym - (bfd_byte *) extsym_buf) / extsym_size;
+	  (*_bfd_error_handler) (_("%B symbol number %lu references "
+				   "nonexistent SHT_SYMTAB_SHNDX section"),
+				 ibfd, (unsigned long) symoffset);
+	  intsym_buf = NULL;
+	  goto out;
+	}
+      if (elf_tdata (ibfd)->code_segment_section != 0
+	  && ELF_ST_TYPE (isym->st_info) == STT_FUNC)
+	isym->st_shndx = elf_tdata (ibfd)->code_segment_section;
+    }
 
  out:
   if (alloc_ext != NULL)
Index: bfd/elfcode.h
===================================================================
RCS file: /cvs/src/src/bfd/elfcode.h,v
retrieving revision 1.86
diff -u -p -r1.86 elfcode.h
--- bfd/elfcode.h	14 Aug 2007 08:04:47 -0000	1.86
+++ bfd/elfcode.h	24 Aug 2007 08:50:36 -0000
@@ -73,6 +73,7 @@
 #include "bfdlink.h"
 #include "libbfd.h"
 #include "elf-bfd.h"
+#include "safe-ctype.h"
 
 /* Renaming structures, typedefs, macros and functions to be size-specific.  */
 #define Elf_External_Ehdr	NAME(Elf,External_Ehdr)
@@ -483,6 +484,20 @@ valid_section_index_p (unsigned index, u
   return (index >= SHN_LOPROC && index <= SHN_HIOS);
 }
 
+/* Find the section by BFD_SECTION_FROM_PHDR coming from the first PT_LOAD
+   using PF_X.  */
+
+static bfd_boolean
+find_code_segment (bfd *abfd ATTRIBUTE_UNUSED, asection *sect,
+		   void *obj ATTRIBUTE_UNUSED)
+{
+  return (sect->flags & SEC_ALLOC) != 0
+	 && (sect->flags & SEC_LOAD) != 0
+	 && (sect->flags & SEC_CODE) != 0
+	 && CONST_STRNEQ (sect->name, "load")
+	 && ISDIGIT (sect->name[sizeof "load" - 1]);
+}
+
 /* Check to see if the file associated with ABFD matches the target vector
    that ABFD points to.
 
@@ -861,6 +876,183 @@ elf_object_p (bfd *abfd)
       if (! _bfd_elf_setup_sections (abfd))
 	goto got_wrong_format_error;
     }
+  else
+    {
+      unsigned int phindex;
+
+      /* Even non-core ELF files may have the sections stripped.
+         Process each program header.  */
+      for (phindex = 0; phindex < i_ehdrp->e_phnum; ++phindex)
+	if (! bfd_section_from_phdr (abfd, elf_tdata (abfd)->phdr + phindex, (int) phindex))
+	  return NULL;
+    }
+
+  /* We may need to reconstruct the symbol section from PT_DYNAMIC.  */
+  if (elf_tdata (abfd)->dynsymtab_hdr.sh_size == 0)
+    {
+      unsigned int phindex;
+      Elf_Internal_Phdr *seg_dynamic = NULL;
+
+      for (phindex = 0; phindex < elf_elfheader (abfd)->e_phnum; ++phindex)
+	if (elf_tdata (abfd)->phdr[phindex].p_type == PT_DYNAMIC)
+	  {
+	    seg_dynamic = elf_tdata (abfd)->phdr + phindex;
+	    break;
+	  }
+      if (seg_dynamic != NULL)
+	{
+	  bfd_vma seg_size;
+	  bfd_byte *bufend, *bufstart, *buf;
+	  int arch_size;
+
+	  seg_size = seg_dynamic->p_filesz;
+	  buf = bufstart = alloca (seg_size);
+	  arch_size = bfd_get_arch_size (abfd);
+	  if ((arch_size == 32 || arch_size == 64)
+	      && bfd_seek (abfd, seg_dynamic->p_offset, SEEK_SET) == 0
+	      && bfd_bread (buf, seg_size, abfd) == seg_size)
+	    {
+	      size_t step;
+	      bfd_uint64_t ptr_symtab = 0, ptr_strtab = 0;
+	      Elf_Internal_Shdr *hdr_symtab = &elf_tdata (abfd)->dynsymtab_hdr;
+	      Elf_Internal_Shdr *hdr_strtab = &elf_tdata (abfd)->dynstrtab_hdr;
+	      Elf_Internal_Shdr *hdr_versym = &elf_tdata (abfd)->dynversym_hdr;
+	      bfd_vma baseaddr = 0;
+
+	      /* PT_DYNAMIC D_PTRs are VMAs, we need file offsets instead.  */
+	      for (phindex = 0;
+		   phindex < elf_elfheader (abfd)->e_phnum;
+		   ++phindex)
+	        {
+		  Elf_Internal_Phdr *seg_load;
+
+		  seg_load = elf_tdata (abfd)->phdr + phindex;
+		  if (seg_load->p_type == PT_LOAD)
+		    {
+		      baseaddr = seg_load->p_vaddr - seg_load->p_offset;
+		      break;
+		    }
+		}
+	      step = (arch_size == 32) ? sizeof (Elf32_External_Dyn)
+				       : sizeof (Elf64_External_Dyn);
+	      for (bufend = buf + seg_size; buf < bufend; buf += step)
+	        {
+		  /* We use DYN_VAL even for D_UN.D_PTR.  */
+		  bfd_uint64_t dyn_tag, dyn_val;
+
+		  if (arch_size == 32)
+		    {
+		      Elf32_External_Dyn *x_dynp_32;
+		      x_dynp_32 = (Elf32_External_Dyn *) buf;
+		      dyn_tag = bfd_h_get_32 (abfd,
+					      (bfd_byte *) x_dynp_32->d_tag);
+		      dyn_val = bfd_h_get_32 (abfd,
+					    (bfd_byte *) x_dynp_32->d_un.d_val);
+		    }
+		  else
+		    {
+		      Elf64_External_Dyn *x_dynp_64;
+		      x_dynp_64 = (Elf64_External_Dyn *) buf;
+		      dyn_tag = bfd_h_get_64 (abfd,
+					      (bfd_byte *) x_dynp_64->d_tag);
+		      dyn_val = bfd_h_get_64 (abfd,
+					    (bfd_byte *) x_dynp_64->d_un.d_val);
+		    }
+		  if (dyn_tag == DT_NULL)
+		    break;
+		  switch (dyn_tag)
+		    {
+		      case DT_SYMTAB:
+		        ptr_symtab = dyn_val - baseaddr;
+		        break;
+		      case DT_SYMENT:
+		        hdr_symtab->sh_entsize = dyn_val;
+		        break;
+		      case DT_STRTAB:
+		        ptr_strtab = dyn_val - baseaddr;
+			break;
+		      case DT_STRSZ:
+		        hdr_strtab->sh_size = dyn_val;
+			break;
+		      case DT_VERSYM:
+			hdr_versym->sh_offset = dyn_val - baseaddr;
+			break;
+		    }
+		}
+	      if (ptr_symtab != 0 && ptr_strtab != 0)
+	        {
+		  asection *code_bfd_sect;
+
+		  /* BFD_SECTION_FROM_PHDR does not enlist its result there.  */
+		  BFD_ASSERT (elf_numsections (abfd) == 0);
+		  BFD_ASSERT (elf_elfsections (abfd) == NULL);
+
+		  /* Two last sections are optional.  */
+		  elf_numsections (abfd) = 3;
+		  elf_elfsections (abfd) = bfd_zalloc2 (abfd, 3 + 1 + 1,
+					      sizeof (*elf_elfsections (abfd)));
+		  if (elf_elfsections (abfd) == NULL)
+		    return NULL;
+		  abfd->flags |= HAS_SYMS;
+
+		  /* Section #0 - SHT_NULL.  */
+		  elf_elfsections (abfd)[0] = bfd_zalloc (abfd,
+					   sizeof (*elf_elfsections (abfd)[0]));
+		  if (elf_elfsections (abfd)[0] == NULL)
+		    return NULL;
+		  elf_elfsections (abfd)[0]->sh_type = SHT_NULL;
+
+		  /* Section #1 - .dynsym. - SHT_DYNSYM.  */
+		  elf_dynsymtab (abfd) = 1;
+		  elf_elfsections (abfd)[elf_dynsymtab (abfd)] = hdr_symtab;
+		  hdr_symtab->sh_type = SHT_DYNSYM;
+		  /* We assume that the string table follows the symbol table,
+		     because there is no way in ELF to know the size of the
+		     dynamic symbol table without looking at the section
+		     headers.  */
+		  hdr_symtab->sh_size = ptr_strtab - ptr_symtab;
+		  hdr_symtab->sh_offset = ptr_symtab;
+		  hdr_symtab->sh_link = 2;
+
+		  /* Section #2 - .dynstr. - SHT_STRTAB.  */
+		  elf_elfsections (abfd)[hdr_symtab->sh_link] = hdr_strtab;
+		  hdr_strtab->sh_type = SHT_STRTAB;
+		  hdr_strtab->sh_offset = ptr_strtab;
+		  hdr_strtab->sh_link = 1;
+
+		  /* Section #3 (optional) - .gnu.version - SHT_GNU_versym.  */
+		  if (hdr_versym->sh_offset != 0)
+		    {
+		      elf_dynversym (abfd) = elf_numsections (abfd)++;
+		      elf_elfsections (abfd)[elf_dynversym (abfd)] = hdr_versym;
+		      hdr_versym->sh_type = SHT_GNU_versym;
+		      hdr_versym->sh_size = (hdr_symtab->sh_size
+					     / hdr_symtab->sh_entsize)
+					    * sizeof (Elf_External_Versym);
+		    }
+
+		  /* Section #4 (optional) for - .text - SHT_PROGBITS.
+		     We need to supply valid ELF section with associated
+		     BFD_SECTION to be able to fixup symbols' ST_SHNDXes.
+		     GDB's would ELF_SYMTAB_READ would ignore BSF_FUNCTION for
+		     symbols with BFD_ABS_SECTION otherwise.  */
+		  code_bfd_sect = bfd_sections_find_if (abfd, find_code_segment,
+							NULL);
+		  if (code_bfd_sect != NULL)
+		    {
+		      Elf_Internal_Shdr *hdr = bfd_zalloc (abfd, sizeof (*hdr));
+		      if (hdr == NULL)
+		        return NULL;
+		      elf_tdata (abfd)->code_segment_section
+						     = elf_numsections (abfd)++;
+		      elf_elfsections (abfd)[elf_tdata (abfd)
+		                             ->code_segment_section] = hdr;
+		      hdr->bfd_section = code_bfd_sect;
+		    }
+		}
+	    }
+	}
+    }
 
   /* Let the backend double check the format and override global
      information.  */