[patch 2/6] PIE: Attach binary even after ld.so re-prelinked underneath

[patch 2/6] PIE: Attach binary even after ld.so re-prelinked underneath

[Jan Kratochvil]

Hi,

when I have seen the PIE-binary-changed-sice-start tested also
ld.so-changed-since-start and it did not work.

There is questionable when to print and when to not to print non-fatal memory
read errors.  Similar change I requested in
	Re: [RFC/ia64] memory error when reading wrong core file
	http://sourceware.org/ml/gdb-patches/2010-02/msg00001.html
so that GDB prints at least as a warning
	Cannot access memory at address ...
when some unexpected error happens.

Contrary to it there is not such warning present in the code below as in the
case of non-valgrind PIE with re-prelinked ld.so the memory read error happens
there.  Some GDB code rework could probably avoid it.

Just for example all the memory read errors in read_program_header or even
- primarily - scan_dyntag are currently IMO-incorrectly silent and others.
Therefore posted the patch this way, making all these warnings printed and
possibly fixing code attempting incorrect reads along the way could be done by
a different patch.

No regressions on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu for the whole
patch series together.


Thanks,
Jan


gdb/
2010-03-29  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* auxv.c (ld_so_xfer_auxv): Do not error on failed read of data_address.

gdb/testsuite/
2010-03-29  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/break-interp.exp (test_attach): Keep $interp changed.  Move
	its restore after the <$relink_args != ""> loop.  new comment.

--- a/gdb/auxv.c
+++ b/gdb/auxv.c
@@ -96,7 +96,16 @@ ld_so_xfer_auxv (gdb_byte *readbuf,
 
   pointer_address = SYMBOL_VALUE_ADDRESS (msym);
 
-  data_address = read_memory_typed_address (pointer_address, ptr_type);
+  /* While it is an error I am not aware how to solve attaching to PIE under
+     valgrind --db-attach=yes different way when ld.so on-disk file has prelink
+     change in the meantime.  Currently GDB will drop back to procfs_xfer_auxv
+     (so that at least non-valgrind PIE attachments with prelink change of
+     ld.so work).  To make it working even with valgrind is PR 11440 requiring
+     a valgrind extension.  */
+  if (target_read_memory (pointer_address, ptr_buf, ptr_size) != 0)
+    return -1;
+
+  data_address = extract_typed_address (ptr_buf, ptr_type);
 
   /* Possibly still not initialized such as during an inferior startup.  */
   if (data_address == 0)
--- a/gdb/testsuite/gdb.base/break-interp.exp
+++ b/gdb/testsuite/gdb.base/break-interp.exp
@@ -419,12 +419,16 @@ proc test_attach {file displacement {relink_args ""}} {
 	global exec interp_saved interp
 
 	foreach relink {YES NO} {
-	    if {[prelink$relink $relink_args [file tail $exec]]
-	        && [copy $interp_saved $interp]} {
+	    # It would be more correct to also [copy $interp_saved $interp]
+	    # here to really test just different prelink of $exec.
+	    # But we would need a separate test for different prelink of ld.so
+	    # where a bug occured.  It is now all merged into this single test.
+	    if [prelink$relink $relink_args [file tail $exec]] {
 		# /proc/PID/exe cannot be loaded as it is "EXECNAME (deleted)".
 		test_attach_gdb $exec $pid $displacement "attach-relink$relink"
 	    }
 	}
+	copy $interp_saved $interp
     }
 
     remote_exec host "kill -9 $pid"

ping: [patch 2/6] PIE: Attach binary even after ld.so re-prelinked underneath

[Jan Kratochvil]

Hi,

originally posted as:
	[patch 2/6] PIE: Attach binary even after ld.so re-prelinked underneath
	http://sourceware.org/ml/gdb-patches/2010-03/msg01000.html

Rediffed only.

------------------------------------------------------------------------------

when I have seen the PIE-binary-changed-sice-start tested also
ld.so-changed-since-start and it did not work.

There is questionable when to print and when to not to print non-fatal memory
read errors.  Similar change I requested in
	Re: [RFC/ia64] memory error when reading wrong core file
	http://sourceware.org/ml/gdb-patches/2010-02/msg00001.html
so that GDB prints at least as a warning
	Cannot access memory at address ...
when some unexpected error happens.

Contrary to it there is not such warning present in the code below as in the
case of non-valgrind PIE with re-prelinked ld.so the memory read error happens
there.  Some GDB code rework could probably avoid it.

Just for example all the memory read errors in read_program_header or even
- primarily - scan_dyntag are currently IMO-incorrectly silent and others.
Therefore posted the patch this way, making all these warnings printed and
possibly fixing code attempting incorrect reads along the way could be done by
a different patch.

No regressions on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu for the whole
patch series together.


Thanks,
Jan


gdb/
2010-03-29  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* auxv.c (ld_so_xfer_auxv): Do not error on failed read of data_address.

gdb/testsuite/
2010-03-29  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/break-interp.exp (test_attach): Keep $interp changed.  Move
	its restore after the <$relink_args != ""> loop.  new comment.

--- a/gdb/auxv.c
+++ b/gdb/auxv.c
@@ -96,7 +96,16 @@ ld_so_xfer_auxv (gdb_byte *readbuf,
 
   pointer_address = SYMBOL_VALUE_ADDRESS (msym);
 
-  data_address = read_memory_typed_address (pointer_address, ptr_type);
+  /* While it is an error I am not aware how to solve attaching to PIE under
+     valgrind --db-attach=yes different way when ld.so on-disk file has prelink
+     change in the meantime.  Currently GDB will drop back to procfs_xfer_auxv
+     (so that at least non-valgrind PIE attachments with prelink change of
+     ld.so work).  To make it working even with valgrind is PR 11440 requiring
+     a valgrind extension.  */
+  if (target_read_memory (pointer_address, ptr_buf, ptr_size) != 0)
+    return -1;
+
+  data_address = extract_typed_address (ptr_buf, ptr_type);
 
   /* Possibly still not initialized such as during an inferior startup.  */
   if (data_address == 0)
--- a/gdb/testsuite/gdb.base/break-interp.exp
+++ b/gdb/testsuite/gdb.base/break-interp.exp
@@ -420,12 +420,16 @@ proc test_attach {file displacement {relink_args ""}} {
 	global exec interp_saved interp
 
 	foreach relink {YES NO} {
-	    if {[prelink$relink $relink_args [file tail $exec]]
-	        && [copy $interp_saved $interp]} {
+	    # It would be more correct to also [copy $interp_saved $interp]
+	    # here to really test just different prelink of $exec.
+	    # But we would need a separate test for different prelink of ld.so
+	    # where a bug occured.  It is now all merged into this single test.
+	    if [prelink$relink $relink_args [file tail $exec]] {
 		# /proc/PID/exe cannot be loaded as it is "EXECNAME (deleted)".
 		test_attach_gdb $exec $pid $displacement "attach-relink$relink"
 	    }
 	}
+	copy $interp_saved $interp
     }
 
     remote_exec host "kill -9 $pid"

Re: ping: [patch 2/6] PIE: Attach binary even after ld.so re-prelinked underneath

[Joel Brobecker]

> gdb/
> 2010-03-29  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* auxv.c (ld_so_xfer_auxv): Do not error on failed read of data_address.
> 
> gdb/testsuite/
> 2010-03-29  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* gdb.base/break-interp.exp (test_attach): Keep $interp changed.  Move
> 	its restore after the <$relink_args != ""> loop.  new comment.

The code changes themselves look reasonable, but as discussed on IRC,
I think the comments could have more info.  If you want, we can commit
the code, and update the comments as a followup patch. I think we can
also converge on the comments within a day or two, so your choice.

> +  /* While it is an error I am not aware how to solve attaching to PIE under
> +     valgrind --db-attach=yes different way when ld.so on-disk file has prelink
> +     change in the meantime.  Currently GDB will drop back to procfs_xfer_auxv
> +     (so that at least non-valgrind PIE attachments with prelink change of
> +     ld.so work).  To make it working even with valgrind is PR 11440 requiring
> +     a valgrind extension.  */

In this case, I think we need to split the explanation that provides
the situation when the memory read error could happen; what we are doing
in this case (returning -1 to fallback on another xfer method) and its
consequences in terms of correctness; and the problem that is left to
solve. Something like this, perhaps?

    /* The location of the _dl_auxv symbol may no longer be correct if
       ld.so was prelinked after the program was started, which can cause
       the memory read at its address to fail.  When that happens, return
       -1 to fallback on another mechanism for retrieving the AUXV.

       While this allows things to work in most cases, this still does not
       work in the case where we are attaching the debugger to a PIE
       running under valgrind and --db-attach=yes (again, when the on-disk
       ld.so file was changed due to prelink after the process was started).
       [WHY???]
       However, we have not found a solution to that problem yet, and we
       think that we might need a valgrind extension to make it work.
       This is PR 11440.  */

>  	foreach relink {YES NO} {
> -	    if {[prelink$relink $relink_args [file tail $exec]]
> -	        && [copy $interp_saved $interp]} {
> +	    # It would be more correct to also [copy $interp_saved $interp]
> +	    # here to really test just different prelink of $exec.
> +	    # But we would need a separate test for different prelink of ld.so
> +	    # where a bug occured.  It is now all merged into this single test.
> +	    if [prelink$relink $relink_args [file tail $exec]] {
>  		# /proc/PID/exe cannot be loaded as it is "EXECNAME (deleted)".
>  		test_attach_gdb $exec $pid $displacement "attach-relink$relink"

As discussed on IRC, let's expand the comment to provide the explanation
that your provided there.

Basically: * what we used to do (the copy) and why (restore ld.so to
             prevent an ld.so prelink)
           * why we are no longer doing it
             (making sure that we test GDB with ld.so being prelinked too)
-- 
Joel

Re: ping: [patch 2/6] PIE: Attach binary even after ld.so re-prelinked underneath

[Jan Kratochvil]

On Tue, 29 Jun 2010 20:21:32 +0200, Joel Brobecker wrote:
>     /* The location of the _dl_auxv symbol may no longer be correct if
>        ld.so was prelinked after the program was started, which can cause
>        the memory read at its address to fail.  When that happens, return
>        -1 to fallback on another mechanism for retrieving the AUXV.
> 
>        While this allows things to work in most cases, this still does not
>        work in the case where we are attaching the debugger to a PIE
>        running under valgrind and --db-attach=yes (again, when the on-disk
>        ld.so file was changed due to prelink after the process was started).
>        [WHY???]
>        However, we have not found a solution to that problem yet, and we
>        think that we might need a valgrind extension to make it work.
>        This is PR 11440.  */

I see now my comment did not specify the currently failing set of cases
exactly.  Moreover I was not aware of the PIEs prelinking - they are never
prelinked themselves - it is true it does not make much sense.  Still such
functionality is needed for debugging executed prelinked PIC libraries (such
cas ld.so and libc.so, other libraries usually do not support their execution
and they crash on such attempt).  Still the testsuite should therefore target
more executing prelinked PICs than prelinked PIEs (currently kept unchanged).

Therefore used some merged+modified comment:

  /* The location of the _dl_auxv symbol may no longer be correct if
     ld.so runs at a different address than the one present in the file.
     This is very common case - for unprelinked ld.so or with a PIE executable.
     PIE executable forces random address even for libraries already being
     prelinked to some address.  PIE executables themselves are never prelinked
     even on prelinked systems.  Prelinking of a PIE executable would block
     their purpose of randomizing load of everything including the executable.

     If the memory read fails, return -1 to fallback on another mechanism for
     retrieving the AUXV.

     In most cases of a PIE running under valgrind there is no way how to find
     out the base addresses of any of ld.so, executable or AUXV as everything
     is randomized and /proc information is not relevant for the virtual
     executable running under valgrind.  We think that we might need a valgrind
     extension to make it work.  This is PR 11440.  */


> >  	foreach relink {YES NO} {
> > -	    if {[prelink$relink $relink_args [file tail $exec]]
> > -	        && [copy $interp_saved $interp]} {
> > +	    # It would be more correct to also [copy $interp_saved $interp]
> > +	    # here to really test just different prelink of $exec.
> > +	    # But we would need a separate test for different prelink of ld.so
> > +	    # where a bug occured.  It is now all merged into this single test.
> > +	    if [prelink$relink $relink_args [file tail $exec]] {
> >  		# /proc/PID/exe cannot be loaded as it is "EXECNAME (deleted)".
> >  		test_attach_gdb $exec $pid $displacement "attach-relink$relink"
> 
> As discussed on IRC, let's expand the comment to provide the explanation
> that your provided there.
> 
> Basically: * what we used to do (the copy) and why (restore ld.so to
>              prevent an ld.so prelink)
>            * why we are no longer doing it
>              (making sure that we test GDB with ld.so being prelinked too)

Used:
            # Formerly this test was testing only prelinking of $EXEC.  As the
            # prelink command prelinks automatically even all $EXEC's libraries
            # even $INTERP got prelinked.  Therefore we formerly had to
            # `[copy $interp_saved $interp]' to make $INTERP not affected by
            # this prelinking of $EXEC.
            #
            # But now we need to test even prelinking of $INTERP.  We could
            # create a separate test to test just the $INTERP prelinking.  For
            # the test simplicity we merged this test and the test above by not
            # restoring $INTERP after $EXEC prelink.  $INTERP gets restored 
            # later below.


Thanks,
Jan


gdb/
2010-07-04  Jan Kratochvil  <jan.kratochvil@redhat.com>
	    Joel Brobecker  <brobecker@adacore.com>

	* auxv.c (ld_so_xfer_auxv): Do not error on failed read of data_address.

gdb/testsuite/
2010-07-04  Jan Kratochvil  <jan.kratochvil@redhat.com>
	    Joel Brobecker  <brobecker@adacore.com>

	* gdb.base/break-interp.exp (test_attach): Keep $interp changed.  Move
	its restore after the <$relink_args != ""> loop.  new comment.

--- a/gdb/auxv.c
+++ b/gdb/auxv.c
@@ -96,7 +96,27 @@ ld_so_xfer_auxv (gdb_byte *readbuf,
 
   pointer_address = SYMBOL_VALUE_ADDRESS (msym);
 
-  data_address = read_memory_typed_address (pointer_address, ptr_type);
+  /* The location of the _dl_auxv symbol may no longer be correct if
+     ld.so runs at a different address than the one present in the file.
+     This is very common case - for unprelinked ld.so or with a PIE executable.
+     PIE executable forces random address even for libraries already being
+     prelinked to some address.  PIE executables themselves are never prelinked
+     even on prelinked systems.  Prelinking of a PIE executable would block
+     their purpose of randomizing load of everything including the executable.
+
+     If the memory read fails, return -1 to fallback on another mechanism for
+     retrieving the AUXV.
+
+     In most cases of a PIE running under valgrind there is no way how to find
+     out the base addresses of any of ld.so, executable or AUXV as everything
+     is randomized and /proc information is not relevant for the virtual
+     executable running under valgrind.  We think that we might need a valgrind
+     extension to make it work.  This is PR 11440.  */
+
+  if (target_read_memory (pointer_address, ptr_buf, ptr_size) != 0)
+    return -1;
+
+  data_address = extract_typed_address (ptr_buf, ptr_type);
 
   /* Possibly still not initialized such as during an inferior startup.  */
   if (data_address == 0)
--- a/gdb/testsuite/gdb.base/break-interp.exp
+++ b/gdb/testsuite/gdb.base/break-interp.exp
@@ -425,12 +425,24 @@ proc test_attach {file displacement {relink_args ""}} {
 	global exec interp_saved interp
 
 	foreach relink {YES NO} {
-	    if {[prelink$relink $relink_args [file tail $exec]]
-	        && [copy $interp_saved $interp]} {
+	    # Formerly this test was testing only prelinking of $EXEC.  As the
+	    # prelink command prelinks automatically even all $EXEC's libraries
+	    # even $INTERP got prelinked.  Therefore we formerly had to
+	    # `[copy $interp_saved $interp]' to make $INTERP not affected by
+	    # this prelinking of $EXEC.
+	    #
+	    # But now we need to test even prelinking of $INTERP.  We could
+	    # create a separate test to test just the $INTERP prelinking.  For
+	    # the test simplicity we merged this test and the test above by not
+	    # restoring $INTERP after $EXEC prelink.  $INTERP gets restored
+	    # later below.
+
+	    if [prelink$relink $relink_args [file tail $exec]] {
 		# /proc/PID/exe cannot be loaded as it is "EXECNAME (deleted)".
 		test_attach_gdb $exec $pid $displacement "attach-relink$relink"
 	    }
 	}
+	copy $interp_saved $interp
     }
 
     remote_exec host "kill -9 $pid"

Re: ping: [patch 2/6] PIE: Attach binary even after ld.so re-prelinked underneath

[Joel Brobecker]

V> Therefore used some merged+modified comment:

Much clearer, thank you (and I'm continuing to learn as I go)...

> gdb/
> 2010-07-04  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 	    Joel Brobecker  <brobecker@adacore.com>
> 
> 	* auxv.c (ld_so_xfer_auxv): Do not error on failed read of data_address.
> 
> gdb/testsuite/
> 2010-07-04  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 	    Joel Brobecker  <brobecker@adacore.com>
> 
> 	* gdb.base/break-interp.exp (test_attach): Keep $interp changed.  Move

OK with the tiny changes below.

> In most cases of a PIE running under valgrind there is no way how to find
                                                         ^^^^^^^^^^^^^
                                                         no way to find

> # Formerly this test was testing only prelinking of $EXEC.  As the
> # prelink command prelinks automatically even all $EXEC's libraries
                    automatically prelinks all of $EXEC's libraries,
                    (the comma is important)
> # even $INTERP got prelinked.  Therefore we formerly had to
                                        ^^^ Therefore,
> # `[copy $interp_saved $interp]' to make $INTERP not affected by
> # this prelinking of $EXEC.
> #
> # But now we need to test even prelinking of $INTERP.  We could
> # create a separate test to test just the $INTERP prelinking.  For
> # the test simplicity we merged this test and the test above by not
    ^^^^ test simplicity, we merged [...]
> # restoring $INTERP after $EXEC prelink.  $INTERP gets restored 
> # later below.

-- 
Joel

Re: ping: [patch 2/6] PIE: Attach binary even after ld.so re-prelinked underneath

[Jan Kratochvil]

Hi Joel,

checked-in.


Thanks,
Jan


http://sourceware.org/ml/gdb-cvs/2010-07/msg00025.html

--- src/gdb/ChangeLog	2010/07/05 17:57:49	1.11964
+++ src/gdb/ChangeLog	2010/07/05 17:58:55	1.11965
@@ -1,6 +1,11 @@
 2010-07-05  Jan Kratochvil  <jan.kratochvil@redhat.com>
 	    Joel Brobecker  <brobecker@adacore.com>
 
+	* auxv.c (ld_so_xfer_auxv): Do not error on failed read of data_address.
+
+2010-07-05  Jan Kratochvil  <jan.kratochvil@redhat.com>
+	    Joel Brobecker  <brobecker@adacore.com>
+
 	Fix attaching to PIEs prelinked on the disk after the process was
 	started.
 	* solib-svr4.c (svr4_exec_displacement): New variable arch_size.
--- src/gdb/auxv.c	2010/05/18 19:23:37	1.27
+++ src/gdb/auxv.c	2010/07/05 17:58:55	1.28
@@ -96,7 +96,27 @@
 
   pointer_address = SYMBOL_VALUE_ADDRESS (msym);
 
-  data_address = read_memory_typed_address (pointer_address, ptr_type);
+  /* The location of the _dl_auxv symbol may no longer be correct if
+     ld.so runs at a different address than the one present in the file.
+     This is very common case - for unprelinked ld.so or with a PIE executable.
+     PIE executable forces random address even for libraries already being
+     prelinked to some address.  PIE executables themselves are never prelinked
+     even on prelinked systems.  Prelinking of a PIE executable would block
+     their purpose of randomizing load of everything including the executable.
+
+     If the memory read fails, return -1 to fallback on another mechanism for
+     retrieving the AUXV.
+
+     In most cases of a PIE running under valgrind there is no way to find
+     out the base addresses of any of ld.so, executable or AUXV as everything
+     is randomized and /proc information is not relevant for the virtual
+     executable running under valgrind.  We think that we might need a valgrind
+     extension to make it work.  This is PR 11440.  */
+
+  if (target_read_memory (pointer_address, ptr_buf, ptr_size) != 0)
+    return -1;
+
+  data_address = extract_typed_address (ptr_buf, ptr_type);
 
   /* Possibly still not initialized such as during an inferior startup.  */
   if (data_address == 0)
--- src/gdb/testsuite/ChangeLog	2010/07/05 17:57:50	1.2371
+++ src/gdb/testsuite/ChangeLog	2010/07/05 17:58:56	1.2372
@@ -1,6 +1,12 @@
 2010-07-05  Jan Kratochvil  <jan.kratochvil@redhat.com>
 	    Joel Brobecker  <brobecker@adacore.com>
 
+	* gdb.base/break-interp.exp (test_attach): Keep $interp changed.  Move
+	its restore after the <$relink_args != ""> loop.  new comment.
+
+2010-07-05  Jan Kratochvil  <jan.kratochvil@redhat.com>
+	    Joel Brobecker  <brobecker@adacore.com>
+
 	* gdb.base/break-interp.exp: Run $binpie with new value "ATTACH", new
 	code for it.  New variable relink_args.
 	(prelinkYES): Call prelinkNO.
--- src/gdb/testsuite/gdb.base/break-interp.exp	2010/07/05 17:57:50	1.14
+++ src/gdb/testsuite/gdb.base/break-interp.exp	2010/07/05 17:58:56	1.15
@@ -425,12 +425,24 @@
 	global exec interp_saved interp
 
 	foreach relink {YES NO} {
-	    if {[prelink$relink $relink_args [file tail $exec]]
-	        && [copy $interp_saved $interp]} {
+	    # Formerly this test was testing only prelinking of $EXEC.  As the
+	    # prelink command automatically prelinks all of $EXEC's libraries,
+	    # even $INTERP got prelinked.  Therefore, we formerly had to
+	    # `[copy $interp_saved $interp]' to make $INTERP not affected by
+	    # this prelinking of $EXEC.
+	    #
+	    # But now we need to test even prelinking of $INTERP.  We could
+	    # create a separate test to test just the $INTERP prelinking.  For
+	    # test simplicity, we merged this test and the test above by not
+	    # restoring $INTERP after $EXEC prelink.  $INTERP gets restored
+	    # later below.
+
+	    if [prelink$relink $relink_args [file tail $exec]] {
 		# /proc/PID/exe cannot be loaded as it is "EXECNAME (deleted)".
 		test_attach_gdb $exec $pid $displacement "attach-relink$relink"
 	    }
 	}
+	copy $interp_saved $interp
     }
 
     remote_exec host "kill -9 $pid"