Skip to content

Fix push on mac3 #79

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

Closed
kewillford wants to merge 2 commits into from
Closed

Fix push on mac3 #79

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1cb2e4a
run-command: allow child to close all parent file descriptors
Nov 28, 2018
c6d1634
sub-process: turn on the close_parent_file_descriptors flag
Nov 29, 2018
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
6 changes: 6 additions & 0 deletions run-command.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -716,6 +716,7 @@ int start_command(struct child_process *cmd)
{
int notify_pipe[2];
int null_fd = -1;
int fd;
char **childenv;
struct argv_array argv = ARGV_ARRAY_INIT;
struct child_err cerr;
Expand Down Expand Up @@ -795,6 +796,11 @@ int start_command(struct child_process *cmd)
if (cmd->dir && chdir(cmd->dir))
child_die(CHILD_ERR_CHDIR);

if (cmd->close_parent_file_descriptors) {
for (fd = 3; fd < 256; ++fd)
Copy link
Member

@dscho dscho Nov 29, 2018

Choose a reason for hiding this comment

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

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.)

Copy link

@benpeart benpeart Nov 30, 2018

Choose a reason for hiding this comment

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

I'm curious about the magic "3." I'd probably at least comment that you aren't closing stdin/out/err and why.

set_cloexec(fd);
}

/*
* restore default signal handlers here, in case
* we catch a signal right before execve below
Expand Down
1 change: 1 addition & 0 deletions run-command.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ struct child_process {
unsigned use_shell:1;
unsigned clean_on_exit:1;
unsigned wait_after_clean:1;
unsigned close_parent_file_descriptors:1;
Copy link
Member

@dscho dscho Nov 29, 2018

Choose a reason for hiding this comment

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

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.

Copy link

@benpeart benpeart Nov 30, 2018

Choose a reason for hiding this comment

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

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. :)

void (*clean_on_exit_handler)(struct child_process *process);
void *clean_on_exit_handler_cbdata;
};
Expand Down
1 change: 1 addition & 0 deletions sub-process.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co
process->in = -1;
process->out = -1;
process->clean_on_exit = 1;
process->close_parent_file_descriptors = 1;
process->clean_on_exit_handler = subprocess_exit_handler;
process->trace2_child_class = "subprocess";

Expand Down