Re: [patch] Relocate phdr in read_program_header

Re: [patch] Relocate phdr in read_program_header

[Aleksandar Ristovski]

ping?



On 11-09-30 03:31 PM, Aleksandar Ristovski wrote:
> Hello,
>
> When attaching to a process without the executable file, gdb reads
> pheaders from the target (solib-svr4.c: read_program_header).
>
> However, it does not take into account possibility of having PIE
> executable on the other end, resulting in trying to read from
> unrelocated phdr address.
>
> This patch addresses the issue by looking for PT_PHDR; if found, uses it
> to determine relocation and adjusts address before reading it from
> target memory.
>
> Testsuite on x86_64-unknown-linux-gnu did not show any regressions.
>
>
> Thanks,
>
>
> Aleksandar Ristovski
> QNX Software Systems
>
>
>
> ... Aleksandar Ristovski <aristovski@qnx.com>
>
> * solib-svr4.c (read_program_header): If PT_PHDR is present, use it
> to calculate correct address in case of PIE.
>

Re: [patch] Relocate phdr in read_program_header

[Jan Kratochvil]

On Wed, 12 Oct 2011 20:50:00 +0200, Aleksandar Ristovski wrote:
> ping?

It does not have a testcase for example - that would be good - maybe
integrated with break-interp.exp, but I am already looking at it.


Thanks,
Jan

Re: [patch] Relocate phdr in read_program_header

[Jan Kratochvil]

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

Re: [patch] Relocate phdr in read_program_header

[Aleksandar Ristovski]

[-- Attachment #1: Type: text/plain, Size: 2283 bytes --]

On 11-10-16 03:21 PM, Jan Kratochvil wrote:
> 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.


Change log changed (see below).


>>
>> +  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.


pt_phdr_p introduced. Also, pt_phdr initialized to 0 even though not 
strictly necessary, but gcc complained about possible uninitialized use.


>
>
>> +
>>     /* 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).

Cast fixed, p_type decoded only once.

>>     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 was contemplating a testcase but I couldn't think of a trick to "make" 
gdb not find the executable... Thanks for the test.


>
> I would like to review yet the changes.

Revised patch attached.



Thanks,

Aleksandar


New change log:

<date>  Aleksandar Ristovski  <aristovski@qnx.com>

         * solib-svr4.c (read_program_header): New variable pt_phdr, 
initialize
         it from target PT_PHDR p_vaddr, relocate sect_addr by it.



[-- Attachment #2: PIE_read_program_header-201110170900.patch --]
[-- Type: text/x-patch, Size: 2678 bytes --]

Index: gdb/solib-svr4.c
===================================================================
RCS file: /cvs/src/src/gdb/solib-svr4.c,v
retrieving revision 1.157
diff -u -p -r1.157 solib-svr4.c
--- gdb/solib-svr4.c	14 Oct 2011 07:58:58 -0000	1.157
+++ gdb/solib-svr4.c	17 Oct 2011 14:48:31 -0000
@@ -364,10 +364,11 @@ 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 = 0;
   int arch_size, sect_size;
   CORE_ADDR sect_addr;
   gdb_byte *buf;
+  int pt_phdr_p = 0;
 
   /* Get required auxv elements from target.  */
   if (target_auxv_search (&current_target, AT_PHDR, &at_phdr) <= 0)
@@ -401,12 +402,23 @@ read_program_header (int type, int *p_se
       /* Search for requested PHDR.  */
       for (i = 0; i < at_phnum; i++)
 	{
+	  int p_type;
+
 	  if (target_read_memory (at_phdr + i * sizeof (phdr),
 				  (gdb_byte *)&phdr, sizeof (phdr)))
 	    return 0;
 
-	  if (extract_unsigned_integer ((gdb_byte *)phdr.p_type,
-					4, byte_order) == type)
+	  p_type = extract_unsigned_integer ((gdb_byte *) phdr.p_type,
+					     4, byte_order);
+
+	  if (p_type == PT_PHDR)
+	    {
+	      pt_phdr_p = 1;
+	      pt_phdr = extract_unsigned_integer ((gdb_byte *) phdr.p_vaddr,
+						  4, byte_order);
+	    }
+
+	  if (p_type == type)
 	    break;
 	}
 
@@ -427,12 +439,23 @@ read_program_header (int type, int *p_se
       /* Search for requested PHDR.  */
       for (i = 0; i < at_phnum; i++)
 	{
+	  int p_type;
+
 	  if (target_read_memory (at_phdr + i * sizeof (phdr),
 				  (gdb_byte *)&phdr, sizeof (phdr)))
 	    return 0;
 
-	  if (extract_unsigned_integer ((gdb_byte *)phdr.p_type,
-					4, byte_order) == type)
+	  p_type = extract_unsigned_integer ((gdb_byte *) phdr.p_type,
+					     4, byte_order);
+
+	  if (p_type == PT_PHDR)
+	    {
+	      pt_phdr_p = 1;
+	      pt_phdr = extract_unsigned_integer ((gdb_byte *) phdr.p_vaddr,
+						  8, byte_order);
+	    }
+
+	  if (p_type == type)
 	    break;
 	}
 
@@ -446,6 +469,16 @@ read_program_header (int type, int *p_se
 					    8, byte_order);
     }
 
+  /* PT_PHDR is optional, but we really need it
+     for PIE to make this work in general.  */
+
+  if (pt_phdr_p)
+    {
+      /* at_phdr is real address in memory. pt_phdr is what pheader says it is.
+	 Relocation offset is the difference between the two. */
+      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))

Re: [patch] Relocate phdr in read_program_header

[Jan Kratochvil]

On Mon, 17 Oct 2011 18:26:28 +0200, Aleksandar Ristovski wrote:
> I was contemplating a testcase but I couldn't think of a trick to
> "make" gdb not find the executable... Thanks for the test.

The testcase of mine has a problem it will stop working in future.
	[patch] Attach to running but deleted executable
	http://sourceware.org/ml/gdb-patches/2010-03/msg00950.html

The problem is the patch above is not so useful as it still does not work for
deleted libraries, there was discussed some Linux kernel support like
/proc/PID/mappedfileshandlesdirectory/ but it probably has not happened yet.

Anyway one could also use gdbserver - which is probably the case your fix
targets IIUC, the testcase will regress with a fix like above.


> <date>  Aleksandar Ristovski  <aristovski@qnx.com>
> 
>         * solib-svr4.c (read_program_header): New variable pt_phdr,
> initialize
>         it from target PT_PHDR p_vaddr, relocate sect_addr by it.

There should be now two new variables pt_phdr and pt_phdr_p. :-)

OK with that ChangeLog change in a single commit with my testcase.


Thanks,
Jan

Re: [patch] Relocate phdr in read_program_header

[Aleksandar Ristovski]

On 11-10-18 09:31 AM, Jan Kratochvil wrote:
>
> OK with that ChangeLog change in a single commit with my testcase.


Committed with fixed change log, testcase and accompanying testcase 
ChangeLog.


Thanks,

Aleksandar