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

Christopher Faylor
2008-01-11T18:32:16Z

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