[31/34,v1.2] Windows gdb+gdbserver: Check whether DBG_REPLY_LATER is available

Message ID d7b3f201-ee73-496d-b369-35b987bc7451@palves.net
State New
Headers
Series None |

Commit Message

Pedro Alves May 8, 2024, 9:33 p.m. UTC
  On 2024-05-08 13:45, Eli Zaretskii wrote:

>> However, I found out that we can find the Windows major/minor/build in
>> the KUSER_SHARED_DATA structure, which defines the layout of a data
>> area that the kernel places at a pre-set address for sharing with
>> user-mode software:
>>
>>   https://www.geoffchappell.com/studies/windows/km/ntoskrnl/structs/kuser_shared_data/index.htm
>>
>> The Windows major/minor/build version retrieved using that method
>> bypasses the manifest stuff, it actually gets you the real OS version
>> numbers.  That is what this patch is using.
> 
> ...do we really need to do this via a version-check?  Can't we instead
> just call ContinueDebugEvent and if it fails, consider DBG_REPLY_LATER
> unsupported?  (If calling ContinueDebugEvent with that flag on older
> versions of Windows causes an exception, we could use try/catch.)  If
> this works, it is a more reliable way to test, IMO and IME.  I think
> we should prefer that to poking kernel data structures.

We need to know whether DBG_REPLY_LATER will work before starting the inferior.
And we can only call ContinueDebugEvent after starting some inferior, and
after the kernel returns an event for it via WaitForDebugEvent.

Hannes on IRC suggested using RtlGetVersion, which is exported by ntdll.dll.
I tried it, and it works nicely.  Looking around the web, I see suggestions to
use that on stockoverflow, blogs, etc.  I don't know why I didn't run into
that one before.  Maybe because the MSFT page describing it says it is
"kernel-mode":

 https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-rtlgetversion

In truth, you can call it in user mode just fine.

Here is a version of the patch using that function.  Much simpler!

From 4331947c4728071b2d53011bf7d70bd0ab7c4e93 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Wed, 8 May 2024 21:14:26 +0100
Subject: [PATCH] Windows gdb+gdbserver: Check whether DBG_REPLY_LATER is
 available

Per
<https://learn.microsoft.com/en-us/windows/win32/api/debugapi/nf-debugapi-continuedebugevent>,
DBG_REPLY_LATER is "Supported in Windows 10, version 1507 or above, ..."

We need to know whether DBG_REPLY_LATER is available before starting
any inferior.

On Linux, we check which ptrace options are supported by the running
kernel by forking gdb and then the parent gdb debugging the child gdb
with PTRACE_ME, and then trying to set the ptrace options.

Doing something like that on Windows would be more complicated,
because we can't just fork, we have to start some executable, and the
only executable we know we can start, probably, is gdb itself.  And
that's a large program, so takes time to be started.  And then we'd
have to implement a WaitForDebugEvent loop to start up the process,
and then finally try ContinueDebugEvent(DBG_REPLY_LATER).

It's a lot simpler to just check the Windows version.  Unlike on
Linux, we don't have to worry about kernel feature backports.  This
patch does that.

Change-Id: Ia27b981aeecaeef430ec90cebc5b3abdce00449d
---
 gdb/nat/windows-nat.c | 56 +++++++++++++++++++++++++++++++++++++++++++
 gdb/nat/windows-nat.h |  8 +++++++
 2 files changed, 64 insertions(+)
  

Comments

Hannes Domani May 9, 2024, 10:07 a.m. UTC | #1
Am Mittwoch, 8. Mai 2024 um 23:34:25 MESZ hat Pedro Alves <pedro@palves.net> Folgendes geschrieben:

> On 2024-05-08 13:45, Eli Zaretskii wrote:
>
> >> However, I found out that we can find the Windows major/minor/build in
> >> the KUSER_SHARED_DATA structure, which defines the layout of a data
> >> area that the kernel places at a pre-set address for sharing with
> >> user-mode software:
> >>
> >>  https://www.geoffchappell.com/studies/windows/km/ntoskrnl/structs/kuser_shared_data/index.htm
> >>
> >> The Windows major/minor/build version retrieved using that method
> >> bypasses the manifest stuff, it actually gets you the real OS version
> >> numbers.  That is what this patch is using.
> >
> > ...do we really need to do this via a version-check?  Can't we instead
> > just call ContinueDebugEvent and if it fails, consider DBG_REPLY_LATER
> > unsupported?  (If calling ContinueDebugEvent with that flag on older
> > versions of Windows causes an exception, we could use try/catch.)  If
> > this works, it is a more reliable way to test, IMO and IME.  I think
> > we should prefer that to poking kernel data structures.
>
> We need to know whether DBG_REPLY_LATER will work before starting the inferior.
> And we can only call ContinueDebugEvent after starting some inferior, and
> after the kernel returns an event for it via WaitForDebugEvent.

Looks like just trying to call ContinueDebugEvent is possible after all.

#include <windows.h>
#include <stdio.h>

#ifndef DBG_REPLY_LATER
#define DBG_REPLY_LATER 0x40010001
#endif

int main()
{
  if (!ContinueDebugEvent(0, 0, DBG_REPLY_LATER))
    printf("error: 0x%lx\n", GetLastError());

  return 0;
}

On Win10 this gives me error 0x6 (ERROR_INVALID_HANDLE), and on Win7 it
gives me error 0x57 (ERROR_INVALID_PARAMETER).


Hannes
  
Pedro Alves May 9, 2024, 11:11 a.m. UTC | #2
On 2024-05-09 11:46, Eli Zaretskii wrote:
>> Date: Thu, 9 May 2024 10:07:05 +0000 (UTC)
>> From: Hannes Domani <ssbssa@yahoo.de>
>> Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
>>
>>  Am Mittwoch, 8. Mai 2024 um 23:34:25 MESZ hat Pedro Alves <pedro@palves.net> Folgendes geschrieben:
>>
>>> On 2024-05-08 13:45, Eli Zaretskii wrote:
>>>
>>>> ...do we really need to do this via a version-check?  Can't we instead
>>>> just call ContinueDebugEvent and if it fails, consider DBG_REPLY_LATER
>>>> unsupported?  (If calling ContinueDebugEvent with that flag on older
>>>> versions of Windows causes an exception, we could use try/catch.)  If
>>>> this works, it is a more reliable way to test, IMO and IME.  I think
>>>> we should prefer that to poking kernel data structures.
>>>
>>> We need to know whether DBG_REPLY_LATER will work before starting the inferior.
>>> And we can only call ContinueDebugEvent after starting some inferior, and
>>> after the kernel returns an event for it via WaitForDebugEvent.
>>
>> Looks like just trying to call ContinueDebugEvent is possible after all.
>>
>> #include <windows.h>
>> #include <stdio.h>
>>
>> #ifndef DBG_REPLY_LATER
>> #define DBG_REPLY_LATER 0x40010001
>> #endif
>>
>> int main()
>> {
>>   if (!ContinueDebugEvent(0, 0, DBG_REPLY_LATER))
>>     printf("error: 0x%lx\n", GetLastError());
>>
>>   return 0;
>> }
>>
>> On Win10 this gives me error 0x6 (ERROR_INVALID_HANDLE), and on Win7 it
>> gives me error 0x57 (ERROR_INVALID_PARAMETER).
> 
> Yes, ERROR_INVALID_PARAMETER is what I'd expect when the value is not
> supported.
> 

Well, during development, I saw ERROR_INVALID_PARAMETER (0x57/87) errors when
ContinueDebugEvent is called at a time when you should not (when there is no
event to continue), even without passing any invalid option.  So I'm surprised
you'd get ERROR_INVALID_HANDLE on Win10.  I'll try Win11.

Are we going to trust that all the older supported Windows versions will work
this way (without testing them all)?  WinXP, Win7, etc?  If so, I can definitely
switch to that approach.
  
Pedro Alves May 9, 2024, 11:47 a.m. UTC | #3
On 2024-05-09 12:11, Pedro Alves wrote:
> On 2024-05-09 11:46, Eli Zaretskii wrote:
>>> From: Hannes Domani <ssbssa@yahoo.de>

>>> Looks like just trying to call ContinueDebugEvent is possible after all.

...

>>>
>>> On Win10 this gives me error 0x6 (ERROR_INVALID_HANDLE), and on Win7 it
>>> gives me error 0x57 (ERROR_INVALID_PARAMETER).
>>
>> Yes, ERROR_INVALID_PARAMETER is what I'd expect when the value is not
>> supported.
>>
> 
> Well, during development, I saw ERROR_INVALID_PARAMETER (0x57/87) errors when
> ContinueDebugEvent is called at a time when you should not (when there is no
> event to continue), even without passing any invalid option.  So I'm surprised
> you'd get ERROR_INVALID_HANDLE on Win10.  I'll try Win11.
> 
> Are we going to trust that all the older supported Windows versions will work
> this way (without testing them all)?  WinXP, Win7, etc?  If so, I can definitely
> switch to that approach.

I tried Win11, and I get the same result as Hannes on Win10.

Here is the updated patch with that approach, then.  Thanks a lot for discovering this.

From bb2a38325b6f79bc84924b979fe163b5004c2350 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Thu, 9 May 2024 12:32:53 +0100
Subject: [PATCH] Windows gdb+gdbserver: Check whether DBG_REPLY_LATER is
 available

Per
<https://learn.microsoft.com/en-us/windows/win32/api/debugapi/nf-debugapi-continuedebugevent>,
DBG_REPLY_LATER is "Supported in Windows 10, version 1507 or above, ..."

Since we support versions of Windows older than 10, we need to know
whether DBG_REPLY_LATER is available.  And we need to know this before
starting any inferior.

This adds a function that probes for support (and caches the result),
by trying to call ContinueDebugEvent on pid=0,tid=0 with DBG_REPLY_LATER,
and inspecting the resulting error.

Suggested-by: Hannes Domani <ssbssa@yahoo.de>
Suggested-by: Eli Zaretskii <eliz@gnu.org>
Change-Id: Ia27b981aeecaeef430ec90cebc5b3abdce00449d
---
 gdb/nat/windows-nat.c | 20 ++++++++++++++++++++
 gdb/nat/windows-nat.h |  9 +++++++++
 2 files changed, 29 insertions(+)

diff --git a/gdb/nat/windows-nat.c b/gdb/nat/windows-nat.c
index 4fd717e6521..e36b7477b05 100644
--- a/gdb/nat/windows-nat.c
+++ b/gdb/nat/windows-nat.c
@@ -941,6 +941,26 @@ disable_randomization_available ()
 
 /* See windows-nat.h.  */
 
+bool
+dbg_reply_later_available ()
+{
+  static int available = -1;
+  if (available == -1)
+    {
+      /* DBG_REPLY_LATER is supported since Windows 10, Version 1507.
+	 If supported, this fails with ERROR_INVALID_HANDLE (tested on
+	 Win10 and Win11).  If not supported, it fails with
+	 ERROR_INVALID_PARAMETER (tested on Win7).  */
+      if (ContinueDebugEvent (0, 0, DBG_REPLY_LATER))
+	internal_error (_("ContinueDebugEvent call should not "
+			  "have succeeded"));
+      available = (GetLastError() != ERROR_INVALID_PARAMETER);
+    }
+  return available;
+}
+
+/* See windows-nat.h.  */
+
 bool
 initialize_loadable ()
 {
diff --git a/gdb/nat/windows-nat.h b/gdb/nat/windows-nat.h
index 2efb54e1ce7..96835b3ef40 100644
--- a/gdb/nat/windows-nat.h
+++ b/gdb/nat/windows-nat.h
@@ -415,6 +415,15 @@ extern DeleteProcThreadAttributeList_ftype *DeleteProcThreadAttributeList;
 
 extern bool disable_randomization_available ();
 
+/* This is available starting with Windows 10.  */
+#ifndef DBG_REPLY_LATER
+# define DBG_REPLY_LATER 0x40010001L
+#endif
+
+/* Return true if it's possible to use DBG_REPLY_LATER with
+   ContinueDebugEvent on this host.  */
+extern bool dbg_reply_later_available ();
+
 /* Load any functions which may not be available in ancient versions
    of Windows.  */
  
Eli Zaretskii May 9, 2024, 12:28 p.m. UTC | #4
> Date: Thu, 9 May 2024 12:47:19 +0100
> From: Pedro Alves <pedro@palves.net>
> Cc: gdb-patches@sourceware.org
> 
> I tried Win11, and I get the same result as Hannes on Win10.
> 
> Here is the updated patch with that approach, then.  Thanks a lot for discovering this.

LGTM, thanks.
  
Pedro Alves May 9, 2024, 1:27 p.m. UTC | #5
On 2024-05-09 13:24, Eli Zaretskii wrote:
>> Date: Thu, 9 May 2024 12:11:40 +0100
>> Cc: gdb-patches@sourceware.org
>> From: Pedro Alves <pedro@palves.net>
>>
>> Are we going to trust that all the older supported Windows versions will work
>> this way (without testing them all)?  WinXP, Win7, etc?  If so, I can definitely
>> switch to that approach.
> 
> AFAIK, ERROR_INVALID_PARAMETER is the standard response to an
> unsupported parameter value, yes.
> 

That part will never in question.  As I said earlier, I have seen ERROR_INVALID_PARAMETER
errors when you call ContinueDebugEvent at a time when there is no event to continue, because
it had been continued already.  The error we see when we pass down pid=0 is ERROR_INVALID_HANDLE.
But we don't pass a handle down to ContinueDebugEvent, we pass down a pid and a tid.
So it could be reasonable for Windows to return ERROR_INVALID_PARAMETER in this situation too.

The risk is that some version of Windows does that (either older and newer), and we misdetect
the support as non-existing.

But we can cross that bridge when we come to it.  I'm happy with the approach as is.
  
Tom Tromey May 9, 2024, 2:17 p.m. UTC | #6
>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:

Pedro> +      available = (GetLastError() != ERROR_INVALID_PARAMETER);

Missing space before the "(".

I like this version of the patch, thanks to everyone.
Approved-By: Tom Tromey <tom@tromey.com>

Tom
  

Patch

diff --git a/gdb/nat/windows-nat.c b/gdb/nat/windows-nat.c
index 4fd717e6521..a7383276ec2 100644
--- a/gdb/nat/windows-nat.c
+++ b/gdb/nat/windows-nat.c
@@ -40,6 +40,7 @@  namespace windows_nat
    ContinueDebugEvent.  */
 static DEBUG_EVENT last_wait_event;
 
+RtlGetVersion_ftype *RtlGetVersion;
 AdjustTokenPrivileges_ftype *AdjustTokenPrivileges;
 DebugActiveProcessStop_ftype *DebugActiveProcessStop;
 DebugBreakProcess_ftype *DebugBreakProcess;
@@ -939,6 +940,57 @@  disable_randomization_available ()
 	  && DeleteProcThreadAttributeList != nullptr);
 }
 
+/* Helper for dbg_reply_later_available.  Does the actual work, while
+   dbg_reply_later_available handles caching.  */
+
+static bool
+dbg_reply_later_available_1 ()
+{
+  /* Windows has a number of functions you can use to check the OS
+     version, like GetVersion/GetVersionEx, or the Version Helper
+     functions like IsWindows10OrGreater, VerifyVersionInfo, etc.
+     However, as explained by:
+
+       https://learn.microsoft.com/en-us/windows/win32/api/sysinfoapi/nf-sysinfoapi-getversionexa
+
+     ... and other pages, "Applications not manifested for Windows 8.1
+     or Windows 10 will return the Windows 8 OS version value (6.2)."
+
+     RtlGetVersion is simpler because it bypasses the manifest
+     machinery.  */
+
+  /* We require Windows XP, and RtlGetVersion exists since Windows
+     2000.  */
+  gdb_assert (RtlGetVersion != nullptr);
+
+  OSVERSIONINFOW version_info;
+  version_info.dwOSVersionInfoSize = sizeof (OSVERSIONINFOW);
+  /* This is documented to always succeed.  */
+  gdb_assert (RtlGetVersion (&version_info) == 0);
+
+  debug_printf ("gdb: Windows version: major=%d, minor=%d, build=%d\n",
+		version_info.dwMajorVersion,
+		version_info.dwMinorVersion,
+		version_info.dwBuildNumber);
+
+  /* DBG_REPLY_LATER is supported since Windows 10, Version 1507,
+     which is reported as build number 10240.  */
+  return (version_info.dwMajorVersion > 10
+	  || (version_info.dwMajorVersion == 10
+	      && version_info.dwBuildNumber >= 10240));
+}
+
+/* See windows-nat.h.  */
+
+bool
+dbg_reply_later_available ()
+{
+  static int available = -1;
+  if (available == -1)
+    available = dbg_reply_later_available_1 ();
+  return available;
+}
+
 /* See windows-nat.h.  */
 
 bool
@@ -950,6 +1002,10 @@  initialize_loadable ()
 #define GPA(m, func)					\
   func = (func ## _ftype *) GetProcAddress (m, #func)
 
+  hm = LoadLibrary (TEXT ("ntdll.dll"));
+  if (hm)
+    GPA (hm, RtlGetVersion);
+
   hm = LoadLibrary (TEXT ("kernel32.dll"));
   if (hm)
     {
diff --git a/gdb/nat/windows-nat.h b/gdb/nat/windows-nat.h
index 2efb54e1ce7..cee710e07d4 100644
--- a/gdb/nat/windows-nat.h
+++ b/gdb/nat/windows-nat.h
@@ -295,6 +295,7 @@  extern BOOL create_process (const wchar_t *image, wchar_t *command_line,
 			    PROCESS_INFORMATION *process_info);
 #endif /* __CYGWIN__ */
 
+#define RtlGetVersion			dyn_RtlGetVersion
 #define AdjustTokenPrivileges		dyn_AdjustTokenPrivileges
 #define DebugActiveProcessStop		dyn_DebugActiveProcessStop
 #define DebugBreakProcess		dyn_DebugBreakProcess
@@ -322,6 +323,9 @@  extern BOOL create_process (const wchar_t *image, wchar_t *command_line,
 #define UpdateProcThreadAttribute dyn_UpdateProcThreadAttribute
 #define DeleteProcThreadAttributeList dyn_DeleteProcThreadAttributeList
 
+typedef NTSTATUS NTAPI (RtlGetVersion_ftype) (PRTL_OSVERSIONINFOW);
+extern RtlGetVersion_ftype *RtlGetVersion;
+
 typedef BOOL WINAPI (AdjustTokenPrivileges_ftype) (HANDLE, BOOL,
 						   PTOKEN_PRIVILEGES,
 						   DWORD, PTOKEN_PRIVILEGES,
@@ -415,6 +419,10 @@  extern DeleteProcThreadAttributeList_ftype *DeleteProcThreadAttributeList;
 
 extern bool disable_randomization_available ();
 
+/* Return true if it's possible to use DBG_REPLY_LATER with
+   ContinueDebugEvent on this host.  */
+extern bool dbg_reply_later_available ();
+
 /* Load any functions which may not be available in ancient versions
    of Windows.  */