[RFA] win32-nat.c: Restore wrongly removed code that closes two handles returned by CreateProcess

[RFA] win32-nat.c: Restore wrongly removed code that closes two handles returned by CreateProcess

[Pierre Muller]

 This patch restores two lines of code that I wrongfully
removed in a previous patch about problems related to 
closing of handles that the system should close.

The applied patch is:
http://sourceware.org/ml/gdb-patches/2007-11/msg00481.html

  In that patch,
 I also removed two calls to CloseHandle functions for the two
handles that are returned by the CreateProcess function.
To make things worse, I forgot to mention that
removal in the corresponding ChangeLog.

The wrong part of the patch is in win32_create_inferior
@@ -1886,9 +1893,6 @@ win32_create_inferior (char *exec_file,
     error (_("Error creating process %s, (error %d)."),
           exec_file, (unsigned) GetLastError ());

-  CloseHandle (pi.hThread);
-  CloseHandle (pi.hProcess);
-
   if (useshell && shell[0] != '\0')
     saw_create = -1;
   else

  I was supposing that the handles returned by
CreateProcess in the ProcessInformation structure
were the same as the handles given in the 
initial CREATE_PROCESS_DEBUG_EVENT.
  But these two handles are completely separate, as
printing out the values enabled me to see. The handles
returned by CreateProcess are given to any program
launching another executable, while the other handles are debugger
specific.

   The purpose of that patch was about avoiding to close the handles
that are given on debug events, as stated in the windows API 
documentation. But the same docs do specify that
the two handles returned in the ProcessInformation structure
need to be closed.

  I ran the testsuite on that patch and found no
change, but the missing CloseHandle calls might still
have some influence on other cases.
  Luckily no gdb release contains that error.

  Christopher, should I modify the ChangeLog-2007 entry
that contains the bad removal of that code, as
the entry is incomplete?


  Sorry about this mistake,

Pierre Muller


ChangeLog entry:

2008-01-11  Pierre Muller  <muller@ics.u-strasbg.fr> 

	* win32-nat.c (win32_create_inferior): Restore
	code calling CloseHandle on ProcessInformation structure.


$ cvs diff -up gdb/win32-nat.c
Index: gdb/win32-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/win32-nat.c,v
retrieving revision 1.146
diff -u -p -r1.146 win32-nat.c
--- gdb/win32-nat.c     6 Jan 2008 06:59:14 -0000       1.146
+++ gdb/win32-nat.c     11 Jan 2008 17:08:40 -0000
@@ -1880,6 +1880,9 @@ win32_create_inferior (char *exec_file,
     error (_("Error creating process %s, (error %d)."),
           exec_file, (unsigned) GetLastError ());

+  CloseHandle (pi.hThread);
+  CloseHandle (pi.hProcess);
+
   if (useshell && shell[0] != '\0')
     saw_create = -1;
   else

Re: [RFA] win32-nat.c: Restore wrongly removed code that closes two handles returned by CreateProcess

[Christopher Faylor]

On Fri, Jan 11, 2008 at 06:20:48PM +0100, Pierre Muller wrote:
> This patch restores two lines of code that I wrongfully
>removed in a previous patch about problems related to 
>closing of handles that the system should close.
>
>The applied patch is:
>http://sourceware.org/ml/gdb-patches/2007-11/msg00481.html
>
>  In that patch,
> I also removed two calls to CloseHandle functions for the two
>handles that are returned by the CreateProcess function.
>To make things worse, I forgot to mention that
>removal in the corresponding ChangeLog.
>
>The wrong part of the patch is in win32_create_inferior
>@@ -1886,9 +1893,6 @@ win32_create_inferior (char *exec_file,
>     error (_("Error creating process %s, (error %d)."),
>           exec_file, (unsigned) GetLastError ());
>
>-  CloseHandle (pi.hThread);
>-  CloseHandle (pi.hProcess);
>-
>   if (useshell && shell[0] != '\0')
>     saw_create = -1;
>   else
>
>  I was supposing that the handles returned by
>CreateProcess in the ProcessInformation structure
>were the same as the handles given in the 
>initial CREATE_PROCESS_DEBUG_EVENT.
>  But these two handles are completely separate, as
>printing out the values enabled me to see. The handles
>returned by CreateProcess are given to any program
>launching another executable, while the other handles are debugger
>specific.
>
>   The purpose of that patch was about avoiding to close the handles
>that are given on debug events, as stated in the windows API 
>documentation. But the same docs do specify that
>the two handles returned in the ProcessInformation structure
>need to be closed.
>
>  I ran the testsuite on that patch and found no
>change, but the missing CloseHandle calls might still
>have some influence on other cases.
>  Luckily no gdb release contains that error.
>
>  Christopher, should I modify the ChangeLog-2007 entry
>that contains the bad removal of that code, as
>the entry is incomplete?

No, AFAIK, you are not supposed to retroactively change a ChangeLog
entry like that.

>  Sorry about this mistake,

No need to apologize! I'm glad you caught this and I'm glad that you
researched your earlier changes.  I actually stumbled across your
initial bug separately last week while debugging something with an older
version of gdb.  So, I'm happy that you fixed it.

>ChangeLog entry:
>
>2008-01-11  Pierre Muller  <muller@ics.u-strasbg.fr> 
>
>	* win32-nat.c (win32_create_inferior): Restore
>	code calling CloseHandle on ProcessInformation structure.

Please check in.  I'm sorry that I didn't catch this when I reviewed
your change.  It's obvious in perfect 20-20 hindsight.

cgf

RE: [RFA] win32-nat.c: Restore wrongly removed code that closes two handles returned by CreateProcess

[Pierre Muller]

 
> No need to apologize! I'm glad you caught this and I'm glad that you
> researched your earlier changes.  I actually stumbled across your
> initial bug separately last week while debugging something with an
> older
> version of gdb.  So, I'm happy that you fixed it.
> 
> >ChangeLog entry:
> >
> >2008-01-11  Pierre Muller  <muller@ics.u-strasbg.fr>
> >
> >	* win32-nat.c (win32_create_inferior): Restore
> >	code calling CloseHandle on ProcessInformation structure.
> 
> Please check in.  I'm sorry that I didn't catch this when I reviewed
> your change.  It's obvious in perfect 20-20 hindsight.

Done, thanks,

Pierre Muller