Re: [PATCH] Fix Gold/strip discrepancies for PR 11786

Jan Kratochvil
2013-10-31T16:42:23Z

Hi Doug,

On Sat, 26 Oct 2013 01:26:22 +0200, Doug Evans wrote:
> This patch addresses the discrepancy in the flags and align fields
> of PT_GNU_RELRO between Gold and strip.

originally I thought the discrepancy happened due to some speciality of Gold;
but you state Gold probably just because you use Gold by default.
On CentOS-5 x86_64 the problem happens to me even with GNU ld.

And the problem is binutils strip. It is known it modifies the executable
more than it has to, AFAIK (IIRC according to Jakub) due to the generalization
by bfd.

Red Hat OSes have been always using eu-strip instead (=elfutils strip), which
is native for ELF and modifies the file only minimally.

During my test on CentOS-5 x86_64 really binutils strip changed it:

Program Headers:
Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align
- GNU_RELRO 0x000de8 0x0000000000200de8 0x0000000000200de8 0x000218 0x000218 R 0x1
+ GNU_RELRO 0x000de8 0x0000000000200de8 0x0000000000200de8 0x000200 0x000200 R 0x1

while elfutils strip left Program Headers intact. So reviewed the patch below
and I am fine with it that way but I do not think it is the right solution to
your problem.

On Sat, 26 Oct 2013 01:26:22 +0200, Doug Evans wrote:
> --- a/gdb/solib-svr4.c
> +++ b/gdb/solib-svr4.c
> @@ -2608,6 +2608,22 @@ svr4_exec_displacement (CORE_ADDR *displacementp)
> if (memcmp (phdrp, phdr2p, sizeof (*phdrp)) == 0)
> continue;
>
> + /* Gold and strip differ on the flags and alignment of
> + PT_GNU_RELRO. See PR 11786. */
> + if (phdr2[i].p_type == PT_GNU_RELRO)
> + {
> + Elf32_External_Phdr tmp_phdr = *phdrp;
> + Elf32_External_Phdr tmp_phdr2 = *phdr2p;

One can modify directly *phdrp and *phdr2p, when we decide to ignore the
PT_GNU_RELRO differences there is no other use for that data.

> +
> + memset (tmp_phdr.p_flags, 0, 4);
> + memset (tmp_phdr.p_align, 0, 4);
> + memset (tmp_phdr2.p_flags, 0, 4);
> + memset (tmp_phdr2.p_align, 0, 4);

Missing here also FileSiz and MemSiz, during my test on CentOS-5 x86_64 the
testcase was still FAILing due to:

Program Headers:
Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align
- GNU_RELRO 0x000de8 0x0000000000200de8 0x0000000000200de8 0x000218 0x000218 R 0x1
+ GNU_RELRO 0x000de8 0x0000000000200de8 0x0000000000200de8 0x000200 0x000200 R 0x1

Therefore tested as PASSing with:

memset (tmp_phdr.p_filesz, 0, 4);
memset (tmp_phdr.p_memsz, 0, 4);
memset (tmp_phdr.p_flags, 0, 4);
memset (tmp_phdr.p_align, 0, 4);
memset (tmp_phdr2.p_filesz, 0, 4);
memset (tmp_phdr2.p_memsz, 0, 4);
memset (tmp_phdr2.p_flags, 0, 4);
memset (tmp_phdr2.p_align, 0, 4);

> +
> + if (memcmp (&tmp_phdr, &tmp_phdr2, sizeof (tmp_phdr)) == 0)
> + continue;
> + }
> +
> /* prelink can convert .plt SHT_NOBITS to SHT_PROGBITS. */
> plt2_asect = bfd_get_section_by_name (exec_bfd, ".plt");
> if (plt2_asect)
> @@ -2717,6 +2733,22 @@ svr4_exec_displacement (CORE_ADDR *displacementp)
> if (memcmp (phdrp, phdr2p, sizeof (*phdrp)) == 0)
> continue;
>
> + /* Gold and strip differ on the flags and alignment of
> + PT_GNU_RELRO. See PR 11786. */
> + if (phdr2[i].p_type == PT_GNU_RELRO)
> + {
> + Elf64_External_Phdr tmp_phdr = *phdrp;
> + Elf64_External_Phdr tmp_phdr2 = *phdr2p;

Likewise.

> +
> + memset (tmp_phdr.p_flags, 0, 4);
> + memset (tmp_phdr.p_align, 0, 8);
> + memset (tmp_phdr2.p_flags, 0, 4);
> + memset (tmp_phdr2.p_align, 0, 8);

Change to:
memset (tmp_phdr.p_filesz, 0, 8);
memset (tmp_phdr.p_memsz, 0, 8);
memset (tmp_phdr.p_flags, 0, 4);
memset (tmp_phdr.p_align, 0, 8);
memset (tmp_phdr2.p_filesz, 0, 8);
memset (tmp_phdr2.p_memsz, 0, 8);
memset (tmp_phdr2.p_flags, 0, 4);
memset (tmp_phdr2.p_align, 0, 8);

> +
> + if (memcmp (&tmp_phdr, &tmp_phdr2, sizeof (tmp_phdr)) == 0)
> + continue;
> + }
> +
> /* prelink can convert .plt SHT_NOBITS to SHT_PROGBITS. */
> plt2_asect = bfd_get_section_by_name (exec_bfd, ".plt");
> if (plt2_asect)
> diff --git a/gdb/testsuite/gdb.base/gcore-relro-pie.c b/gdb/testsuite/gdb.base/gcore-relro-pie.c
> new file mode 100644
> index 0000000..1594385
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/gcore-relro-pie.c
> @@ -0,0 +1,41 @@
> +/* Copyright 2013 Free Software Foundation, Inc.
> +
> + This file is part of GDB.
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see <http://www.gnu.org/licenses/>. */
> +
> +void
> +break_here ()

(void)

> +{
> + *(int *) 0 = 0;
> +}
> +
> +void
> +foo ()

(void)

> +{
> + break_here ();
> +}
> +
> +void
> +bar ()

(void)

> +{
> + foo ();
> +}
> +
> +int
> +main (void)
> +{
> + bar ();
> + return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/gcore-relro-pie.exp b/gdb/testsuite/gdb.base/gcore-relro-pie.exp
> new file mode 100644
> index 0000000..1fcfd8c
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/gcore-relro-pie.exp
> @@ -0,0 +1,70 @@
> +# Copyright 2013 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +
> +# PR 11786 (Gold and strip differ on flags,align fields of PT_GNU_RELRO).
> +# Generate a core file from the stripped version of the program,
> +# and then try to debug the core with the unstripped version.
> +
> +standard_testfile
> +
> +if {[prepare_for_testing $testfile.exp $testfile $srcfile {debug additional_flags=-fpie additional_flags=-pie additional_flags=-Wl,-z,relro}]} {

CentOS-5 (older GCC) compatibility:
gdb compile failed, gcc: -z: linker input file unused because linking not done
gcc: relro: linker input file unused because linking not done

coding style: Three times repeating additional_flags=...

->

if {[prepare_for_testing $testfile.exp $testfile $srcfile [list debug additional_flags=-fpie "ldflags=-pie -Wl,-z,relro"]]} {

> + return -1
> +}
> +
> +set stripped_binfile ${binfile}.stripped
> +set gcorefile ${binfile}.gcore
> +
> +set strip_program [transform strip]
> +remote_file host delete ${stripped_binfile}
> +if [run_on_host "strip" "$strip_program" "-g -o ${stripped_binfile} $binfile"] {
> + return -1
> +}

For CentOS-5 is missing this part, similar to what is in lib/gdb.exp:

# Workaround PR binutils/10802:
# Preserve the 'x' bit also for PIEs (Position Independent Executables).
set perm [file attributes ${binfile} -permissions]
file attributes ${stripped_binfile} -permissions $perm

> +
> +clean_restart ${stripped_binfile}
> +
> +# Does this gdb support gcore?
> +set test "help gcore"
> +gdb_test_multiple $test $test {
> + -re "Undefined command: .gcore.*\r\n$gdb_prompt $" {
> + # gcore command not supported -- nothing to test here.
> + unsupported "gdb does not support gcore on this target"
> + return -1
> + }
> + -re "Save a core file .*\r\n$gdb_prompt $" {
> + pass $test
> + }
> +}
> +
> +# The binary is stripped of debug info, but not minsyms.
> +if ![runto break_here] {
> + fail "Can't run to break_here"
> + return -1
> +}
> +
> +if {![gdb_gcore_cmd $gcorefile "save a corefile"]} {
> + return -1
> +}
> +
> +# Now restart gdb with the unstripped binary and load the corefile.
> +
> +clean_restart ${binfile}
> +
> +gdb_test "core ${gcorefile}" \
> + "Core was generated by .*" "re-load generated corefile"
> +
> +# Put $pc in gdb.log for debug purposes for comparison with stripped case.
> +gdb_test "x/i \$pc" "break_here.*"
> +
> +gdb_test "frame" "#0 \[^\r\n\]* break_here .*" "unstripped + core ok"

Thanks,
Jan