[PATCH] Fix ctrl-c when debugging WOW64 processes

[PATCH] Fix ctrl-c when debugging WOW64 processes

[Hannes Domani]

DebugBreakProcess starts a new thread in the target process with the
entry point DbgUiRemoteBreakin, where an int3 triggers a breakpoint
exception for gdb.

But this uses DbgUiRemoteBreakin of the 64bit ntdll.dll even for
WOW64 processes.
It stops in 64bit code, Wow64GetThreadContext reports a wrong pc without
the int3, and gdb lets the target process continue.

So this uses DbgUiRemoteBreakin of the 32bit ntdll.dll as the thread
entry point for WOW64 processes instead.

gdb/ChangeLog:

2020-09-17  Hannes Domani  <ssbssa@yahoo.de>

	* windows-nat.c (ctrl_c_handler): Use 32bit DbgUiRemoteBreakin
	for WOW64 processes.
---
 gdb/windows-nat.c | 35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 188a920cbb..284c529b9c 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -71,6 +71,7 @@
 #include "gdbsupport/pathstuff.h"
 #include "gdbsupport/gdb_wait.h"
 #include "nat/windows-nat.h"
+#include "gdbsupport/symbol.h"
 
 using namespace windows_nat;
 
@@ -235,6 +236,7 @@ static int saw_create;
 static int open_process_used = 0;
 #ifdef __x86_64__
 static bool wow64_process = false;
+static void *wow64_dbgbreak;
 #endif
 
 /* User options.  */
@@ -1522,9 +1524,36 @@ ctrl_c_handler (DWORD event_type)
   if (!new_console && !attach_flag)
     return TRUE;
 
-  if (!DebugBreakProcess (current_process_handle))
-    warning (_("Could not interrupt program.  "
-	       "Press Ctrl-c in the program console."));
+#ifdef __x86_64__
+  if (wow64_process)
+    {
+      /* Call DbgUiRemoteBreakin of the 32bit ntdll.dll in the target process.
+	 DebugBreakProcess would call the one of the 64bit ntdll.dll, which
+	 can't be correctly handled by gdb.  */
+      if (!wow64_dbgbreak)
+	{
+	  CORE_ADDR addr;
+	  if (!find_minimal_symbol_address ("ntdll!DbgUiRemoteBreakin",
+					    &addr, 0))
+	    wow64_dbgbreak = (void *) addr;
+	}
+
+      if (wow64_dbgbreak)
+	{
+	  HANDLE thread = CreateRemoteThread (current_process_handle, NULL,
+					      0, (LPTHREAD_START_ROUTINE)
+					      wow64_dbgbreak, NULL, 0, NULL);
+	  if (thread)
+	    CloseHandle (thread);
+	}
+    }
+  else
+#endif
+    {
+      if (!DebugBreakProcess (current_process_handle))
+	warning (_("Could not interrupt program.  "
+		   "Press Ctrl-c in the program console."));
+    }
 
   /* Return true to tell that Ctrl-C has been handled.  */
   return TRUE;
-- 
2.27.0

Re: [PATCH] Fix ctrl-c when debugging WOW64 processes

[Simon Marchi]

On 2020-09-17 2:03 p.m., Hannes Domani via Gdb-patches wrote:
> DebugBreakProcess starts a new thread in the target process with the
> entry point DbgUiRemoteBreakin, where an int3 triggers a breakpoint
> exception for gdb.
>
> But this uses DbgUiRemoteBreakin of the 64bit ntdll.dll even for
> WOW64 processes.
> It stops in 64bit code, Wow64GetThreadContext reports a wrong pc without
> the int3, and gdb lets the target process continue.
>
> So this uses DbgUiRemoteBreakin of the 32bit ntdll.dll as the thread
> entry point for WOW64 processes instead.

Interesting.  Is there any reference somewhere that explains that things
must be done this way when a 64 bit process is debugging a 32 bit
process?

In any case, the patch is OK, with the formatting nits below fixed.

> @@ -1522,9 +1524,36 @@ ctrl_c_handler (DWORD event_type)
>    if (!new_console && !attach_flag)
>      return TRUE;
>
> -  if (!DebugBreakProcess (current_process_handle))
> -    warning (_("Could not interrupt program.  "
> -	       "Press Ctrl-c in the program console."));
> +#ifdef __x86_64__
> +  if (wow64_process)
> +    {
> +      /* Call DbgUiRemoteBreakin of the 32bit ntdll.dll in the target process.
> +	 DebugBreakProcess would call the one of the 64bit ntdll.dll, which
> +	 can't be correctly handled by gdb.  */
> +      if (!wow64_dbgbreak)

wow64_dbgbreak == nullptr

> +	{
> +	  CORE_ADDR addr;
> +	  if (!find_minimal_symbol_address ("ntdll!DbgUiRemoteBreakin",
> +					    &addr, 0))
> +	    wow64_dbgbreak = (void *) addr;
> +	}
> +
> +      if (wow64_dbgbreak)

wow64_dbgbreak != nullptr

Simon

Re: [PATCH] Fix ctrl-c when debugging WOW64 processes

[Hannes Domani]

 Am Donnerstag, 17. September 2020, 22:00:29 MESZ hat Simon Marchi <simark@simark.ca> Folgendes geschrieben:

> On 2020-09-17 2:03 p.m., Hannes Domani via Gdb-patches wrote:
> > DebugBreakProcess starts a new thread in the target process with the
> > entry point DbgUiRemoteBreakin, where an int3 triggers a breakpoint
> > exception for gdb.
> >
> > But this uses DbgUiRemoteBreakin of the 64bit ntdll.dll even for
> > WOW64 processes.
> > It stops in 64bit code, Wow64GetThreadContext reports a wrong pc without
> > the int3, and gdb lets the target process continue.
> >
> > So this uses DbgUiRemoteBreakin of the 32bit ntdll.dll as the thread
> > entry point for WOW64 processes instead.
>
> Interesting.  Is there any reference somewhere that explains that things
> must be done this way when a 64 bit process is debugging a 32 bit
> process?

Sadly, I could find no real reference for this.
It took me quite a few hours to debug/understand this problem.


> In any case, the patch is OK, with the formatting nits below fixed.
>
> > @@ -1522,9 +1524,36 @@ ctrl_c_handler (DWORD event_type)
> >    if (!new_console && !attach_flag)
> >      return TRUE;
> >
> > -  if (!DebugBreakProcess (current_process_handle))
> > -    warning (_("Could not interrupt program.  "
> > -          "Press Ctrl-c in the program console."));
> > +#ifdef __x86_64__
> > +  if (wow64_process)
> > +    {
> > +      /* Call DbgUiRemoteBreakin of the 32bit ntdll.dll in the target process.
> > +    DebugBreakProcess would call the one of the 64bit ntdll.dll, which
> > +    can't be correctly handled by gdb.  */
> > +      if (!wow64_dbgbreak)
>
> wow64_dbgbreak == nullptr
>
>
> > +    {
> > +      CORE_ADDR addr;
> > +      if (!find_minimal_symbol_address ("ntdll!DbgUiRemoteBreakin",
> > +                        &addr, 0))
> > +        wow64_dbgbreak = (void *) addr;
> > +    }
> > +
> > +      if (wow64_dbgbreak)
>
>
> wow64_dbgbreak != nullptr

Pushed with these changes, thanks.


Hannes

Re: [PATCH] Fix ctrl-c when debugging WOW64 processes

[Tom Tromey]

>>>>> "Hannes" == Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> writes:

Hannes> 2020-09-17  Hannes Domani  <ssbssa@yahoo.de>

Hannes> 	* windows-nat.c (ctrl_c_handler): Use 32bit DbgUiRemoteBreakin
Hannes> 	for WOW64 processes.

Does gdbserver need this treatment as well?
If so maybe this code can be shared.

Tom

Re: [PATCH] Fix ctrl-c when debugging WOW64 processes

[Hannes Domani]

 Am Donnerstag, 17. September 2020, 22:18:01 MESZ hat Tom Tromey <tom@tromey.com> Folgendes geschrieben:

> >>>>> "Hannes" == Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Hannes> 2020-09-17  Hannes Domani  <ssbssa@yahoo.de>
>
> Hannes>     * windows-nat.c (ctrl_c_handler): Use 32bit DbgUiRemoteBreakin
> Hannes>     for WOW64 processes.
>
> Does gdbserver need this treatment as well?
> If so maybe this code can be shared.

I thought so, but I just tried it and it works without it.

The reason seems to be that gdbserver first calls GenerateConsoleCtrlEvent(),
and only when that fails falls back to DebugBreakProcess().

When I tried to do the same in gdb just now, it didn't work.
Weird, I will have to investigate this some more.


Hannes

Re: [PATCH] Fix ctrl-c when debugging WOW64 processes

[Hannes Domani]

 Am Donnerstag, 17. September 2020, 22:56:45 MESZ hat Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> Folgendes geschrieben:

> Am Donnerstag, 17. September 2020, 22:18:01 MESZ hat Tom Tromey <tom@tromey.com> Folgendes geschrieben:
>
> > >>>>> "Hannes" == Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> writes:
> >
> > Hannes> 2020-09-17  Hannes Domani  <ssbssa@yahoo.de>
> >
> > Hannes>     * windows-nat.c (ctrl_c_handler): Use 32bit DbgUiRemoteBreakin
> > Hannes>     for WOW64 processes.
> >
> > Does gdbserver need this treatment as well?
> > If so maybe this code can be shared.
>
> I thought so, but I just tried it and it works without it.
>
> The reason seems to be that gdbserver first calls GenerateConsoleCtrlEvent(),
> and only when that fails falls back to DebugBreakProcess().
>
> When I tried to do the same in gdb just now, it didn't work.
> Weird, I will have to investigate this some more.

I did some experiments.

Seems like GenerateConsoleCtrlEvent() only works if the target process was
created without CREATE_NEW_CONSOLE.
And what's worse, it seems to always return TRUE, so it never reaches
DebugBreakProcess(), even if the target process doesn't have a console at all.

I also tried the "last resort", with soft_interrupt_requested=1, but this
immediatly crashed gdbserver.

I fixed this crash with:

--- a/gdbserver/win32-low.cc
+++ b/gdbserver/win32-low.cc
@@ -1339,6 +1335,7 @@ fake_breakpoint_event (void)
   faked_breakpoint = 1;

   memset (&current_event, 0, sizeof (current_event));
+  current_event.dwProcessId = current_process_id;
   current_event.dwThreadId = main_thread_id;
   current_event.dwDebugEventCode = EXCEPTION_DEBUG_EVENT;
   current_event.u.Exception.ExceptionRecord.ExceptionCode

But this didn't work either, I think it's because gdb checks if the current
instruction is int3, and continues if not.

I'm wondering why this last resort was added, was this done for pre-XP where
DebugBreakProcess() didn't exist?

So right now ctrl-c doesn't really work that well with gdbserver, and not
at all when debugging a program without console (WOW64 or not doesn't matter).


And the WOW64 fix for DbgUiRemoteBreakin() probably can't be used in
gdbserver, because find_minimal_symbol_address() is not available.


Meanwhile I came up with a possible alternative for DbgUiRemoteBreakin():

--- a/gdb/nat/windows-nat.c
+++ b/gdb/nat/windows-nat.c
@@ -240,6 +241,13 @@ handle_exception (struct target_waitstatus *ourstatus, bool debug_exceptions)
     case EXCEPTION_BREAKPOINT:
 #ifdef __x86_64__
       if (ignore_first_breakpoint)
     {
       /* For WOW64 processes, there are always 2 breakpoint exceptions
          on startup, first a BREAKPOINT for the 64bit ntdll.dll,
          then a WX86_BREAKPOINT for the 32bit ntdll.dll.
          Here we only care about the WX86_BREAKPOINT's.  */
           ourstatus->kind = TARGET_WAITKIND_SPURIOUS;
          ignore_first_breakpoint = false;
        }
+      else if (wow64_process)
+       {
+         DEBUG_EXCEPTION_SIMPLE ("EXCEPTION_BREAKPOINT");
+         rec->ExceptionCode = DBG_CONTROL_C;
+         ourstatus->value.sig = GDB_SIGNAL_INT;
+         break;
+       }
 #endif
       /* FALLTHROUGH */
     case STATUS_WX86_BREAKPOINT:

This transforms the (64bit) breakpoint from DebugBreakProcess() into
SIGINT, then the check for the int3 instruction is not done, and gdb stops
the target process in any case.

So far this seems to work fine for both gdb and gdbserver.


Hannes

Re: [PATCH] Fix ctrl-c when debugging WOW64 processes

[Simon Marchi]

On 2020-09-18 10:27 a.m., Hannes Domani wrote:
> I did some experiments.
>
> Seems like GenerateConsoleCtrlEvent() only works if the target process was
> created without CREATE_NEW_CONSOLE.
> And what's worse, it seems to always return TRUE, so it never reaches
> DebugBreakProcess(), even if the target process doesn't have a console at all.
>
> I also tried the "last resort", with soft_interrupt_requested=1, but this
> immediatly crashed gdbserver.
>
> I fixed this crash with:
>
> --- a/gdbserver/win32-low.cc
> +++ b/gdbserver/win32-low.cc
> @@ -1339,6 +1335,7 @@ fake_breakpoint_event (void)
>    faked_breakpoint = 1;
>
>    memset (&current_event, 0, sizeof (current_event));
> +  current_event.dwProcessId = current_process_id;
>    current_event.dwThreadId = main_thread_id;
>    current_event.dwDebugEventCode = EXCEPTION_DEBUG_EVENT;
>    current_event.u.Exception.ExceptionRecord.ExceptionCode
>
> But this didn't work either, I think it's because gdb checks if the current
> instruction is int3, and continues if not.
>
> I'm wondering why this last resort was added, was this done for pre-XP where
> DebugBreakProcess() didn't exist?

I digged a little bit.  It was introduced by this commit by a young
Pedro :)

  https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=4d5d1aaa19ea

The corresponding mailing list post is:

  https://sourceware.org/legacy-ml/gdb-patches/2007-11/msg00217.html

It doesn't explain the rationale though, it just implicitly refers to a
previous thread.  It's probably this one, which gives a bit more
context:

  https://sourceware.org/legacy-ml/gdb-patches/2007-11/msg00035.html

From what I understand, this was for WinCE, which lacked
GenerateConsoleCtrlEvent and DebugBreakProcess.  Since we removed
support for WinCE, I think that code could very well be GC'ed.

>
> So right now ctrl-c doesn't really work that well with gdbserver, and not
> at all when debugging a program without console (WOW64 or not doesn't matter).
>
>
> And the WOW64 fix for DbgUiRemoteBreakin() probably can't be used in
> gdbserver, because find_minimal_symbol_address() is not available.

Using the qSymbol packet, there is a window during which GDBserver can
as GDB to look up minimal symbol values.  On the GDBserver side, this is
done through process_stratum_target::look_up_symbols, you would just
need to implement it for win32_process_target.

> Meanwhile I came up with a possible alternative for DbgUiRemoteBreakin():
>
> --- a/gdb/nat/windows-nat.c
> +++ b/gdb/nat/windows-nat.c
> @@ -240,6 +241,13 @@ handle_exception (struct target_waitstatus *ourstatus, bool debug_exceptions)
>      case EXCEPTION_BREAKPOINT:
>  #ifdef __x86_64__
>        if (ignore_first_breakpoint)
>      {
>        /* For WOW64 processes, there are always 2 breakpoint exceptions
>           on startup, first a BREAKPOINT for the 64bit ntdll.dll,
>           then a WX86_BREAKPOINT for the 32bit ntdll.dll.
>           Here we only care about the WX86_BREAKPOINT's.  */
>            ourstatus->kind = TARGET_WAITKIND_SPURIOUS;
>           ignore_first_breakpoint = false;
>         }
> +      else if (wow64_process)
> +       {
> +         DEBUG_EXCEPTION_SIMPLE ("EXCEPTION_BREAKPOINT");
> +         rec->ExceptionCode = DBG_CONTROL_C;
> +         ourstatus->value.sig = GDB_SIGNAL_INT;
> +         break;
> +       }
>  #endif
>        /* FALLTHROUGH */
>      case STATUS_WX86_BREAKPOINT:
>
> This transforms the (64bit) breakpoint from DebugBreakProcess() into
> SIGINT, then the check for the int3 instruction is not done, and gdb stops
> the target process in any case.

That sounds simple (which is good).  Would this replace completely
spawning the remote thread (which you added in the previous patch)?

Simon

Re: [PATCH] Fix ctrl-c when debugging WOW64 processes

[Hannes Domani]

 Am Freitag, 18. September 2020, 17:48:58 MESZ hat Simon Marchi <simark@simark.ca> Folgendes geschrieben:

> On 2020-09-18 10:27 a.m., Hannes Domani wrote:
> > I did some experiments.
> >
> > Seems like GenerateConsoleCtrlEvent() only works if the target process was
> > created without CREATE_NEW_CONSOLE.
> > And what's worse, it seems to always return TRUE, so it never reaches
> > DebugBreakProcess(), even if the target process doesn't have a console at all.
> >
> > I also tried the "last resort", with soft_interrupt_requested=1, but this
> > immediatly crashed gdbserver.
> >
> > I fixed this crash with:
> >
> > --- a/gdbserver/win32-low.cc
> > +++ b/gdbserver/win32-low.cc
> > @@ -1339,6 +1335,7 @@ fake_breakpoint_event (void)
> >    faked_breakpoint = 1;
> >
> >    memset (&current_event, 0, sizeof (current_event));
> > +  current_event.dwProcessId = current_process_id;
> >    current_event.dwThreadId = main_thread_id;
> >    current_event.dwDebugEventCode = EXCEPTION_DEBUG_EVENT;
> >    current_event.u.Exception.ExceptionRecord.ExceptionCode
> >
> > But this didn't work either, I think it's because gdb checks if the current
> > instruction is int3, and continues if not.
> >
> > I'm wondering why this last resort was added, was this done for pre-XP where
> > DebugBreakProcess() didn't exist?
>
> I digged a little bit.  It was introduced by this commit by a young
> Pedro :)
>
>   https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=4d5d1aaa19ea
>
> The corresponding mailing list post is:
>
>   https://sourceware.org/legacy-ml/gdb-patches/2007-11/msg00217.html
>
> It doesn't explain the rationale though, it just implicitly refers to a
> previous thread.  It's probably this one, which gives a bit more
> context:
>
>   https://sourceware.org/legacy-ml/gdb-patches/2007-11/msg00035.html
>
> From what I understand, this was for WinCE, which lacked
> GenerateConsoleCtrlEvent and DebugBreakProcess.  Since we removed
> support for WinCE, I think that code could very well be GC'ed.
>
> >
> > So right now ctrl-c doesn't really work that well with gdbserver, and not
> > at all when debugging a program without console (WOW64 or not doesn't matter).
> >
> >
> > And the WOW64 fix for DbgUiRemoteBreakin() probably can't be used in
> > gdbserver, because find_minimal_symbol_address() is not available.
>
> Using the qSymbol packet, there is a window during which GDBserver can
> as GDB to look up minimal symbol values.  On the GDBserver side, this is
> done through process_stratum_target::look_up_symbols, you would just
> need to implement it for win32_process_target.

OK, I will look into this (even though we maybe won't need it).


> > Meanwhile I came up with a possible alternative for DbgUiRemoteBreakin():
> >
> > --- a/gdb/nat/windows-nat.c
> > +++ b/gdb/nat/windows-nat.c
> > @@ -240,6 +241,13 @@ handle_exception (struct target_waitstatus *ourstatus, bool debug_exceptions)
> >      case EXCEPTION_BREAKPOINT:
> >  #ifdef __x86_64__
> >        if (ignore_first_breakpoint)
> >      {
> >        /* For WOW64 processes, there are always 2 breakpoint exceptions
> >          on startup, first a BREAKPOINT for the 64bit ntdll.dll,
> >          then a WX86_BREAKPOINT for the 32bit ntdll.dll.
> >          Here we only care about the WX86_BREAKPOINT's.  */
> >            ourstatus->kind = TARGET_WAITKIND_SPURIOUS;
> >          ignore_first_breakpoint = false;
> >        }
> > +      else if (wow64_process)
> > +      {
> > +        DEBUG_EXCEPTION_SIMPLE ("EXCEPTION_BREAKPOINT");
> > +        rec->ExceptionCode = DBG_CONTROL_C;
> > +        ourstatus->value.sig = GDB_SIGNAL_INT;
> > +        break;
> > +      }
> >  #endif
> >        /* FALLTHROUGH */
> >      case STATUS_WX86_BREAKPOINT:
> >
> > This transforms the (64bit) breakpoint from DebugBreakProcess() into
> > SIGINT, then the check for the int3 instruction is not done, and gdb stops
> > the target process in any case.
>
>
> That sounds simple (which is good).  Would this replace completely
> spawning the remote thread (which you added in the previous patch)?

Yes, this would replace the remote thread.


Hannes