Re: [PATCH v5] Fix dynamic linker issue with bind-now

["H.J. Lu" <hjl dot tools at gmail dot com>]

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.