Mark stdin/out/err pipe fds as close on exec so that forking post start_command would be safe #128
Conversation
dscho
left a comment
dscho
left a comment
There was a problem hiding this comment.
I really like where this is going.
run-command.c
Outdated
There was a problem hiding this comment.
This is a very good idea. At first, though, I misread this as closing the very file descriptor in the child that the child needs to read from.
I guess it would make for another really good idea to describe a bit better what this patch is all about. In any case, the format of the commit message should also be made to conform to https://git-scm.com/docs/SubmittingPatches#describe-changes. A good example to follow is ccdc481, I would think.
It also misses a Signed-off-by footer, which is required by the Git project (to which we will want to contribute your fix, eventually).
There was a problem hiding this comment.
Good points, will write up in detail what the issue was with existing code and how this change fixes it. Really appreciate the example.
There was a problem hiding this comment.
I updated commit messages, hope they make things clearer. If you have any feedback though, suggestions are welcome.
|
FWIW the CI jobs fail for reasons that are unrelated to your changes (fixed and explained in the "status: add status serialization mechanism", "status: serialize to path" and "Add virtual file system settings and hook proc" sections of #122 (comment)). Read: those failures do not block this here PR. |
This makes it possible to run more than one command started using start_command at the same time. When invoked start_command would create a set of pipes that it uses to communicate with the subprocess that was spawned, the git process that invoked start_command would own the file descriptors corresponding to the ends of the pipes pointing at stdout, stdin and stderr respectivelly. Which meant that if another process is spawned via fork + exec it would inherit all of those file descriptors from the git process, file descriptors that whatever process just got spawned by exec has no idea what to do with, which in some cases would cause the process that got spawned first to hang. For example, if git spawns two subprocesses A and B, and tries to shut down process A first, git closes the corresponding file descriptors, but as process B still has those same file descriptors pointing at the same pipes open, process A is still trying to read from those two pipes and not getting an EOF. Once this fix is applied - the process B does not inherit pipe descriptions, because this flag specifies that the file descriptor should be closed when an exec function is invoked. This makes running multiple commands safe. Signed-off-by: Ignas Mikalajunas <ignas@uber.com>
Ignas
force-pushed
the
close-fds-on-exec
branch
from
1b3cc04 to
80bdd5d
Compare
|
Will you create a fixup commit for the revert by running: This makes it easier to drop the commit when we rebase to the next version of git. Also you can use https://github.com/gitgitgadget/gitgitgadget to submit the single commit upstream to get additional feedback on the change from the git mailing list. Thanks. |
Ignas
force-pushed
the
close-fds-on-exec
branch
from
80bdd5d to
987df9e
Compare
|
FWIW this is the corresponding GitGitGadget PR: gitgitgadget#164 |
Closing this PR as this will go upstream with GitGitGadget. |
4 participants
This fixes microsoft/VFSForGit#830 by reverting #68 and fixing the underlying issue (which is not closing pipe fds before exec).