diff mbox

[v5] Fix dynamic linker issue with bind-now

Message ID 1436883776-79869-1-git-send-email-petar.jovanovic@rt-rk.com
State New
Headers show

Commit Message

Petar Jovanovic July 14, 2015, 2:22 p.m. UTC 
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

Comments

H.J. Lu July 14, 2015, 6:36 p.m. UTC | #1 
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.
Andreas Schwab July 14, 2015, 8:15 p.m. UTC | #2 
"H.J. Lu" <hjl.tools@gmail.com> writes:

> Please add () around "ranges[0].start + ranges[0].size".

Why?

Andreas.
Petar Jovanovic July 15, 2015, 6:20 p.m. UTC | #3 
-----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
H.J. Lu July 15, 2015, 6:31 p.m. UTC | #4 
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.
H.J. Lu July 15, 2015, 7:09 p.m. UTC | #5 
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.
Roland McGrath July 15, 2015, 7:31 p.m. UTC | #6 
> 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.
Petar Jovanovic July 15, 2015, 8:45 p.m. UTC | #7 
-----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 mbox

Patch

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 \