Ping (Re: Patch : gdbserver get_image_name on CE)

Ping (Re: Patch : gdbserver get_image_name on CE)

[Danny Backx]

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

Ping again.

On Mon, 2009-07-27, Danny Backx wrote:
> Ping ?
> 
> On Wed, 2009-07-08 at 13:33 +0200, Danny Backx wrote:
> > On Wed, 2009-07-08 at 11:55 +0200, Danny Backx wrote:
> > > It looks like ReadProcessMemory refuses to read from an address
> beyond
> > > the pointer, but it does work from the pointer itself.
> > > 
> > > I've changed my code such that it loops, and tries to read a
> bigger
> > > chunk of memory at each iteration.
> > > 
> > > I'm not bothering you with that code or its debug output right
> now.
> > > 
> > > In that way, the code appears to work, and it complies with the
> > > documentation saying not to read more than required.
> > > 
> > > Shall I submit a new patch based on this ?
> > > 
> > >     Danny
> > 
> > Ignore that question, here's the patch.
> > 
> > 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.
> > 
> >       Danny
> > 
> > 2009-07-08  Danny Backx  <dannybackx@users.sourceforge.net>
> > 
> >       * win32-low.c (get_image_name) : Work around ReadProcessMemory
> >       failure when reading at arbitrary addresses.
-- 
Danny Backx ; danny.backx - at - scarlet.be ; http://danny.backx.info

[-- Attachment #2: x --]
[-- Type: text/x-patch, Size: 1629 bytes --]

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.
+   */
+  WCHAR *wbuf = alloca ((MAX_PATH + 1) * size);
+  while (1)
+    {
+      ReadProcessMemory (h, address_ptr, wbuf, ++len * size, &done);
+      if (wbuf[len - 1] == 0)
+        break;
+    }
 
   if (!unicode)
     ReadProcessMemory (h, address_ptr, buf, len, &done);
   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;
 }

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

[Pedro Alves]

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

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

[Danny Backx]

On Wed, 2009-08-12 at 15:11 +0100, Pedro Alves wrote:
> 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.

Summary on the "broken DLL" issue : in the gcc port, I wasn't emitting
the type info for the function symbols. This made binutils (dlltool)
behave in the wrong way, which in turn resulted in relocations that
didn't happen right.

Now that this is understood, I don't see how this can have influence on
the gdbserver/ReadProcessMemory issue. But before understanding this,
your question was certainly valid.

But in order to be more certain, here's output I created just now of an
unfixed gdbserver :

(gdb) info share
From        To          Syms Read   Shared Object Library
                        No          \Windows\iphlpapi.dll
                        No          \Windows\ws2.dll
                        No          \network\x86\ace\libgcc_s_sjlj-1.dll
                        No          \network\x86\ace\libstdc++-6.dll
                        No          li
                        No          \Windows\coredll.dll
(gdb) 

Here is output with my fix :

(gdb) info share
From        To          Syms Read   Shared Object Library
                        No          \Windows\iphlpapi.dll
                        No          \Windows\ws2.dll
                        No          \network\x86\ace\libgcc_s_sjlj-1.dll
                        No          \network\x86\ace\libstdc++-6.dll
0x41f21000  0x420d9744  Yes         libace.dll
                        No          \Windows\coredll.dll
(gdb) 

> Does this
> ReadProcessMemory problem still exist with a fixed dll?  Did you ever
> see this problem with another dll?

Yes, and yes. See above. The one that shows it in this output is not the
DLL that had that other problem (that was libstdc++-6.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.

I understand your point. But they're separate issues. I do have to admit
I don't know what's causing it.

I'll look into your code style comments and submit a new patch, unless
you want other information before I start going down that path.

	Danny
-- 
Danny Backx ; danny.backx - at - scarlet.be ; http://danny.backx.info

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

[Pedro Alves]

On Wednesday 12 August 2009 16:17:50, Danny Backx wrote:
> (gdb) info share
> >From        To          Syms Read   Shared Object Library
>                         No          \Windows\iphlpapi.dll
>                         No          \Windows\ws2.dll
>                         No          \network\x86\ace\libgcc_s_sjlj-1.dll
>                         No          \network\x86\ace\libstdc++-6.dll
> 0x41f21000  0x420d9744  Yes         libace.dll
>                         No          \Windows\coredll.dll
> (gdb) 
> 
> > Does this
> > ReadProcessMemory problem still exist with a fixed dll?  Did you ever
> > see this problem with another dll?

Well, both these dlls have considerable size, they're both written
in C++, and they were both generated from your toolchain.  Maybe that's
a coincidence, but maybe not.  I don't know how other
folks tend to catch these issues, but one thing I would do would be to
either remove things from the dll until I could reproduce it with
a *minimal* dll, or go the other way around, start with a minimal
dll that doesn't have  the problem, and add things until the
problem appears.  There's the other issue that CE isn't reporting
full paths for these dlls.  It doesn't look like a coincidence to me.

> Yes, and yes. See above. The one that shows it in this output is not the
> DLL that had that other problem (that was libstdc++-6.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.
> 
> I understand your point. But they're separate issues. I do have to admit
> I don't know what's causing it.

Don't you want to know what is?

> I'll look into your code style comments and submit a new patch, unless
> you want other information before I start going down that path.

I give up.  Let's follow this road then.

-- 
Pedro Alves