From 5d87dd4fcac808fc0f0b993b470644ba7d2d62a2 Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Sun, 14 Jun 2009 22:38:51 +0200 Subject: [PATCH 1/3] daemon: send stderr of service programs to the syslog If git-daemon is run with --detach or --inetd, then stderr is explicitly redirected to /dev/null. But notice that the service programs were spawned via execl_git_cmd(), in particular, the stderr channel is inherited from the daemon. This means that errors that the programs wrote to stderr (for example, via die()), went to /dev/null. This patch arranges that the daemon does not merely exec the service program, but forks it and monitors stderr of the child; it writes the errors that it produces to the daemons log via logerror(). A consequence is that the daemon process remains in memory for the full duration of the service program, but this cannot be avoided. Signed-off-by: Johannes Sixt Signed-off-by: Junio C Hamano --- daemon.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 48 insertions(+), 8 deletions(-) diff --git a/daemon.c b/daemon.c index b2babcc076..a7834efbab 100644 --- a/daemon.c +++ b/daemon.c @@ -1,6 +1,8 @@ #include "cache.h" #include "pkt-line.h" #include "exec_cmd.h" +#include "run-command.h" +#include "strbuf.h" #include @@ -343,28 +345,66 @@ static int run_service(char *dir, struct daemon_service *service) return service->fn(); } +static void copy_to_log(int fd) +{ + struct strbuf line = STRBUF_INIT; + FILE *fp; + + fp = fdopen(fd, "r"); + if (fp == NULL) { + logerror("fdopen of error channel failed"); + close(fd); + return; + } + + while (strbuf_getline(&line, fp, '\n') != EOF) { + logerror("%s", line.buf); + strbuf_setlen(&line, 0); + } + + strbuf_release(&line); + fclose(fp); +} + +static int run_service_command(const char **argv) +{ + struct child_process cld; + + memset(&cld, 0, sizeof(cld)); + cld.argv = argv; + cld.git_cmd = 1; + cld.err = -1; + if (start_command(&cld)) + return -1; + + close(0); + close(1); + + copy_to_log(cld.err); + + return finish_command(&cld); +} + static int upload_pack(void) { /* Timeout as string */ char timeout_buf[64]; + const char *argv[] = { "upload-pack", "--strict", timeout_buf, ".", NULL }; snprintf(timeout_buf, sizeof timeout_buf, "--timeout=%u", timeout); - - /* git-upload-pack only ever reads stuff, so this is safe */ - execl_git_cmd("upload-pack", "--strict", timeout_buf, ".", NULL); - return -1; + return run_service_command(argv); } static int upload_archive(void) { - execl_git_cmd("upload-archive", ".", NULL); - return -1; + static const char *argv[] = { "upload-archive", ".", NULL }; + return run_service_command(argv); } static int receive_pack(void) { - execl_git_cmd("receive-pack", ".", NULL); - return -1; + static const char *argv[] = { "receive-pack", ".", NULL }; + return run_service_command(argv); } static struct daemon_service daemon_service[] = { From 9462e3f59cd5a545330a9490a27d68b79f1d0ce7 Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Tue, 16 Jun 2009 20:41:16 +0200 Subject: [PATCH 2/3] upload-pack: squelch progress indicator if client cannot see it upload-pack runs pack-objects, which generates progress indicator output on its stderr. If the client requests a sideband, this indicator is sent to the client; but if it did not, then the progress is written to upload-pack's own stderr. If upload-pack is itself run from git-daemon (and if the client did not request a sideband) the progress indicator never reaches the client and it need not be generated in the first place. With this patch the progress indicator is suppressed in this situation. Signed-off-by: Johannes Sixt Signed-off-by: Junio C Hamano --- upload-pack.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/upload-pack.c b/upload-pack.c index edc7861228..841ebb534a 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -28,7 +28,7 @@ static unsigned long oldest_have; static int multi_ack, nr_our_refs; static int use_thin_pack, use_ofs_delta, use_include_tag; -static int no_progress; +static int no_progress, daemon_mode; static struct object_array have_obj; static struct object_array want_obj; static unsigned int timeout; @@ -521,6 +521,10 @@ static void receive_needs(void) } if (debug_fd) write_in_full(debug_fd, "#E\n", 3); + + if (!use_sideband && daemon_mode) + no_progress = 1; + if (depth == 0 && shallows.nr == 0) return; if (depth > 0) { @@ -630,6 +634,7 @@ int main(int argc, char **argv) } if (!prefixcmp(arg, "--timeout=")) { timeout = atoi(arg+10); + daemon_mode = 1; continue; } if (!strcmp(arg, "--")) { From 2ff4d1ab9ef6660c88020ddaadc410157e130cdc Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Sun, 21 Jun 2009 23:16:09 +0200 Subject: [PATCH 3/3] receive-pack: do not send error details to the client If the objects that a client pushes to the server cannot be processed for any reason, an error is reported back to the client via the git protocol. We used to send quite detailed information if a system call failed if unpack-objects is run. This can be regarded as an information leak. Now we do not send any error details like we already do in the case where index-pack failed. Errors in system calls as well as the exit code of unpack-objects and index-pack are now reported to stderr; in the case of a local push or via ssh these messages still go to the client, but that is OK since these forms of access to the server assume that the client can be trusted. If receive-pack is run from git-daemon, then the daemon should put the error messages into the syslog. With this reasoning a new status report is added for the post-update-hook; untrusted (i.e. daemon's) clients cannot observe its status anyway, others may want to know failure details. Signed-off-by: Johannes Sixt Signed-off-by: Junio C Hamano --- builtin-receive-pack.c | 53 ++++++++++++++++++------------------------ 1 file changed, 22 insertions(+), 31 deletions(-) diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c index 33d345dc39..6ec1d056e6 100644 --- a/builtin-receive-pack.c +++ b/builtin-receive-pack.c @@ -123,27 +123,27 @@ static struct command *commands; static const char pre_receive_hook[] = "hooks/pre-receive"; static const char post_receive_hook[] = "hooks/post-receive"; -static int hook_status(int code, const char *hook_name) +static int run_status(int code, const char *cmd_name) { switch (code) { case 0: return 0; case -ERR_RUN_COMMAND_FORK: - return error("hook fork failed"); + return error("fork of %s failed", cmd_name); case -ERR_RUN_COMMAND_EXEC: - return error("hook execute failed"); + return error("execute of %s failed", cmd_name); case -ERR_RUN_COMMAND_PIPE: - return error("hook pipe failed"); + return error("pipe failed"); case -ERR_RUN_COMMAND_WAITPID: return error("waitpid failed"); case -ERR_RUN_COMMAND_WAITPID_WRONG_PID: return error("waitpid is confused"); case -ERR_RUN_COMMAND_WAITPID_SIGNAL: - return error("%s died of signal", hook_name); + return error("%s died of signal", cmd_name); case -ERR_RUN_COMMAND_WAITPID_NOEXIT: - return error("%s died strangely", hook_name); + return error("%s died strangely", cmd_name); default: - error("%s exited with error code %d", hook_name, -code); + error("%s exited with error code %d", cmd_name, -code); return -code; } } @@ -174,7 +174,7 @@ static int run_receive_hook(const char *hook_name) code = start_command(&proc); if (code) - return hook_status(code, hook_name); + return run_status(code, hook_name); for (cmd = commands; cmd; cmd = cmd->next) { if (!cmd->error_string) { size_t n = snprintf(buf, sizeof(buf), "%s %s %s\n", @@ -186,7 +186,7 @@ static int run_receive_hook(const char *hook_name) } } close(proc.in); - return hook_status(finish_command(&proc), hook_name); + return run_status(finish_command(&proc), hook_name); } static int run_update_hook(struct command *cmd) @@ -203,7 +203,7 @@ static int run_update_hook(struct command *cmd) argv[3] = sha1_to_hex(cmd->new_sha1); argv[4] = NULL; - return hook_status(run_command_v_opt(argv, RUN_COMMAND_NO_STDIN | + return run_status(run_command_v_opt(argv, RUN_COMMAND_NO_STDIN | RUN_COMMAND_STDOUT_TO_STDERR), update_hook); } @@ -394,7 +394,7 @@ static char update_post_hook[] = "hooks/post-update"; static void run_update_post_hook(struct command *cmd) { struct command *cmd_p; - int argc; + int argc, status; const char **argv; for (argc = 0, cmd_p = cmd; cmd_p; cmd_p = cmd_p->next) { @@ -417,8 +417,9 @@ static void run_update_post_hook(struct command *cmd) argc++; } argv[argc] = NULL; - run_command_v_opt(argv, RUN_COMMAND_NO_STDIN - | RUN_COMMAND_STDOUT_TO_STDERR); + status = run_command_v_opt(argv, RUN_COMMAND_NO_STDIN + | RUN_COMMAND_STDOUT_TO_STDERR); + run_status(status, update_post_hook); } static void execute_commands(const char *unpacker_error) @@ -534,24 +535,10 @@ static const char *unpack(void) unpacker[i++] = hdr_arg; unpacker[i++] = NULL; code = run_command_v_opt(unpacker, RUN_GIT_CMD); - switch (code) { - case 0: + if (!code) return NULL; - case -ERR_RUN_COMMAND_FORK: - return "unpack fork failed"; - case -ERR_RUN_COMMAND_EXEC: - return "unpack execute failed"; - case -ERR_RUN_COMMAND_WAITPID: - return "waitpid failed"; - case -ERR_RUN_COMMAND_WAITPID_WRONG_PID: - return "waitpid is confused"; - case -ERR_RUN_COMMAND_WAITPID_SIGNAL: - return "unpacker died of signal"; - case -ERR_RUN_COMMAND_WAITPID_NOEXIT: - return "unpacker died strangely"; - default: - return "unpacker exited with error code"; - } + run_status(code, unpacker[0]); + return "unpack-objects abnormal exit"; } else { const char *keeper[7]; int s, status, i = 0; @@ -574,8 +561,11 @@ static const char *unpack(void) ip.argv = keeper; ip.out = -1; ip.git_cmd = 1; - if (start_command(&ip)) + status = start_command(&ip); + if (status) { + run_status(status, keeper[0]); return "index-pack fork failed"; + } pack_lockfile = index_pack_lockfile(ip.out); close(ip.out); status = finish_command(&ip); @@ -583,6 +573,7 @@ static const char *unpack(void) reprepare_packed_git(); return NULL; } + run_status(status, keeper[0]); return "index-pack abnormal exit"; } }