Commit Message
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
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")
}
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.
> -----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
@@ -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);