code review 5086050: syscall: mark stdin, stdout and stderr as non-inheritab...

syscall: mark stdin, stdout and stderr as non-inheritable by child process

Fixes issue 2250.

[brainman, 2011-09-22 09:01:53]

Hello golang-dev@googlegroups.com (cc: hectorchu@gmail.com,
vincent.vanackere@gmail.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[hector, 2011-09-22 13:46:53]

http://codereview.appspot.com/5086050/diff/5001/src/pkg/syscall/syscall_windo...
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/5086050/diff/5001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:282: h, e :=
CreateFile(StringToUTF16Ptr(path), access, sharemode, nil, createmode,
FILE_ATTRIBUTE_NORMAL, 0)
We don't need to remove this, the caller can use O_CLOEXEC if it needs to (which
os.OpenFile does).

http://codereview.appspot.com/5086050/diff/5001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:425: e := CreatePipe(&r, &w, nil, 0)
We don't need to remove this either, the caller can use CloseOnExec if it needs
to (which os.Pipe does).

[brainman, 2011-09-23 05:46:52]

http://codereview.appspot.com/5086050/diff/5001/src/pkg/syscall/syscall_windo...
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/5086050/diff/5001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:282: h, e :=
CreateFile(StringToUTF16Ptr(path), access, sharemode, nil, createmode,
FILE_ATTRIBUTE_NORMAL, 0)
On 2011/09/22 13:46:53, hector wrote:
> We don't need to remove this, the caller can use O_CLOEXEC if it needs to
(which
> os.OpenFile does).

Perhaps I am mistaken, but reading comment for os.ProcAttr.Files:

// Files specifies the open files inherited by the new process.  The
// first three entries correspond to standard input, standard output, and
// standard error.  An implementation may support additional entries,
// depending on the underlying operating system.  A nil entry corresponds
// to that file being closed when the process starts.

My interpretation for windows is, we must supply at most 3 first file handles
and that is it. No?

[hector, 2011-09-23 07:16:48]

On Sep 23, 2011 6:46 AM, <alex.brainman@gmail.com> wrote:
> Perhaps I am mistaken, but reading comment for os.ProcAttr.Files:
>
> // Files specifies the open files inherited by the new process.  The
> // first three entries correspond to standard input, standard output,
> and
> // standard error.  An implementation may support additional entries,
> // depending on the underlying operating system.  A nil entry
> corresponds
> // to that file being closed when the process starts.
>
> My interpretation for windows is, we must supply at most 3 first file
> handles and that is it. No?

Windows handle inheritance is more general than just the standard io
handles. For example the Microsoft C runtime library uses inherited handles
to implement file descriptor inheritance. Inherited handles will have the
same value in the child as in the parent. I am suggesting we leave around
this functionality in syscall in case we or others need it in the future.

[brainman, 2011-09-23 07:39:07]

On 2011/09/23 07:16:48, hector wrote:
> ... I am suggesting we leave around
> this functionality in syscall in case we or others need it in the future.

OK. But why did you make CreatePipe handles inheritable by default. Wouldn't we
have problems similar to this one?

Alex

[hector, 2011-09-23 07:52:28]

It's only to match the default in Unix.  As I said in a previous
email, os.Pipe immediately sets it to close-on-exec so the default for
most people will be non-inheritable.

On 23 September 2011 08:39,  <alex.brainman@gmail.com> wrote:
> On 2011/09/23 07:16:48, hector wrote:
>>
>> ... I am suggesting we leave around
>> this functionality in syscall in case we or others need it in the
>
> future.
>
> OK. But why did you make CreatePipe handles inheritable by default.
> Wouldn't we have problems similar to this one?
>
> Alex
>
> http://codereview.appspot.com/5086050/
>

[brainman, 2011-09-23 07:54:04]

Hello golang-dev@googlegroups.com, hectorchu@gmail.com (cc:
golang-dev@googlegroups.com, vincent.vanackere@gmail.com),

Please take another look.

[hector, 2011-09-23 07:55:43]

LGTM

On 23 September 2011 08:54,  <alex.brainman@gmail.com> wrote:
> Hello golang-dev@googlegroups.com, hectorchu@gmail.com (cc:
> golang-dev@googlegroups.com, vincent.vanackere@gmail.com),
>
> Please take another look.
>
>
> http://codereview.appspot.com/5086050/
>

[brainman, 2011-09-23 08:16:22]

*** Submitted as http://code.google.com/p/go/source/detail?r=086dc976137c ***

syscall: mark stdin, stdout and stderr as non-inheritable by child process

Fixes issue 2250.

R=golang-dev, hectorchu
CC=golang-dev, vincent.vanackere
http://codereview.appspot.com/5086050