Fix push on mac3 #79
Fix push on mac3 #79
Conversation
When starting a child process using fork() all file descriptors of the parent are inherited by the child process. This can cause deadlocks in git when the following happens. 1. Start a child process - this opens pipes in the parent process in order for the parent to read/write data to the child. The child process dups the end of the pipes to stdin/stdout/stderr. 2. Parent does some work that starts another child process - this process will get the open pipes that the parent is using to communicate with the first child process. 3. Parent writes to the first child through pipe and closes it. 4. Parent reads from the first child through pipe while the second child process is still running. 5. Parent is deadlocked on the read because although the first child is done and pipes were closed, the second child has the file descriptors to the pipes for the first child which are still open. Since the parent might not know that it is spawning a child process, all possible file descriptors will to be closed on exec if flag is set.
|
@benpeart Do you know how upstreamable this change is? I don't like that it is a for loop checking each number to determine if it should close the file descriptor. I didn't find any good way to get all the open file descriptors for the current process though. |
dscho
left a comment
dscho
left a comment
There was a problem hiding this comment.
Maybe you can think of a better name for that flag...
| child_die(CHILD_ERR_CHDIR); | ||
|
|
||
| if (cmd->close_parent_file_descriptors) { | ||
| for (fd = 3; fd < 256; ++fd) |
There was a problem hiding this comment.
If you want to upstream this, change ++fd to fd++. For some reason, the Git developers prefer this.
About the magic 256... This sent me down a fun rabbit hole, thank you. The short version is that you should replace the constant by getrlimit(RLIMIT_NOFILE, &fd_max_plus_one).
But wait! Is getrlimit() available everywhere? And even more importantly: does Git already use that function? Let's find out... git grep getrlimit to the rescue.
Turns out that my short version was wrong: there is a get_max_fd_limit() function in packfile.c. What you will want to do is to move it to a more sensible place, declare it as a global function in cache.h, and then use it also here.
(BTW typically, the RLIMIT_NOFILE value seems to be 1024... and beware: get_max_fd_limit() returns 1 as a fall-back, if there is no other available means to determine the value.)
There was a problem hiding this comment.
I'm curious about the magic "3." I'd probably at least comment that you aren't closing stdin/out/err and why.
| unsigned use_shell:1; | ||
| unsigned clean_on_exit:1; | ||
| unsigned wait_after_clean:1; | ||
| unsigned close_parent_file_descriptors:1; |
There was a problem hiding this comment.
I don't know about that name... I read it initially as all of the parent's file descriptors, but that is not true. For one, the fds that need to be inherited (because they are connected to a pipe so that the spawning process can talk to the spawned process) need to not be closed. And then, we only close file descriptors referring to files. If there is a socket open, the code typically still talks about "file" descriptors, but we do not close those.
Maybe restrict_fd_inheritance. Nah. Also not very good. I am not very good at naming things.
There was a problem hiding this comment.
I can't think of anything better than 'close_parent_file_descriptors' - it doesn't say "all" file descriptors so someone will have to look at the code/commit/comments if they want to know which ones. :)
benpeart
left a comment
benpeart
left a comment
There was a problem hiding this comment.
I agree with dscho on using 'get_max_fd_limit()' I'd also add/improve the comments on which file descriptors are being closed and why.
| child_die(CHILD_ERR_CHDIR); | ||
|
|
||
| if (cmd->close_parent_file_descriptors) { | ||
| for (fd = 3; fd < 256; ++fd) |
There was a problem hiding this comment.
I'm curious about the magic "3." I'd probably at least comment that you aren't closing stdin/out/err and why.
| unsigned use_shell:1; | ||
| unsigned clean_on_exit:1; | ||
| unsigned wait_after_clean:1; | ||
| unsigned close_parent_file_descriptors:1; |
There was a problem hiding this comment.
I can't think of anything better than 'close_parent_file_descriptors' - it doesn't say "all" file descriptors so someone will have to look at the code/commit/comments if they want to know which ones. :)
3 participants
When starting a child process using fork() all file descriptors of
the parent are inherited by the child process. This can cause
deadlocks in git when the following happens.
in order for the parent to read/write data to the child. The child
process dups the end of the pipes to stdin/stdout/stderr.
process will get the open pipes that the parent is using to
communicate with the first child process.
child process is still running.
is done and pipes were closed, the second child has the file
descriptors to the pipes for the first child which are still open.
Since the parent might not know that it is spawning a child process,
all possible file descriptors will to be closed on exec if flag is
set.