Skip to content

Set FD_CLOEXEC on stdin/out/err pipe fds in start_command #164

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.

Sign up for GitHub

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

Open
Ignas wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Set FD_CLOEXEC on stdin/out/err pipe fds in start_command #164

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions run-command.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -966,18 +966,24 @@ int start_command(struct child_process *cmd)
return -1;
Copy link
Member

@dscho dscho Mar 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about changing the oneline of the commit message to something that explains more the "why?" than the "what?"? I have something in mind like:

Let only the intended child process inherit the stdin/stdout/stderr pipes

... and then explaining that we optionally create pipes for the standard file descriptors, to be able to feed stdin to the child process in the parent process, and consume stdout and stderr. We already close the pipes' ends in the parent process that are intended for the child process. But when spawning another child process, the other end is still open, and will be inherited by the wrong child process.

I would then try to explain that this does not currently pose a big problem in Git, as pretty much all child processes are short-lived. And then I would try to give a concrete example (not "A" and "B", but rather something like our read-object hook in VFS for Git and any other child process such as a hook (is this understanding correct?)).

Maybe you want to adjust the commit message in this way?

I might very well misunderstand the precise problem/solution, but even in that case, I think it is more important to answer the "why?" than the "what?" in the commit message...

}

if (need_in)
if (need_in) {
close(fdin[0]);
set_cloexec(fdin[1]);
}
else if (cmd->in)
close(cmd->in);

if (need_out)
if (need_out) {
close(fdout[1]);
set_cloexec(fdout[0]);
}
else if (cmd->out)
close(cmd->out);

if (need_err)
if (need_err) {
close(fderr[1]);
set_cloexec(fderr[0]);
}
else if (cmd->err)
close(cmd->err);

Expand Down