[PATCH 0/3] Stackdump improvements

[PATCH 0/3] Stackdump improvements

[Jon Turney]

Jon Turney (3):
  Cygwin: Tidy up formatting of stackdump
  Cygwin: Add addresses as module offsets in .stackdump file
  Cygwin: Add loaded module base address list to stackdump

 winsup/cygwin/exceptions.cc | 46 +++++++++++++++++++++++++++++++++----
 1 file changed, 42 insertions(+), 4 deletions(-)

-- 
2.38.1

[PATCH 1/3] Cygwin: Tidy up formatting of stackdump

[Jon Turney]

Resize stackdump headers for b9e97f58
Consistently use \r\n line endings
---
 winsup/cygwin/exceptions.cc | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
index e6f868511..a15bc16c5 100644
--- a/winsup/cygwin/exceptions.cc
+++ b/winsup/cygwin/exceptions.cc
@@ -342,7 +342,7 @@ cygwin_exception::dumpstack ()
       int i;
 
       thestack.init (framep, 1, ctx);	/* Initialize from the input CONTEXT */
-      small_printf ("Stack trace:\r\nFrame        Function    Args\r\n");
+      small_printf ("Stack trace:\r\nFrame         Function      Args\r\n");
       for (i = 0; i < DUMPSTACK_FRAME_LIMIT && thestack++; i++)
 	{
 	  small_printf ("%012X  %012X", thestack.sf.AddrFrame.Offset,
@@ -352,9 +352,9 @@ cygwin_exception::dumpstack ()
 			  thestack.sf.Params[j]);
 	  small_printf (")\r\n");
 	}
-      small_printf ("End of stack trace%s\n",
+      small_printf ("End of stack trace%s\r\n",
 		    i == DUMPSTACK_FRAME_LIMIT ?
-		        " (more stack frames may be present)" : "");
+		    " (more stack frames may be present)" : "");
       if (h)
 	NtClose (h);
     }
-- 
2.38.1

[PATCH 2/3] Cygwin: Add addresses as module offsets in .stackdump file

[Jon Turney]

This adds an additional column to the stack trace in a .stackdump file,
which gives the stack frame return address as a module name+offset. This
makes it a possible to convert the address to a function name without
having to guess what module the address belongs to.

> Stack trace:
> Frame         Function     Args
> 0007FFFFCC30  0001004010E9 (000180048055, 000180046FA0, 000000000002, 00018031E160) segv-test.exe+0x10E9
> 0007FFFFCD30  0001800480C1 (000000000000, 000000000000, 000000000000, 000000000000) cygwin1.dll+0x80C1
> 0007FFFFFFF0  000180045C86 (000000000000, 000000000000, 000000000000, 000000000000) cygwin1.dll+0x5C86
> 0007FFFFFFF0  000180045D34 (000000000000, 000000000000, 000000000000, 000000000000) cygwin1.dll+0x5D34
> End of stack trace

Loosely based on this patch [1] by Brian Dessent.

[1] https://cygwin.com/pipermail/cygwin-patches/2008q1/006306.html
---
 winsup/cygwin/exceptions.cc | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
index a15bc16c5..1e9ea26bf 100644
--- a/winsup/cygwin/exceptions.cc
+++ b/winsup/cygwin/exceptions.cc
@@ -324,6 +324,34 @@ stack_info::walk ()
   return 1;
 }
 
+/*
+  Walk the list of modules in the current process to find the one containing
+   'func_va'.
+
+   This implementation requires no allocation of memory and minimal system
+   calls, so it should be safe in the context of an exception handler.
+*/
+static char *
+prettyprint_va (PVOID func_va)
+{
+  static char buf[256];
+  buf[0] = '\0';
+
+  PLIST_ENTRY head = &NtCurrentTeb()->Peb->Ldr->InMemoryOrderModuleList;
+  for (PLIST_ENTRY x = head->Flink; x != head; x = x->Flink)
+    {
+      PLDR_DATA_TABLE_ENTRY mod = CONTAINING_RECORD (x, LDR_DATA_TABLE_ENTRY,
+						     InMemoryOrderLinks);
+      if (mod->DllBase > func_va)
+	continue;
+
+      __small_sprintf (buf, "%S+0x%x", &mod->BaseDllName,
+		       (DWORD_PTR)func_va - (DWORD_PTR)mod->DllBase);
+    }
+
+  return buf;
+}
+
 void
 cygwin_exception::dumpstack ()
 {
@@ -350,7 +378,7 @@ cygwin_exception::dumpstack ()
 	  for (unsigned j = 0; j < NPARAMS; j++)
 	    small_printf ("%s%012X", j == 0 ? " (" : ", ",
 			  thestack.sf.Params[j]);
-	  small_printf (")\r\n");
+	  small_printf (") %s\r\n", prettyprint_va((PVOID)thestack.sf.AddrPC.Offset));
 	}
       small_printf ("End of stack trace%s\r\n",
 		    i == DUMPSTACK_FRAME_LIMIT ?
-- 
2.38.1

[PATCH 3/3] Cygwin: Add loaded module base address list to stackdump

[Jon Turney]

This adds an extra section to the stackdump, which lists the loaded
modules and their base address.  This is perhaps useful as it makes it
immediately clear if RandomCrashInjectedDll.dll is loaded...

XXX: It seems like the 'InMemoryOrder' part of 'InMemoryOrderModuleList' is a lie?

> Loaded modules
> 000100400000 segv-test.exe
> 7FFF2AC30000 ntdll.dll
> 7FFF29050000 KERNEL32.DLL
> 7FFF28800000 KERNELBASE.dll
> 000180040000 cygwin1.dll
> 7FFF28FA0000 advapi32.dll
> 7FFF29F20000 msvcrt.dll
> 7FFF299E0000 sechost.dll
> 7FFF29B30000 RPCRT4.dll
> 7FFF27C10000 CRYPTBASE.DLL
> 7FFF28770000 bcryptPrimitives.dll
---
 winsup/cygwin/exceptions.cc | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
index 1e9ea26bf..7dde44140 100644
--- a/winsup/cygwin/exceptions.cc
+++ b/winsup/cygwin/exceptions.cc
@@ -383,6 +383,16 @@ cygwin_exception::dumpstack ()
       small_printf ("End of stack trace%s\r\n",
 		    i == DUMPSTACK_FRAME_LIMIT ?
 		    " (more stack frames may be present)" : "");
+
+      small_printf ("Loaded modules\r\n");
+      PLIST_ENTRY head = &NtCurrentTeb()->Peb->Ldr->InMemoryOrderModuleList;
+      for (PLIST_ENTRY x = head->Flink; x != head; x = x->Flink)
+	{
+	  PLDR_DATA_TABLE_ENTRY mod = CONTAINING_RECORD (x, LDR_DATA_TABLE_ENTRY,
+							 InMemoryOrderLinks);
+	  small_printf ("%012X %S\r\n", mod->DllBase, &mod->BaseDllName);
+	}
+
       if (h)
 	NtClose (h);
     }
-- 
2.38.1

Re: [PATCH 3/3] Cygwin: Add loaded module base address list to stackdump

[Corinna Vinschen]

On Oct 28 16:05, Jon Turney wrote:
> This adds an extra section to the stackdump, which lists the loaded
> modules and their base address.  This is perhaps useful as it makes it
> immediately clear if RandomCrashInjectedDll.dll is loaded...
> 
> XXX: It seems like the 'InMemoryOrder' part of 'InMemoryOrderModuleList' is a lie?

Probably just an alternative fact...

> 
> > Loaded modules
> > 000100400000 segv-test.exe
> > 7FFF2AC30000 ntdll.dll
> > 7FFF29050000 KERNEL32.DLL
> > 7FFF28800000 KERNELBASE.dll
> > 000180040000 cygwin1.dll
> > 7FFF28FA0000 advapi32.dll
> > 7FFF29F20000 msvcrt.dll
> > 7FFF299E0000 sechost.dll
> > 7FFF29B30000 RPCRT4.dll
> > 7FFF27C10000 CRYPTBASE.DLL
> > 7FFF28770000 bcryptPrimitives.dll
> ---
>  winsup/cygwin/exceptions.cc | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
> index 1e9ea26bf..7dde44140 100644
> --- a/winsup/cygwin/exceptions.cc
> +++ b/winsup/cygwin/exceptions.cc
> @@ -383,6 +383,16 @@ cygwin_exception::dumpstack ()
>        small_printf ("End of stack trace%s\r\n",
>  		    i == DUMPSTACK_FRAME_LIMIT ?
>  		    " (more stack frames may be present)" : "");
> +
> +      small_printf ("Loaded modules\r\n");
> +      PLIST_ENTRY head = &NtCurrentTeb()->Peb->Ldr->InMemoryOrderModuleList;
> +      for (PLIST_ENTRY x = head->Flink; x != head; x = x->Flink)
> +	{
> +	  PLDR_DATA_TABLE_ENTRY mod = CONTAINING_RECORD (x, LDR_DATA_TABLE_ENTRY,
> +							 InMemoryOrderLinks);
> +	  small_printf ("%012X %S\r\n", mod->DllBase, &mod->BaseDllName);
> +	}
> +
>        if (h)
>  	NtClose (h);
>      }
> -- 
> 2.38.1

Re: [PATCH 3/3] Cygwin: Add loaded module base address list to stackdump

[Jon Turney]

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

On 29/10/2022 09:32, Corinna Vinschen wrote:
> On Oct 28 16:05, Jon Turney wrote:
>> This adds an extra section to the stackdump, which lists the loaded
>> modules and their base address.  This is perhaps useful as it makes it
>> immediately clear if RandomCrashInjectedDll.dll is loaded...
>>
>> XXX: It seems like the 'InMemoryOrder' part of 'InMemoryOrderModuleList' is a lie?
> 
> Probably just an alternative fact...

Yeah.  I did stared a bit at the code wondering if the structure layouts 
were incorrect so we were somehow traversing one of the other module 
lists with a different ordering, but everything looks correct.

The attached might be a good idea, then, to ensure that module+offset is 
calculated correctly.

[-- Attachment #2: 0001-Cygwin-Handle-out-of-order-modules-for-module-offset.patch --]
[-- Type: text/plain, Size: 1150 bytes --]

From ea47826047e8bb175b1b0e0286d7d7b8cf15c7fe Mon Sep 17 00:00:00 2001
From: Jon Turney <jon.turney@dronecode.org.uk>
Date: Tue, 1 Nov 2022 14:01:08 +0000
Subject: [PATCH] Cygwin: Handle out of order modules for module offsets in
 stackdump

Improve address to module+offset conversion, to work correctly in the
presence of out-of-order elements in InMemoryOrderModuleList.

Fixes: d59651d4
---
 winsup/cygwin/exceptions.cc | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
index 8cc454c90..c3433ab94 100644
--- a/winsup/cygwin/exceptions.cc
+++ b/winsup/cygwin/exceptions.cc
@@ -342,11 +342,13 @@ prettyprint_va (PVOID func_va)
     {
       PLDR_DATA_TABLE_ENTRY mod = CONTAINING_RECORD (x, LDR_DATA_TABLE_ENTRY,
 						     InMemoryOrderLinks);
-      if (mod->DllBase > func_va)
+      if ((func_va < mod->DllBase) ||
+	  (func_va > (PVOID)((DWORD_PTR)mod->DllBase + mod->SizeOfImage)))
 	continue;
 
       __small_sprintf (buf, "%S+0x%x", &mod->BaseDllName,
 		       (DWORD_PTR)func_va - (DWORD_PTR)mod->DllBase);
+      break;
     }
 
   return buf;
-- 
2.38.1

Re: [PATCH 3/3] Cygwin: Add loaded module base address list to stackdump

[Corinna Vinschen]

On Nov  3 17:02, Jon Turney wrote:
> On 29/10/2022 09:32, Corinna Vinschen wrote:
> > On Oct 28 16:05, Jon Turney wrote:
> > > This adds an extra section to the stackdump, which lists the loaded
> > > modules and their base address.  This is perhaps useful as it makes it
> > > immediately clear if RandomCrashInjectedDll.dll is loaded...
> > > 
> > > XXX: It seems like the 'InMemoryOrder' part of 'InMemoryOrderModuleList' is a lie?
> > 
> > Probably just an alternative fact...
> 
> Yeah.  I did stared a bit at the code wondering if the structure layouts
> were incorrect so we were somehow traversing one of the other module lists
> with a different ordering, but everything looks correct.
> 
> The attached might be a good idea, then, to ensure that module+offset is
> calculated correctly.

Good idea, please push.


Corinna

> From ea47826047e8bb175b1b0e0286d7d7b8cf15c7fe Mon Sep 17 00:00:00 2001
> From: Jon Turney <jon.turney@dronecode.org.uk>
> Date: Tue, 1 Nov 2022 14:01:08 +0000
> Subject: [PATCH] Cygwin: Handle out of order modules for module offsets in
>  stackdump
> 
> Improve address to module+offset conversion, to work correctly in the
> presence of out-of-order elements in InMemoryOrderModuleList.
> 
> Fixes: d59651d4
> ---
>  winsup/cygwin/exceptions.cc | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
> index 8cc454c90..c3433ab94 100644
> --- a/winsup/cygwin/exceptions.cc
> +++ b/winsup/cygwin/exceptions.cc
> @@ -342,11 +342,13 @@ prettyprint_va (PVOID func_va)
>      {
>        PLDR_DATA_TABLE_ENTRY mod = CONTAINING_RECORD (x, LDR_DATA_TABLE_ENTRY,
>  						     InMemoryOrderLinks);
> -      if (mod->DllBase > func_va)
> +      if ((func_va < mod->DllBase) ||
> +	  (func_va > (PVOID)((DWORD_PTR)mod->DllBase + mod->SizeOfImage)))
>  	continue;
>  
>        __small_sprintf (buf, "%S+0x%x", &mod->BaseDllName,
>  		       (DWORD_PTR)func_va - (DWORD_PTR)mod->DllBase);
> +      break;
>      }
>  
>    return buf;
> -- 
> 2.38.1
> 

Re: [PATCH 0/3] Stackdump improvements

[Corinna Vinschen]

On Oct 28 16:05, Jon Turney wrote:
> Jon Turney (3):
>   Cygwin: Tidy up formatting of stackdump
>   Cygwin: Add addresses as module offsets in .stackdump file
>   Cygwin: Add loaded module base address list to stackdump
> 
>  winsup/cygwin/exceptions.cc | 46 +++++++++++++++++++++++++++++++++----
>  1 file changed, 42 insertions(+), 4 deletions(-)

Looks really good to me and will be quite helpful as soon as
ASLR is used.  Please push.


Thanks,
Corinna