Re: ping: [patch 1/6] PIE: Attach binary even after re-prelinked underneath

[Joel Brobecker <brobecker at adacore dot com>]

> gdb/
> 2010-03-29  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Fix attaching to PIEs prelinked on the disk since their start.
> 	* solib-svr4.c (svr4_exec_displacement): New variable arch_size.
> 	Verify it against bfd_get_arch_size.  Try to match arbitrary
> 	displacement for the phdrs comparison.
> 
> gdb/testsuite/
> 2010-03-29  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* gdb.base/break-interp.exp: Run $binpie with new value "ATTACH", new
> 	code for it.  New variable relink_args.
> 	(prelinkYES): Call prelinkNO.
> 	(test_attach): Accept new parameter relink_args.  Re-prelink the binary
> 	in such case.  Move the core code to ...
> 	(test_attach_gdb): ... a new function.  Send GDB command "file".
> 	Extend expected "Attaching to " string.

OK with a few editorial changes: Instead of saying "*since* [process]
started", can you use "*after* the process was started". That would make
things a little clearer for me.  I'll just highlight areas where I think
the change should be made.

> +	  /* We are dealing with three different addresses.  EXEC_BFD
> +	     represents current address in on-disk file.  target memory content
> +	     may be different from EXEC_BFD as the file may have been prelinked
> +	     to a different address since the executable has been loaded.
                                    ^^^^^ after
> +	     Moreover the address of placement in target memory can be
> +	     different from what say the target memory program headers - this
                            what the program headers in target memory say
> +	     is the goal of PIE.

> +	     Detected DISPLACEMENT covers both the offsets of PIE placement and
> +	     possible new prelink since start of the program.  Here relocate
                                  ^^^^^ performed after (?)

> +	      /* DISPLACEMENT could be found easier by the difference of
                                             ^^^^^^ more easily
> +	         ehdr2->e_entry but already read BUF does not contain ehdr.  */

"already read BUF" is a bit terse and sounds like incomplete English
to me (I am not a specialist, though). Is the "already read" part the
important part? I think we need to explain why we use the more
complicated route. For instance, we could say something like this:

        /* DISPLACEMENT could be found easier by the difference of
           ehdr2->e_entry.  But we haven't read the ehdr yet, and we
           already have enough information to compute that displacement
           with what we've read.  */

> +		  /* PT_GNU_STACK addresses are left as zero not being
> +		     relocated by prelink, their displacing would create false
> +		     verification failure.  Feel free to test the unrelocated
> +		     comparison for any segment type.  */

Can you explain differently what you are try to say?

> -			    set displacement "ZERO"
> +			    # If the file has been randomly prelinked it must
> +			    # be "NONZERO".  We could see "ZERO" only if it was
> +			    # unprelinked na it is now running at the same
                                          ^^

> +			    # ATTACH executables + libraries get modified since
> +			    # they have been run.

I'm having problems understanding this sentence. Do you mean perhaps

        ATTACH means that executables and libraries have been modified
        after they have been run.

?

> +                           # they have been run.  They cannot be used for
> +                           # problem reproducibility after the testcase ends.

I would personally add a conclusion to the last sentence, explaining that
this is the reason why you are deleting all associated binary files.

And I use "reused" instead of "used", to make it clearer that the binaries
are saved in order to help reproduce issues found by this testcase.

-- 
Joel