Fix Windows process implementation to only inherit the three

Fix Windows process implementation to only inherit the three
handles needed for stdin, stdout and stderr.

R=sgjesse@google.com,ahe@google.com,kustermann@google.com
BUG=dartbug.com/7191

Committed: https://code.google.com/p/dart/source/detail?r=15896

[ahe, 2012-12-07 17:01:21]

Initial comments: can you think of a way to test this?

I'll take a closer look at the code on Monday.

[Mads Ager (google), 2012-12-07 17:03:51]

I had the same thought just after uploading. I can't immediately think of a good
way of testing this automatically, but I will give it more thought on Monday. I
tested this manually before updating with the recursive process starting from
the bug report.

[ahe, 2012-12-07 17:17:30]

Will this work:

Start three "recursive" processes. 

The first waits for stdin of the sub process to close. It receives a process id
from the second so it can kill the third.

The second exits right after starting the third, and letting its parent know the
third process' id. 

The third waits forever.

[kustermann, 2012-12-08 11:53:42]

It seems like this approach (passing an explicit list of  handles to
CreateProcess) works only from Vista on.

Do we want to support WinXP?

https://codereview.chromium.org/11474026/diff/1/runtime/bin/process_win.cc
File runtime/bin/process_win.cc (right):

https://codereview.chromium.org/11474026/diff/1/runtime/bin/process_win.cc#ne...
runtime/bin/process_win.cc:421: HANDLE* inherited_handles = new
HANDLE[num_inherited_handles];
Is there a reason why you allocate this array on the heap and not on the stack?

[kustermann, 2012-12-10 08:15:41]

On 2012/12/07 17:17:30, ahe wrote:
> Will this work:
> 
> Start three "recursive" processes. 
> 
> The first waits for stdin of the sub process to close. It receives a process
id
> from the second so it can kill the third.
> 
> The second exits right after starting the third, and letting its parent know
the
> third process' id. 
> 
> The third waits forever.

IMHO we should not restrict ourselves to {stdin,stdout,stderr}. The same issue
can occur on any handle (pipe, file, socket, mutex, ....) [as you know, we
observed the socket-fd inheritance issue on linux already].

But since the c++ OS-specific code is not changed that often, it would probably
be ok if we just run manual tests (it is hard anyway to get a good test for all
handle-types).

[Mads Ager (google), 2012-12-10 08:46:16]

On 2012/12/08 11:53:42, kustermann wrote:
> It seems like this approach (passing an explicit list of  handles to
> CreateProcess) works only from Vista on.
> 
> Do we want to support WinXP?

This will only work correctly on Vista and forward. XP will have the old
behavior. We should attempt to support WinXP, but this seems like a good start.

https://codereview.chromium.org/11474026/diff/1/runtime/bin/process_win.cc
> File runtime/bin/process_win.cc (right):
> 
>
https://codereview.chromium.org/11474026/diff/1/runtime/bin/process_win.cc#ne...
> runtime/bin/process_win.cc:421: HANDLE* inherited_handles = new
> HANDLE[num_inherited_handles];
> Is there a reason why you allocate this array on the heap and not on the
stack?

[ahe, 2012-12-10 10:30:51]

Overall, LGTM, but it would be great with a test.

https://codereview.chromium.org/11474026/diff/1/runtime/bin/process_win.cc
File runtime/bin/process_win.cc (right):

https://codereview.chromium.org/11474026/diff/1/runtime/bin/process_win.cc#ne...
runtime/bin/process_win.cc:404: GetLastError() != ERROR_INSUFFICIENT_BUFFER) {
Why are you ignoring ERROR_INSUFFICIENT_BUFFER?

https://codereview.chromium.org/11474026/diff/1/runtime/bin/process_win.cc#ne...
runtime/bin/process_win.cc:414: free(attribute_list);
Shouldn't you call free after getting the error message?

https://codereview.chromium.org/11474026/diff/1/runtime/bin/process_win.cc#ne...
runtime/bin/process_win.cc:463: int error_code =
SetOsErrorMessage(os_error_message, os_error_message_len);
Shouldn't you call SetOsErrorMessage first?

[Mads Ager (google), 2012-12-10 10:45:29]

https://codereview.chromium.org/11474026/diff/1/runtime/bin/process_win.cc
File runtime/bin/process_win.cc (right):

https://codereview.chromium.org/11474026/diff/1/runtime/bin/process_win.cc#ne...
runtime/bin/process_win.cc:404: GetLastError() != ERROR_INSUFFICIENT_BUFFER) {
On 2012/12/10 10:30:51, ahe wrote:
> Why are you ignoring ERROR_INSUFFICIENT_BUFFER?

I will add a comment about that. When you pass in NULL to
InitializeProcThreadAttributeList it retuns the size needed and reports
INSUFFICIENT_BUFFER. This is slightly weird but is apparently the way to use
this API to catch other potential errors.

https://codereview.chromium.org/11474026/diff/1/runtime/bin/process_win.cc#ne...
runtime/bin/process_win.cc:414: free(attribute_list);
On 2012/12/10 10:30:51, ahe wrote:
> Shouldn't you call free after getting the error message?

Yes, thanks!

https://codereview.chromium.org/11474026/diff/1/runtime/bin/process_win.cc#ne...
runtime/bin/process_win.cc:421: HANDLE* inherited_handles = new
HANDLE[num_inherited_handles];
On 2012/12/08 11:53:42, kustermann wrote:
> Is there a reason why you allocate this array on the heap and not on the
stack?

Thanks for catching that Martin. No, there is no reason for heap allocation
here.

https://codereview.chromium.org/11474026/diff/1/runtime/bin/process_win.cc#ne...
runtime/bin/process_win.cc:463: int error_code =
SetOsErrorMessage(os_error_message, os_error_message_len);
On 2012/12/10 10:30:51, ahe wrote:
> Shouldn't you call SetOsErrorMessage first?

Yes, thanks!