[PATCH 1/2] Don't close thread handles provided by WaitForDebugEvent

[PATCH 1/2] Don't close thread handles provided by WaitForDebugEvent

[Hannes Domani]

I sometimes encountered a weird breakpoint failure when using start:

(gdb) start
Temporary breakpoint 2 at 0x40162d: file gdb-25911.c, line 4.
Starting program: C:\src\tests\gdb-25911.exe
Warning:
Cannot insert breakpoint 2.
Cannot access memory at address 0x401628

After trying a lot of combinations, I found a way to reproduce it:

(gdb) file gdb-25987.exe
Reading symbols from gdb-25987.exe...
(gdb) start
Temporary breakpoint 1 at 0x401638: file gdb-25987.cpp, line 13.
Starting program: C:\src\tests\gdb-25987.exe

Temporary breakpoint 1, main () at gdb-25987.cpp:13
13      int main() {
(gdb) c
Continuing.

Program received signal SIGSEGV, Segmentation fault.
MyClass::call (this=0x3d20d0) at gdb-25987.cpp:8
8           *(char*)(nullptr) = 1;
(gdb) kill
Kill the program being debugged? (y or n) y
[Inferior 1 (process 1140) killed]
(gdb) file gdb-25911.exe
Load new symbol table from "gdb-25911.exe"? (y or n) y
Reading symbols from gdb-25911.exe...
(gdb) start
Temporary breakpoint 2 at 0x40162d: file gdb-25911.c, line 4.
Starting program: C:\src\tests\gdb-25911.exe
Warning:
Cannot insert breakpoint 2.
Cannot access memory at address 0x401628

Command aborted.

The actual failure was that ReadProcessMemory used a process handle that
was no longer valid.
And the underlying reason was that the windows_thread_info destructor
closes a thread handle that was provided earlier by WaitForDebugEvent.

But since this is not allowed (and it was actually already closed at this
point, and the handle value reused), this closed another still-needed handle.

gdb/ChangeLog:

2020-05-24  Hannes Domani  <ssbssa@yahoo.de>

	* nat/windows-nat.c (windows_thread_info::~windows_thread_info):
	Don't close thread handle.
---
 gdb/nat/windows-nat.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/gdb/nat/windows-nat.c b/gdb/nat/windows-nat.c
index 8c2092a51d..709a9d3a31 100644
--- a/gdb/nat/windows-nat.c
+++ b/gdb/nat/windows-nat.c
@@ -51,7 +51,6 @@ bool ignore_first_breakpoint = false;
 
 windows_thread_info::~windows_thread_info ()
 {
-  CloseHandle (h);
 }
 
 void
-- 
2.26.2

[PATCH 2/2] Don't close process handle provided by WaitForDebugEvent

[Hannes Domani]

Only the process handle returned by OpenProcess or CreateProcess needs to
be closed, the one provided by WaitForDebugEvent is closed automatically.

gdbserver/ChangeLog:

2020-05-25  Hannes Domani  <ssbssa@yahoo.de>

	* win32-low.cc (do_initial_child_stuff): Set open_process_used.
	(win32_clear_inferiors): Use open_process_used.
	(get_child_debug_event): Likewise.
---
 gdbserver/win32-low.cc | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc
index 4eb63b7ca2..e440e28913 100644
--- a/gdbserver/win32-low.cc
+++ b/gdbserver/win32-low.cc
@@ -88,6 +88,9 @@ static int soft_interrupt_requested = 0;
    by suspending all the threads.  */
 static int faked_breakpoint = 0;
 
+/* True if current_process_handle needs to be closed.  */
+bool open_process_used = false;
+
 #ifdef __x86_64__
 bool wow64_process = false;
 #endif
@@ -383,6 +386,7 @@ do_initial_child_stuff (HANDLE proch, DWORD pid, int attached)
 
   soft_interrupt_requested = 0;
   faked_breakpoint = 0;
+  open_process_used = true;
 
   memset (&current_event, 0, sizeof (current_event));
 
@@ -859,8 +863,11 @@ windows_nat::handle_output_debug_string (struct target_waitstatus *ourstatus)
 static void
 win32_clear_inferiors (void)
 {
-  if (current_process_handle != NULL)
-    CloseHandle (current_process_handle);
+  if (open_process_used)
+    {
+      CloseHandle (current_process_handle);
+      open_process_used = false;
+    }
 
   for_each_thread (delete_thread_info);
   siginfo_er.ExceptionCode = 0;
@@ -1513,6 +1520,12 @@ get_child_debug_event (DWORD *continue_status,
 		(unsigned) current_event.dwThreadId));
       CloseHandle (current_event.u.CreateProcessInfo.hFile);
 
+      if (open_process_used)
+	{
+	  CloseHandle (current_process_handle);
+	  open_process_used = false;
+	}
+
       current_process_handle = current_event.u.CreateProcessInfo.hProcess;
       main_thread_id = current_event.dwThreadId;
 
@@ -1560,8 +1573,6 @@ get_child_debug_event (DWORD *continue_status,
 	  }
       }
       child_continue (DBG_CONTINUE, desired_stop_thread_id);
-      CloseHandle (current_process_handle);
-      current_process_handle = NULL;
       break;
 
     case LOAD_DLL_DEBUG_EVENT:
-- 
2.26.2

Re: [PATCH 1/2] Don't close thread handles provided by WaitForDebugEvent

[Tom Tromey]

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

Hannes> 2020-05-24  Hannes Domani  <ssbssa@yahoo.de>

Hannes> 	* nat/windows-nat.c (windows_thread_info::~windows_thread_info):
Hannes> 	Don't close thread handle.

Thanks, I found this, which seems to agree with your analysis:

https://docs.microsoft.com/en-us/windows/win32/api/debugapi/nf-debugapi-waitfordebugevent

So, I think this patch is ok.

Tom

Re: [PATCH 2/2] Don't close process handle provided by WaitForDebugEvent

[Tom Tromey]

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

Hannes> +/* True if current_process_handle needs to be closed.  */
Hannes> +bool open_process_used = false;

This should probably be "static".

The patch is ok with this changed.

Perhaps a bit more of this code can be merged between gdb and gdbserver?
I didn't look too deeply.

Tom

Re: [PATCH 2/2] Don't close process handle provided by WaitForDebugEvent

[Hannes Domani]

 Am Dienstag, 26. Mai 2020, 22:30:58 MESZ hat Tom Tromey <tom@tromey.com> Folgendes geschrieben:

> >>>>> "Hannes" == Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Hannes> +/* True if current_process_handle needs to be closed.  */
> Hannes> +bool open_process_used = false;
>
> This should probably be "static".
>
> The patch is ok with this changed.

Pushed with this change, and also the thread one.


> Perhaps a bit more of this code can be merged between gdb and gdbserver?
> I didn't look too deeply.

I plan to look into this once I don't have so many open patches any more.


Thanks,
Hannes