Re: Ping (Re: Patch : gdbserver get_image_name on CE)

[Pedro Alves <pedro@codesourcery.com>]

On Tuesday 11 August 2009 21:05:02, Danny Backx wrote:

> > > Please comment on its contents and on my adherence to the coding
> > > standards. Apologies for that, I seem to have a hard time learning
> > that.

See below.

> > > 2009-07-08  Danny Backx  <dannybackx@users.sourceforge.net>
> > > 
> > >       * win32-low.c (get_image_name) : Work around ReadProcessMemory

                                          ^
No space between ) and : please.

> > >       failure when reading at arbitrary addresses.


> Index: win32-low.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/win32-low.c,v
> retrieving revision 1.38
> diff -u -r1.38 win32-low.c
> --- win32-low.c 4 Jul 2009 18:13:28 -0000       1.38
> +++ win32-low.c 11 Aug 2009 20:01:19 -0000
> @@ -922,7 +922,6 @@
>    DWORD size = unicode ? sizeof (WCHAR) : sizeof (char);
>    char *address_ptr;
>    int len = 0;
> -  char b[2];
>    DWORD done;
>  
>    /* Attempt to read the name of the dll that was detected.
> @@ -945,21 +944,23 @@
>      return NULL;
>  #endif
>  
> -  /* Find the length of the string */
> -  while (ReadProcessMemory (h, address_ptr + len++ * size, &b, size, &done)
> -        && (b[0] != 0 || b[size - 1] != 0) && done == size)
> -    continue;
> +  /* ReadProcessMemory sometimes fails when reading a (w)char at a time, but
> +   * we can't just read MAX_PATH (w)chars either : msdn says not to cross the
> +   * boundary into inaccessible areas.
> +   * So we loop, reading more characters each time, until we find the NULL.
> +   */

Format comments as:

  /* ReadProcessMemory sometimes fails when reading a (w)char at a
     time, but we can't just read MAX_PATH (w)chars either: MSDN says
     not to cross the boundary into inaccessible areas.  So we loop,
     reading more characters each time, until we find the NULL.  */

 - no '*' at the beginning of each comment line.
 - period and double space between sentences.
 - might as well uppercase msdn.

In any case, the comment doesn't really explain the problem, and would make
more sense when looking at the old code than at the post-patch code.  From
what I could tell so far, the problem is not with the size of how much
we read each time, but with starting the read from any address other
than address_ptr.  The comment should mention that instead.

BTW, did you ever try to read the dll name from GDB
using 'x' 'print', etc. ?  If you play with the arguments to 'x', e.g.,

  (gdb) x /10b $address_ptr

you should see the same problem triggering.  (this is one of
the reasons I'm relunctant to working around an issue I'm
not sure is understood fully).

> +  WCHAR *wbuf = alloca ((MAX_PATH + 1) * size);

Declarations go at the beggining of a block/function.

> +  while (1)
> +    {
> +      ReadProcessMemory (h, address_ptr, wbuf, ++len * size, &done);
> +      if (wbuf[len - 1] == 0)
> +        break;
> +    }
>  

This is busted for the non unicode case.  This doesn't check for the possibility
of overflowing wbuf.  The old code handled both cases.  Given that this
now reads the whole string, why not read directly into buf, instead of
alloca'ing a big buffer, and then you wouldn't need...

>    if (!unicode)
>      ReadProcessMemory (h, address_ptr, buf, len, &done);

... this?

>    else
> -    {
> -      WCHAR *unicode_address = (WCHAR *) alloca (len * sizeof (WCHAR));
> -      ReadProcessMemory (h, address_ptr, unicode_address, len * sizeof (WCHAR),
> -                        &done);
> -
> -      WideCharToMultiByte (CP_ACP, 0, unicode_address, len, buf, len, 0, 0);
> -    }
> +    WideCharToMultiByte (CP_ACP, 0, wbuf, len, buf, len, 0, 0);
>  
>    return buf;
>  }

Sorry, I'm quite behind on reading cegcc-devel@.  Do you now have a
fully working non-broken libstdc++ dll (the dll that triggered this
problem)?  IIUC from the binutils posts, the runtime loader was ending
up doing things it shouldn't do to the dll image in memory.  Does this
ReadProcessMemory problem still exist with a fixed dll?  Did you ever
see this problem with another dll?  I'm very relunctant to this
fix without understanding it fully, and without having seen it
trigger with a sane dll, and not really understanding why some dlls
cause it and other don't, even though they're apparently built
the exact same way --- not because I'm hard headed, but because
these workarounds tend to mask other real problems.  From what
I've heard so far, this only triggered on that broken dll.  I wonder if
anyone has tried loading an application with a dll that triggers
this into MSFT's debugger, and see if it reads the dll name correctly.

-- 
Pedro Alves