Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os/exec: status of FDs >= len(cmd.ExtraFiles)+3 is undocumented #37857

Open
pam4 opened this issue Mar 14, 2020 · 6 comments
Open

os/exec: status of FDs >= len(cmd.ExtraFiles)+3 is undocumented #37857

pam4 opened this issue Mar 14, 2020 · 6 comments

Comments

@pam4
Copy link

@pam4 pam4 commented Mar 14, 2020

What version of Go are you using (go version)?

go version go1.14 linux/amd64

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOOS="linux"

What did you do?

Invocation: $ parent-cmd 0<a 1>b 2>c 3>d 4>e 5>f

cmd := exec.Command("child-cmd")
cmd.ExtraFiles = []*os.File{os.Stderr}
cmd.Stdin = os.Stdin
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
cmd.Start() // in child-cmd: 0=a 1=b 2=c 3=c 4=e 5=closed!

What did you expect to see?

File descriptors 4 and 5 (>= len(cmd.ExtraFiles)+3) to be either both inherited or none, and the behavior to be documented.

What did you see instead?

File descriptor 4 is inherited, but 5 is not. The doc doesn't say anything about
FDs >= len(cmd.ExtraFiles)+3 in the child process.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 15, 2020

The status of files that are not listed in ExtraFiles is not unpredictable: those files are closed. If someone wants to add a doc sentence to that effect, that should be fine.

Loading

@ianlancetaylor ianlancetaylor changed the title os/exec: status of FDs >= len(cmd.ExtraFiles)+3 is unpredictable and undocumented os/exec: status of FDs >= len(cmd.ExtraFiles)+3 is undocumented Mar 15, 2020
@ianlancetaylor ianlancetaylor added this to the Backlog milestone Mar 15, 2020
@pam4
Copy link
Author

@pam4 pam4 commented Mar 15, 2020

@ianlancetaylor

The status of files that are not listed in ExtraFiles is not unpredictable: those files are closed.

I'm talking about FDs >= len(cmd.ExtraFiles)+3: I demonstrated a case in which the parent has 2 of them (from shell redirection, hence FD_CLOEXEC unset), and one of them is closed in the child but the other is not. Have you actually read my test case?

Here is another example:

cmd := exec.Command("child-cmd")
cmd.Stdin = os.Stdin
cmd.Stdout = os.Stderr // swapping stdout and stderr
cmd.Stderr = os.Stdout
cmd.Start()

Invocation: $ parent-cmd 0<a 1>b 2>c 3>d 4>e
In child-cmd: 0=a 1=c 2=b 3=d 4=closed (3 is inherited but 4 is closed!)

Without swapping:

cmd := exec.Command("child-cmd")
cmd.Stdin = os.Stdin
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
cmd.Start()

Same invocation: $ parent-cmd 0<a 1>b 2>c 3>d 4>e
In child-cmd: 0=a 1=b 2=c 3=d 4=e (now both 3 and 4 are inherited)

Loading

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 15, 2020

Sorry, you're right; I was very unclear. When I said "files", I mean *os.File values, which are the kinds of values that can be put into ExtraFiles.

But upon reflection even that is not quite right, since if you do use shell redirection and then use os.NewFile to open a redirected descriptor, that *os.File is likely to remain open after an exec, since nothing will set the FD_CLOEXEC flag. So I guess it may take a couple of sentences.

Loading

@pam4
Copy link
Author

@pam4 pam4 commented Mar 15, 2020

@ianlancetaylor, sorry, you are still missing my point.

I would expect parent FDs >= len(cmd.ExtraFiles)+3 to be never inherited if they have FD_CLOEXEC set (e.g. from os.OpenFile), and always inherited if they have FD_CLOEXEC unset.

But if they have FD_CLOEXEC unset, the result is actually unpredictable, because they may or may not be overwritten during the FD shuffling that happens in the child before execve.

Have you read the example in my previous message? Can you explain why the effect on FD 4 is different between the two programs?

Loading

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 16, 2020

Hmmm, I still haven't looked very closely at this, but that makes me think that this is just a bug. Perhaps the code in syscall should fstat the descriptor before calling dup3.

Loading

@pam4
Copy link
Author

@pam4 pam4 commented Mar 17, 2020

Yes, nextfd setup here doesn't take into account FDs not included in attr.Files.
Consider also replacing dup[23] in "pass 1" with something like newfd = fcntl(oldfd, F_DUPFD_CLOEXEC, minfd).

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants