Re: [patch] Relocate phdr in read_program_header

Jan Kratochvil
2011-10-16T19:22:28Z

On Fri, 30 Sep 2011 21:31:33 +0200, Aleksandar Ristovski wrote:
> * solib-svr4.c (read_program_header): If PT_PHDR is present, use it
> to calculate correct address in case of PIE.

FSF ChangeLog should describe the changes, not their reason. Therefore:
* solib-svr4.c (read_program_header): New variable pt_phdr, initialize
it from target PT_PHDR p_vaddr, relocate sect_addr by it.

> --- gdb/solib-svr4.c 30 Aug 2011 02:48:05 -0000 1.154
> +++ gdb/solib-svr4.c 30 Sep 2011 17:47:13 -0000
> @@ -385,7 +385,7 @@ static gdb_byte *
> read_program_header (int type, int *p_sect_size, int *p_arch_size)
> {
> enum bfd_endian byte_order = gdbarch_byte_order (target_gdbarch);
> - CORE_ADDR at_phdr, at_phent, at_phnum;
> + CORE_ADDR at_phdr, at_phent, at_phnum, pt_phdr;
> int arch_size, sect_size;
> CORE_ADDR sect_addr;
> gdb_byte *buf;
> @@ -408,6 +408,10 @@ read_program_header (int type, int *p_se
> else
> return 0;
>
> + pt_phdr = (CORE_ADDR)-1; /* Address of the first entry. If not PIE,
> + this will be zero.
> + For PIE, it will be unrelocated at_phdr. */

There is CORE_ADDR_MAX. But I would prefer new `int pt_phdr_p' as "predicate"
boolean flag for valid pt_phdr value, these special values are fragile and one
has to think whether they can happen.

> +
> /* Find the requested segment. */
> if (type == -1)
> {
> @@ -427,6 +431,11 @@ read_program_header (int type, int *p_se
> return 0;
>
> if (extract_unsigned_integer ((gdb_byte *)phdr.p_type,
> + 4, byte_order) == PT_PHDR)
> + pt_phdr = extract_unsigned_integer ((gdb_byte *)phdr.p_vaddr,
> + 4, byte_order);
> +
> + if (extract_unsigned_integer ((gdb_byte *)phdr.p_type,
> 4, byte_order) == type)

That p_type could be decoded only once. Also any casta are `(type) val' with
a space accoridng to GCS (GNU Coding Standards).

> break;
> }
> @@ -453,6 +462,11 @@ read_program_header (int type, int *p_se
> return 0;
>
> if (extract_unsigned_integer ((gdb_byte *)phdr.p_type,
> + 4, byte_order) == PT_PHDR)
> + pt_phdr = extract_unsigned_integer ((gdb_byte *)phdr.p_vaddr,
> + 8, byte_order);
> +
> + if (extract_unsigned_integer ((gdb_byte *)phdr.p_type,
> 4, byte_order) == type)
> break;
> }
> @@ -467,6 +481,16 @@ read_program_header (int type, int *p_se
> 8, byte_order);
> }
>
> + /* at_phdr is real address in memory. pt_phdr is what pheader says it is.
> + Relocation offset is the difference between the two. */
> + if (pt_phdr != (CORE_ADDR)-1)
> + {
> + /* PT_PHDR is optional in the execution view, but we really need it
> + for PIE to make this work in general. However, if not found,
> + work with what you have. */
> + sect_addr = sect_addr + (at_phdr - pt_phdr);
> + }
> +
> /* Read in requested program header. */
> buf = xmalloc (sect_size);
> if (target_read_memory (sect_addr, buf, sect_size))

Just about the style, I do not see any problems of the patch, thanks.

Wrote the testcase, it is probably GNU/Linux-specific.

I would like to review yet the changes.

Thanks,
Jan

2011-10-16 Jan Kratochvil <jan.kratochvil@redhat.com>

* gdb.base/attach-pie-noexec.c: New files.
* gdb.base/attach-pie-noexec.exp: New files.

--- /dev/null
+++ b/gdb/testsuite/gdb.base/attach-pie-noexec.c
@@ -0,0 +1,25 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2011 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/>. */
+
+#include <unistd.h>
+
+int
+main (void)
+{
+ sleep (600);
+ return 0;
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.base/attach-pie-noexec.exp
@@ -0,0 +1,66 @@
+# Copyright (C) 2011 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/>.
+
+# Manipulation with PID on target is not supported.
+if [is_remote target] then {
+ return 0
+}
+
+set testfile attach-pie-noexec
+set executable ${testfile}
+set binfile ${objdir}/${subdir}/${executable}
+
+if { [prepare_for_testing ${testfile}.exp $executable "" [list debug "additional_flags=-fPIE -pie"]] } {
+ return -1
+}
+
+clean_restart $executable
+set arch ""
+set test "show architecture"
+gdb_test_multiple $test $test {
+ -re "The target architecture is set automatically \\(currently (.*)\\)\r\n$gdb_prompt $" {
+ set arch $expect_out(1,string)
+ pass $test
+ }
+}
+if ![runto_main] {
+ return 0
+}
+set test "sanity check info shared"
+gdb_test_multiple "info shared" $test {
+ -re "From\[ \t\]+To\[ \t\]+Syms Read\[ \t\]+Shared Object Library\r\n0x.*\r\n$gdb_prompt $" {
+ pass $test
+ }
+ -re "No shared libraries loaded at this time\\.\r\n$gdb_prompt $" {
+ untested ${testfile}.exp
+ }
+}
+gdb_exit
+
+if {$arch == ""} {
+ untested ${testfile}.exp
+ return 0
+}
+
+set testpid [eval exec $binfile &]
+exec sleep 2
+
+gdb_start
+file delete -- $binfile
+gdb_test "attach $testpid" "Attaching to process $testpid\r\n.*: No such file or directory\\." "attach"
+gdb_test "set architecture $arch" "The target architecture is assumed to be $arch"
+gdb_test "info shared" "From\[ \t\]+To\[ \t\]+Syms Read\[ \t\]+Shared Object Library\r\n0x.*"
+
+eval exec kill -9 $testpid