Re: [PATCH v2] Rebase executable to match relocated base address

[Luis Machado <luis dot machado at linaro dot org>]

On 2/14/20 9:32 AM, Hannes Domani via gdb-patches wrote:
>   Am Freitag, 14. Februar 2020, 12:02:03 MEZ hat Luis Machado <luis.machado@linaro.org> Folgendes geschrieben:
>
> > Hi,
> >
> > On 2/13/20 3:14 PM, Hannes Domani via gdb-patches wrote:
> > > Windows executables linked with -dynamicbase get a new base address
> > > when loaded, which makes debugging impossible if the executable isn't
> > > also rebased in gdb.
> > >
> > > The new base address is read from the Process Environment Block.
> > > ---
> > > v2:
> > > This version now no longer needs the fake auxv entry.
> > > ---
> > >    gdb/windows-tdep.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++
> > >    1 file changed, 49 insertions(+)
> >
> > Thanks. This version looks better.
> >
> > > diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c
> > > index 6eef3fbd96..29c0a828a7 100644
> > > --- a/gdb/windows-tdep.c
> > > +++ b/gdb/windows-tdep.c
> > > @@ -34,6 +34,9 @@
> > >    #include "solib.h"
> > >    #include "solib-target.h"
> > >    #include "gdbcore.h"
> > > +#include "coff/internal.h"
> > > +#include "libcoff.h"
> > > +#include "solist.h"
> > >
> > >    /* Windows signal numbers differ between MinGW flavors and between
> > >        those and Cygwin.  The below enumeration was gleaned from the
> > > @@ -812,6 +815,50 @@ windows_get_siginfo_type (struct gdbarch *gdbarch)
> > >      return siginfo_type;
> > >    }
> > >
> > > +/* Implement the "solib_create_inferior_hook" target_so_ops method.  */
> > > +
> > > +static void
> > > +windows_solib_create_inferior_hook (int from_tty)
> > > +{
> > > +  CORE_ADDR exec_base = 0;
> > > +
> > > +  /* Find base address of main executable in
> > > +    TIB->process_environment_block->image_base_address.  */ > +  struct gdbarch *gdbarch = target_gdbarch ();
> > > +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> > > +  int ptr_bytes;
> > > +  int peb_offset;  /* Offset of process_environment_block in TIB.  */
> > > +  int base_offset; /* Offset of image_base_address in PEB.  */
> > > +  if (gdbarch_ptr_bit (gdbarch) == 32)
> > > +    {
> > > +      ptr_bytes = 4;
> > > +      peb_offset = 48;
> > > +      base_offset = 8;
> > > +    }
> > > +  else
> > > +    {
> > > +      ptr_bytes = 8;
> > > +      peb_offset = 96;
> > > +      base_offset = 16;
> > > +    }
> >
> > How about stashing the above offsets in windows_gdbarch_data, and then
> > using them here?
>
> To be honest, that would seem a bit weird for me, since they are just these
> simple numbers, and aren't used anywhere else.

Fair enough. I don't have a strong opinion on this, but i usually try to 
avoid having these magic numbers in the code without some pointers to 
where those came from. Folks dealing with this code in the future may 
try to understand what it is doing and how they came to be.

Having them at a single place, with some explanation, helps with that. 
That's my take on it, at least.

> > > +  CORE_ADDR tlb;
> > > +  gdb_byte buf[8];
> > > +  if (target_get_tib_address (inferior_ptid, &tlb)
> > > +      && !target_read_memory (tlb + peb_offset, buf, ptr_bytes))
> > > +    {
> > > +      CORE_ADDR peb = extract_unsigned_integer (buf, ptr_bytes, byte_order);
> > > +      if (!target_read_memory (peb + base_offset, buf, ptr_bytes))
> > > +    exec_base = extract_unsigned_integer (buf, ptr_bytes, byte_order);
> > > +    }
> > > +
> > > +  if (symfile_objfile && exec_base)
> > > +    {
> > > +      CORE_ADDR vmaddr = pe_data (exec_bfd)->pe_opthdr.ImageBase;
> > > +      if (vmaddr != exec_base)
> > > +    objfile_rebase (symfile_objfile, exec_base - vmaddr);
> >
> > > +    }
> >
> > I'd add a comment to the above conditional block on why we're doing this
> > relocation now, if you think it is worth mentioning.
> >
> > It seems to me the behavior has changed now, hence why it seems
> > worthwhile adding some information.
>
> Something like?:
> /* Rebase executable if the base address changed because of ASLR.  */

Sounds good to me.