Message ID | 1436883776-79869-1-git-send-email-petar.jovanovic@rt-rk.com |
---|---|
State | New |
Headers | show |
On Tue, Jul 14, 2015 at 7:22 AM, Petar Jovanovic <petar.jovanovic@rt-rk.com> wrote: > Fix the bind-now case when DT_REL and DT_JMPREL sections are separate > and there is a gap between them. This patch fixes bug #14341. > --- > v5: > - Added a ChangeLog entry and bug number into commit message. > > v4: > - Moved the Makefile part into sysdeps/x86_64/Makefile, so the test is > executed for x86-64 only > > v3: > - addressed comments raised by Mike Frysinger > - use of test-skeleton.c > - use -Wl,-z,now instead of LD_BIND_NOW=1 > - moved comments to the start of the test file > > v2: > - addressed all comments raised by Andreas Schwab > > ChangeLog | 9 +++++++++ > elf/dynamic-link.h | 4 +++- > elf/tst-split-dynreloc.c | 28 ++++++++++++++++++++++++++++ > elf/tst-split-dynreloc.lds | 6 ++++++ > sysdeps/x86_64/Makefile | 4 ++++ > 5 files changed, 50 insertions(+), 1 deletion(-) > create mode 100644 elf/tst-split-dynreloc.c > create mode 100644 elf/tst-split-dynreloc.lds > > diff --git a/ChangeLog b/ChangeLog > index dfef5e0..594b774 100644 > --- a/ChangeLog > +++ b/ChangeLog > @@ -1,3 +1,12 @@ > +2015-07-14 Petar Jovanovic <petar.jovanovic@rt-rk.com> > + > + [BZ #14341] > + * elf/dynamic-link.h (elf_machine_lazy_rel): Properly handle the > + case when there is a gap between DT_REL and DT_JMPREL sections. > + * elf/tst-split-dynreloc.c: New file. > + * elf/tst-split-dynreloc.lds: New file. > + * sysdeps/x86_64/Makefile: Add new test. > + Please put ChangeLog entry in your commit log, not in ChangeLog directly. Otherwise, your patch may not apply. If you can't check it in yourself,. please generate the patch with " gcc format-patch". > 2015-04-01 Wilco Dijkstra <wdijkstr@arm.com> > > * sysdeps/aarch64/fpu/math_private.h > diff --git a/elf/dynamic-link.h b/elf/dynamic-link.h > index 8d428e2..83e760b 100644 > --- a/elf/dynamic-link.h > +++ b/elf/dynamic-link.h > @@ -135,7 +135,9 @@ elf_machine_lazy_rel (struct link_map *map, > \ > if (ranges[0].start + ranges[0].size == (start + size)) \ > ranges[0].size -= size; \ > - if (! ELF_DURING_STARTUP && ((do_lazy) || ranges[0].size == 0)) \ > + if (! ELF_DURING_STARTUP \ > + && ((do_lazy) || ranges[0].size == 0 \ Please put " ranges[0].size == 0" on separate line. > + || ranges[0].start + ranges[0].size != start)) \ Please add () around "ranges[0].start + ranges[0].size". > { \ > ranges[1].start = start; \ > ranges[1].size = size; \ > diff --git a/elf/tst-split-dynreloc.c b/elf/tst-split-dynreloc.c > new file mode 100644 > index 0000000..bdb6b7c > --- /dev/null > +++ b/elf/tst-split-dynreloc.c > @@ -0,0 +1,28 @@ > +/* This test will be used to create an executable with a specific > + section layout in which .rela.dyn and .rela.plt are not contiguous. > + For x86 case, readelf will report something like: > + > + ... > + [10] .rela.dyn RELA > + [11] .bar PROGBITS > + [12] .rela.plt RELA > + ... > + > + This is important as this case was not correctly handled by dynamic > + linker in the bind-now case, and the second section was never > + processed. */ > + > +#include <stdio.h> > + > +static int __attribute__ ((section(".bar"))) bar = 0x12345678; Please make "bar" readonly to avoid writable and executable segment. > +static const char foo[] = "foo"; > + > +static int > +do_test (void) > +{ > + printf ("%s %d\n", foo, bar); > + return 0; > +} > + > +#define TEST_FUNCTION do_test () > +#include "../test-skeleton.c" > diff --git a/elf/tst-split-dynreloc.lds b/elf/tst-split-dynreloc.lds > new file mode 100644 > index 0000000..ed0a656 > --- /dev/null > +++ b/elf/tst-split-dynreloc.lds > @@ -0,0 +1,6 @@ > +SECTIONS > +{ > + .bar : { *(.bar) } > +} > +INSERT AFTER .rela.dyn; > + > diff --git a/sysdeps/x86_64/Makefile b/sysdeps/x86_64/Makefile > index ef70a50..b9d949c 100644 > --- a/sysdeps/x86_64/Makefile > +++ b/sysdeps/x86_64/Makefile > @@ -38,6 +38,10 @@ tests += tst-audit3 tst-audit4 tst-audit5 tst-audit10 > ifeq (yes,$(config-cflags-avx)) > tests += tst-audit6 tst-audit7 > endif > + > +tests += tst-split-dynreloc > +LDFLAGS-tst-split-dynreloc = -Wl,-T,tst-split-dynreloc.lds -Wl,-z,now > + > modules-names += tst-auditmod3a tst-auditmod3b \ > tst-auditmod4a tst-auditmod4b \ > tst-auditmod5a tst-auditmod5b \ Please verify your testcase with the linker from the current binutils master branch. The new linker doesn't have DT_JMPREL at all when -z now is used and the testcase works even without your patch. Please update the testcase such that it always fails without the fix.
"H.J. Lu" <hjl.tools@gmail.com> writes:
> Please add () around "ranges[0].start + ranges[0].size".
Why?
Andreas.
-----Original Message----- From: H.J. Lu [mailto:hjl.tools@gmail.com] Sent: Tuesday, July 14, 2015 8:36 PM To: Petar Jovanovic <petar.jovanovic@rt-rk.com> Cc: GNU C Library <libc-alpha@sourceware.org>; Roland McGrath <roland@hack.frob.com>; Mike Frysinger <vapier@gentoo.org> Subject: Re: [PATCH v5] Fix dynamic linker issue with bind-now > Please put ChangeLog entry in your commit log, not in ChangeLog directly. Otherwise, your patch may not apply. If you can't check it in yourself,. please generate the patch with " gcc format-patch". You mean with "git format-patch"? So far, I have been using git send-email, but I can create it with "git format-patch" and attach it to the email. I will put ChangeLog as part of the message text. >> + && ((do_lazy) || ranges[0].size == 0 \ > Please put " ranges[0].size == 0" on separate line. Done in the next patch set (v6). >> + || ranges[0].start + ranges[0].size != start)) \ > Please add () around "ranges[0].start + ranges[0].size". Done in the next patch set (v6). >> +static int __attribute__ ((section(".bar"))) bar = 0x12345678; > Please make "bar" readonly to avoid writable and executable segment. As "static int const __attribute__ ..."? In that case, bar is removed from the sections list, likely due to some optimizations. > Please verify your testcase with the linker from the current binutils master branch. The new linker doesn't have DT_JMPREL at all when -z now is used and the testcase works even without your patch. Please update the testcase such that it always fails without the fix. It seems that some (recent) changes from binutils had impact on this test case. I have modified it in patch v6, so it fails again. I find the change obvious, that we could even leave out the test case. But it is there in v6. Regards, Petar
On Wed, Jul 15, 2015 at 11:20 AM, Petar Jovanovic <petar.jovanovic@rt-rk.com> wrote: > >>> +static int __attribute__ ((section(".bar"))) bar = 0x12345678; >> Please make "bar" readonly to avoid writable and executable segment. > > As "static int const __attribute__ ..."? > In that case, bar is removed from the sections list, likely due to some optimizations. > Please find a way. You can put int const __attribute__ ((section(".bar"))) bar = 0x12345678; in a separate file.
On Wed, Jul 15, 2015 at 11:20 AM, Petar Jovanovic <petar.jovanovic@rt-rk.com> wrote: > -----Original Message----- > From: H.J. Lu [mailto:hjl.tools@gmail.com] > Sent: Tuesday, July 14, 2015 8:36 PM > To: Petar Jovanovic <petar.jovanovic@rt-rk.com> > Cc: GNU C Library <libc-alpha@sourceware.org>; Roland McGrath <roland@hack.frob.com>; Mike Frysinger <vapier@gentoo.org> > Subject: Re: [PATCH v5] Fix dynamic linker issue with bind-now > >> Please put ChangeLog entry in your commit log, not in ChangeLog directly. Otherwise, your patch may not apply. If you can't check it in yourself,. please generate the patch with " gcc format-patch". > > You mean with "git format-patch"? So far, I have been using git send-email, but I can create it with "git format-patch" and attach it to the email. I will put ChangeLog as part of the message text. I didn't notice that you used "git send-email". If "git am" works, it is fine.
> As "static int const __attribute__ ..."? > In that case, bar is removed from the sections list, likely due to some optimizations. __attribute__ ((used)) will probably fix that.
-----Original Message----- From: Roland McGrath [mailto:roland@hack.frob.com] Sent: Wednesday, July 15, 2015 9:32 PM To: Petar Jovanovic <petar.jovanovic@rt-rk.com> Cc: 'H.J. Lu' <hjl.tools@gmail.com>; 'GNU C Library' <libc-alpha@sourceware.org>; 'Mike Frysinger' <vapier@gentoo.org> Subject: RE: [PATCH v5] Fix dynamic linker issue with bind-now >> As "static int const __attribute__ ..."? >> In that case, bar is removed from the sections list, likely due to some optimizations. >__attribute__ ((used)) will probably fix that. It seems that just removing 'static' is sufficient. I will send another patch with: const int __attribute__ ((section(".bar"))) bar = 0x12345678; Regards, Petar
diff --git a/ChangeLog b/ChangeLog index dfef5e0..594b774 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,12 @@ +2015-07-14 Petar Jovanovic <petar.jovanovic@rt-rk.com> + + [BZ #14341] + * elf/dynamic-link.h (elf_machine_lazy_rel): Properly handle the + case when there is a gap between DT_REL and DT_JMPREL sections. + * elf/tst-split-dynreloc.c: New file. + * elf/tst-split-dynreloc.lds: New file. + * sysdeps/x86_64/Makefile: Add new test. + 2015-04-01 Wilco Dijkstra <wdijkstr@arm.com> * sysdeps/aarch64/fpu/math_private.h diff --git a/elf/dynamic-link.h b/elf/dynamic-link.h index 8d428e2..83e760b 100644 --- a/elf/dynamic-link.h +++ b/elf/dynamic-link.h @@ -135,7 +135,9 @@ elf_machine_lazy_rel (struct link_map *map, \ if (ranges[0].start + ranges[0].size == (start + size)) \ ranges[0].size -= size; \ - if (! ELF_DURING_STARTUP && ((do_lazy) || ranges[0].size == 0)) \ + if (! ELF_DURING_STARTUP \ + && ((do_lazy) || ranges[0].size == 0 \ + || ranges[0].start + ranges[0].size != start)) \ { \ ranges[1].start = start; \ ranges[1].size = size; \ diff --git a/elf/tst-split-dynreloc.c b/elf/tst-split-dynreloc.c new file mode 100644 index 0000000..bdb6b7c --- /dev/null +++ b/elf/tst-split-dynreloc.c @@ -0,0 +1,28 @@ +/* This test will be used to create an executable with a specific + section layout in which .rela.dyn and .rela.plt are not contiguous. + For x86 case, readelf will report something like: + + ... + [10] .rela.dyn RELA + [11] .bar PROGBITS + [12] .rela.plt RELA + ... + + This is important as this case was not correctly handled by dynamic + linker in the bind-now case, and the second section was never + processed. */ + +#include <stdio.h> + +static int __attribute__ ((section(".bar"))) bar = 0x12345678; +static const char foo[] = "foo"; + +static int +do_test (void) +{ + printf ("%s %d\n", foo, bar); + return 0; +} + +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c" diff --git a/elf/tst-split-dynreloc.lds b/elf/tst-split-dynreloc.lds new file mode 100644 index 0000000..ed0a656 --- /dev/null +++ b/elf/tst-split-dynreloc.lds @@ -0,0 +1,6 @@ +SECTIONS +{ + .bar : { *(.bar) } +} +INSERT AFTER .rela.dyn; + diff --git a/sysdeps/x86_64/Makefile b/sysdeps/x86_64/Makefile index ef70a50..b9d949c 100644 --- a/sysdeps/x86_64/Makefile +++ b/sysdeps/x86_64/Makefile @@ -38,6 +38,10 @@ tests += tst-audit3 tst-audit4 tst-audit5 tst-audit10 ifeq (yes,$(config-cflags-avx)) tests += tst-audit6 tst-audit7 endif + +tests += tst-split-dynreloc +LDFLAGS-tst-split-dynreloc = -Wl,-T,tst-split-dynreloc.lds -Wl,-z,now + modules-names += tst-auditmod3a tst-auditmod3b \ tst-auditmod4a tst-auditmod4b \ tst-auditmod5a tst-auditmod5b \