[PATCH] Clean up file descriptors when calling hooks.

[PATCH] Clean up file descriptors when calling hooks.

[Daniel Barkalow]

When calling post-update hook, don't leave stdin and stdout connected to 
the pushing connection.

Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>

---

Someone who's got a better idea of what they should be instead should 
revise this to make it not just use /dev/null. This does fix my particular 
test, though; it doesn't cause a post-update hook to crash if it writes to 
stdout when pushing locally.

 receive-pack.c |    2 +-
 run-command.c  |   15 +++++++++++++--
 run-command.h  |    3 +++
 3 files changed, 17 insertions(+), 3 deletions(-)

ca48629563058ba62b97b6d4cd3776ca7b7242d3
diff --git a/receive-pack.c b/receive-pack.c
index 1873506..cbe37e7 100644
--- a/receive-pack.c
+++ b/receive-pack.c
@@ -173,7 +173,7 @@ static void run_update_post_hook(struct 
 		argc++;
 	}
 	argv[argc] = NULL;
-	run_command_v(argc, argv);
+	run_command_v_opt(argc, argv, RUN_COMMAND_NO_STDIO);
 }
 
 /*
diff --git a/run-command.c b/run-command.c
index 5787a50..8bf5922 100644
--- a/run-command.c
+++ b/run-command.c
@@ -2,13 +2,19 @@
 #include "run-command.h"
 #include <sys/wait.h>
 
-int run_command_v(int argc, char **argv)
+int run_command_v_opt(int argc, char **argv, int flags)
 {
 	pid_t pid = fork();
 
 	if (pid < 0)
 		return -ERR_RUN_COMMAND_FORK;
 	if (!pid) {
+		if (flags & RUN_COMMAND_NO_STDIO) {
+			int fd = open("/dev/null", O_RDWR);
+			dup2(fd, 0);
+			dup2(fd, 1);
+			close(fd);			
+		}
 		execvp(argv[0], (char *const*) argv);
 		die("exec %s failed.", argv[0]);
 	}
@@ -36,6 +42,11 @@ int run_command_v(int argc, char **argv)
 	}
 }
 
+int run_command_v(int argc, char **argv)
+{
+	return run_command_v_opt(argc, argv, 0);
+}
+
 int run_command(const char *cmd, ...)
 {
 	int argc;
@@ -54,5 +65,5 @@ int run_command(const char *cmd, ...)
 	va_end(param);
 	if (MAX_RUN_COMMAND_ARGS <= argc)
 		return error("too many args to run %s", cmd);
-	return run_command_v(argc, argv);
+	return run_command_v_opt(argc, argv, 0);
 }
diff --git a/run-command.h b/run-command.h
index 5ee0972..2469eea 100644
--- a/run-command.h
+++ b/run-command.h
@@ -11,6 +11,9 @@ enum {
 	ERR_RUN_COMMAND_WAITPID_NOEXIT,
 };
 
+#define RUN_COMMAND_NO_STDIO 1
+
+int run_command_v_opt(int argc, char **argv, int opt);
 int run_command_v(int argc, char **argv);
 int run_command(const char *cmd, ...);
 
-- 
0.99.9.GIT

Re: [PATCH] Clean up file descriptors when calling hooks.

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> When calling post-update hook, don't leave stdin and stdout connected to 
> the pushing connection.

A quick question.  I understand "not connected to the pushing
connection" is desirable, but is there a reason you chose to
leave them open to /dev/null, not close()d?

Re: [PATCH] Clean up file descriptors when calling hooks.

[linux]

> A quick question.  I understand "not connected to the pushing
> connection" is desirable, but is there a reason you chose to
> leave them open to /dev/null, not close()d?

Because then the first open() in the hook will assign those fds,
confusing programs that try to use them for their traditional
purposes.  fd 2 (stderr) is of particular concern.

E.g. imagine if I ran gcc -c file.c, and it assigned file.c to fd 0,
file.h to fd1, and file.o to fd 2.   Then wants to print a warning
message... right into the middle of the binary.  (Oversimplified
example, because gcc is actually several separate programs, but
hopefully you get the idea.)

It's just safer to leave those fds open to a null device.

Re: [PATCH] Clean up file descriptors when calling hooks.

[Junio C Hamano]

linux@horizon.com writes:

> E.g. imagine if I ran gcc -c file.c, and it assigned file.c to fd 0,
> file.h to fd1, and file.o to fd 2.   Then wants to print a warning
> message... right into the middle of the binary.  (Oversimplified
> example, because gcc is actually several separate programs, but
> hopefully you get the idea.)
>
> It's just safer to leave those fds open to a null device.

Overdangerous example.  Do people realize hooks run without any
locking?

Re: [PATCH] Clean up file descriptors when calling hooks.

[Daniel Barkalow]

On Wed, 7 Dec 2005, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > When calling post-update hook, don't leave stdin and stdout connected to 
> > the pushing connection.
> 
> A quick question.  I understand "not connected to the pushing
> connection" is desirable, but is there a reason you chose to
> leave them open to /dev/null, not close()d?

Wouldn't that make programs that assume that stdout (or stdin) is still 
valid get errors? I particularly want git-merge (which has a habit of 
saying "Already up-to-date. Yeeah!") to work. Also, something that assumes 
that fd 1 is somewhere to report stuff will get into trouble if it opens a 
couple of files for writing and fd 0/1 weren't open before. I seem to 
recall the standard thing for running with no I/O to be to use /dev/null 
(e.g., daemon(), if you don't keep your fds, replaces them with /dev/null).

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] Clean up file descriptors when calling hooks.

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> On Wed, 7 Dec 2005, Junio C Hamano wrote:
>
>> A quick question.  I understand "not connected to the pushing
>> connection" is desirable, but is there a reason you chose to
>> leave them open to /dev/null, not close()d?
>
> Wouldn't that make programs that assume that stdout (or stdin) is still 
> valid get errors?

right both of you are.  applied and pushed out.  thanks.