From afe19ff7b55129d988e421ae1e0df4ec9659787a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 7 Jan 2012 12:42:43 +0100 Subject: [PATCH 1/5] run-command: optionally kill children on exit When we spawn a helper process, it should generally be done and finish_command called before we exit. However, if we exit abnormally due to an early return or a signal, the helper may continue to run in our absence. In the best case, this may simply be wasted CPU cycles or a few stray messages on a terminal. But it could also mean a process that the user thought was aborted continues to run to completion (e.g., a push's pack-objects helper will complete the push, even though you killed the push process). This patch provides infrastructure for run-command to keep track of PIDs to be killed, and clean them on signal reception or input, just as we do with tempfiles. PIDs can be added in two ways: 1. If NO_PTHREADS is defined, async helper processes are automatically marked. By definition this code must be ready to die when the parent dies, since it may be implemented as a thread of the parent process. 2. If the run-command caller specifies the "clean_on_exit" option. This is not the default, as there are cases where it is OK for the child to outlive us (e.g., when spawning a pager). PIDs are cleared from the kill-list automatically during wait_or_whine, which is called from finish_command and finish_async. Signed-off-by: Jeff King Signed-off-by: Clemens Buchacher Signed-off-by: Junio C Hamano --- run-command.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++ run-command.h | 1 + 2 files changed, 69 insertions(+) diff --git a/run-command.c b/run-command.c index 1c51043884..0204aaf7e8 100644 --- a/run-command.c +++ b/run-command.c @@ -1,8 +1,66 @@ #include "cache.h" #include "run-command.h" #include "exec_cmd.h" +#include "sigchain.h" #include "argv-array.h" +struct child_to_clean { + pid_t pid; + struct child_to_clean *next; +}; +static struct child_to_clean *children_to_clean; +static int installed_child_cleanup_handler; + +static void cleanup_children(int sig) +{ + while (children_to_clean) { + struct child_to_clean *p = children_to_clean; + children_to_clean = p->next; + kill(p->pid, sig); + free(p); + } +} + +static void cleanup_children_on_signal(int sig) +{ + cleanup_children(sig); + sigchain_pop(sig); + raise(sig); +} + +static void cleanup_children_on_exit(void) +{ + cleanup_children(SIGTERM); +} + +static void mark_child_for_cleanup(pid_t pid) +{ + struct child_to_clean *p = xmalloc(sizeof(*p)); + p->pid = pid; + p->next = children_to_clean; + children_to_clean = p; + + if (!installed_child_cleanup_handler) { + atexit(cleanup_children_on_exit); + sigchain_push_common(cleanup_children_on_signal); + installed_child_cleanup_handler = 1; + } +} + +static void clear_child_for_cleanup(pid_t pid) +{ + struct child_to_clean **last, *p; + + last = &children_to_clean; + for (p = children_to_clean; p; p = p->next) { + if (p->pid == pid) { + *last = p->next; + free(p); + return; + } + } +} + static inline void close_pair(int fd[2]) { close(fd[0]); @@ -130,6 +188,9 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure) } else { error("waitpid is confused (%s)", argv0); } + + clear_child_for_cleanup(pid); + errno = failed_errno; return code; } @@ -292,6 +353,8 @@ fail_pipe: if (cmd->pid < 0) error("cannot fork() for %s: %s", cmd->argv[0], strerror(failed_errno = errno)); + else if (cmd->clean_on_exit) + mark_child_for_cleanup(cmd->pid); /* * Wait for child's execvp. If the execvp succeeds (or if fork() @@ -312,6 +375,7 @@ fail_pipe: cmd->pid = -1; } close(notify_pipe[0]); + } #else { @@ -356,6 +420,8 @@ fail_pipe: failed_errno = errno; if (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT)) error("cannot spawn %s: %s", cmd->argv[0], strerror(errno)); + if (cmd->clean_on_exit && cmd->pid >= 0) + mark_child_for_cleanup(cmd->pid); if (cmd->env) free_environ(env); @@ -540,6 +606,8 @@ int start_async(struct async *async) exit(!!async->proc(proc_in, proc_out, async->data)); } + mark_child_for_cleanup(async->pid); + if (need_in) close(fdin[0]); else if (async->in) diff --git a/run-command.h b/run-command.h index 56491b9f23..2a6946668b 100644 --- a/run-command.h +++ b/run-command.h @@ -38,6 +38,7 @@ struct child_process { unsigned silent_exec_failure:1; unsigned stdout_to_stderr:1; unsigned use_shell:1; + unsigned clean_on_exit:1; void (*preexec_cb)(void); }; From 10c6cddd928b24ac6030a172c6c7b46efb32aedc Mon Sep 17 00:00:00 2001 From: Clemens Buchacher Date: Sun, 8 Jan 2012 21:41:09 +0100 Subject: [PATCH 2/5] dashed externals: kill children on exit Several git commands are so-called dashed externals, that is commands executed as a child process of the git wrapper command. If the git wrapper is killed by a signal, the child process will continue to run. This is different from internal commands, which always die with the git wrapper command. Enable the recently introduced cleanup mechanism for child processes in order to make dashed externals act more in line with internal commands. Signed-off-by: Clemens Buchacher Acked-by: Jeff King Signed-off-by: Junio C Hamano --- git.c | 2 +- run-command.c | 1 + run-command.h | 1 + 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/git.c b/git.c index fb9029cbf1..3805616630 100644 --- a/git.c +++ b/git.c @@ -495,7 +495,7 @@ static void execv_dashed_external(const char **argv) * if we fail because the command is not found, it is * OK to return. Otherwise, we just pass along the status code. */ - status = run_command_v_opt(argv, RUN_SILENT_EXEC_FAILURE); + status = run_command_v_opt(argv, RUN_SILENT_EXEC_FAILURE | RUN_CLEAN_ON_EXIT); if (status >= 0 || errno != ENOENT) exit(status); diff --git a/run-command.c b/run-command.c index 0204aaf7e8..1db8abf984 100644 --- a/run-command.c +++ b/run-command.c @@ -497,6 +497,7 @@ static void prepare_run_command_v_opt(struct child_process *cmd, cmd->stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0; cmd->silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0; cmd->use_shell = opt & RUN_USING_SHELL ? 1 : 0; + cmd->clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0; } int run_command_v_opt(const char **argv, int opt) diff --git a/run-command.h b/run-command.h index 2a6946668b..44f7d2bd42 100644 --- a/run-command.h +++ b/run-command.h @@ -53,6 +53,7 @@ extern int run_hook(const char *index_file, const char *name, ...); #define RUN_COMMAND_STDOUT_TO_STDERR 4 #define RUN_SILENT_EXEC_FAILURE 8 #define RUN_USING_SHELL 16 +#define RUN_CLEAN_ON_EXIT 32 int run_command_v_opt(const char **argv, int opt); /* From 71039fb9d562731ed700ef072b6fcb18e2478361 Mon Sep 17 00:00:00 2001 From: Clemens Buchacher Date: Sat, 7 Jan 2012 12:42:45 +0100 Subject: [PATCH 3/5] git-daemon: add tests The semantics of the git daemon tests are similar to the http transport tests. In fact, they are only a slightly modified copy of t5550, plus the newly added remote error tests. All git-daemon tests will be skipped unless the environment variable GIT_TEST_GIT_DAEMON is set. Signed-off-by: Clemens Buchacher Helped-by: Jeff King Signed-off-by: Junio C Hamano --- t/lib-git-daemon.sh | 53 +++++++++++++++ t/t5570-git-daemon.sh | 148 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 201 insertions(+) create mode 100644 t/lib-git-daemon.sh create mode 100755 t/t5570-git-daemon.sh diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh new file mode 100644 index 0000000000..5e81a25942 --- /dev/null +++ b/t/lib-git-daemon.sh @@ -0,0 +1,53 @@ +#!/bin/sh + +if test -z "$GIT_TEST_GIT_DAEMON" +then + skip_all="git-daemon testing disabled (define GIT_TEST_GIT_DAEMON to enable)" + test_done +fi + +LIB_GIT_DAEMON_PORT=${LIB_GIT_DAEMON_PORT-'8121'} + +GIT_DAEMON_PID= +GIT_DAEMON_DOCUMENT_ROOT_PATH="$PWD"/repo +GIT_DAEMON_URL=git://127.0.0.1:$LIB_GIT_DAEMON_PORT + +start_git_daemon() { + if test -n "$GIT_DAEMON_PID" + then + error "start_git_daemon already called" + fi + + mkdir -p "$GIT_DAEMON_DOCUMENT_ROOT_PATH" + + trap 'code=$?; stop_git_daemon; (exit $code); die' EXIT + + say >&3 "Starting git daemon ..." + git daemon --listen=127.0.0.1 --port="$LIB_GIT_DAEMON_PORT" \ + --reuseaddr --verbose \ + --base-path="$GIT_DAEMON_DOCUMENT_ROOT_PATH" \ + "$@" "$GIT_DAEMON_DOCUMENT_ROOT_PATH" \ + >&3 2>&4 & + GIT_DAEMON_PID=$! +} + +stop_git_daemon() { + if test -z "$GIT_DAEMON_PID" + then + return + fi + + trap 'die' EXIT + + # kill git-daemon child of git + say >&3 "Stopping git daemon ..." + kill "$GIT_DAEMON_PID" + wait "$GIT_DAEMON_PID" >&3 2>&4 + ret=$? + # expect exit with status 143 = 128+15 for signal TERM=15 + if test $ret -ne 143 + then + error "git daemon exited with status: $ret" + fi + GIT_DAEMON_PID= +} diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh new file mode 100755 index 0000000000..7cbc9994a3 --- /dev/null +++ b/t/t5570-git-daemon.sh @@ -0,0 +1,148 @@ +#!/bin/sh + +test_description='test fetching over git protocol' +. ./test-lib.sh + +LIB_GIT_DAEMON_PORT=${LIB_GIT_DAEMON_PORT-5570} +. "$TEST_DIRECTORY"/lib-git-daemon.sh +start_git_daemon + +test_expect_success 'setup repository' ' + echo content >file && + git add file && + git commit -m one +' + +test_expect_success 'create git-accessible bare repository' ' + mkdir "$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git" && + (cd "$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git" && + git --bare init && + : >git-daemon-export-ok + ) && + git remote add public "$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git" && + git push public master:master +' + +test_expect_success 'clone git repository' ' + git clone "$GIT_DAEMON_URL/repo.git" clone && + test_cmp file clone/file +' + +test_expect_success 'fetch changes via git protocol' ' + echo content >>file && + git commit -a -m two && + git push public && + (cd clone && git pull) && + test_cmp file clone/file +' + +test_expect_failure 'remote detects correct HEAD' ' + git push public master:other && + (cd clone && + git remote set-head -d origin && + git remote set-head -a origin && + git symbolic-ref refs/remotes/origin/HEAD > output && + echo refs/remotes/origin/master > expect && + test_cmp expect output + ) +' + +test_expect_success 'prepare pack objects' ' + cp -R "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo.git "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git && + (cd "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git && + git --bare repack -a -d + ) +' + +test_expect_success 'fetch notices corrupt pack' ' + cp -R "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_bad1.git && + (cd "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_bad1.git && + p=`ls objects/pack/pack-*.pack` && + chmod u+w $p && + printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc + ) && + mkdir repo_bad1.git && + (cd repo_bad1.git && + git --bare init && + test_must_fail git --bare fetch "$GIT_DAEMON_URL/repo_bad1.git" && + test 0 = `ls objects/pack/pack-*.pack | wc -l` + ) +' + +test_expect_success 'fetch notices corrupt idx' ' + cp -R "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_bad2.git && + (cd "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_bad2.git && + p=`ls objects/pack/pack-*.idx` && + chmod u+w $p && + printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc + ) && + mkdir repo_bad2.git && + (cd repo_bad2.git && + git --bare init && + test_must_fail git --bare fetch "$GIT_DAEMON_URL/repo_bad2.git" && + test 0 = `ls objects/pack | wc -l` + ) +' + +test_remote_error() +{ + do_export=YesPlease + while test $# -gt 0 + do + case $1 in + -x) + shift + chmod -x "$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git" + ;; + -n) + shift + do_export= + ;; + *) + break + esac + done + + if test $# -ne 3 + then + error "invalid number of arguments" + fi + + cmd=$1 + repo=$2 + msg=$3 + + if test -x "$GIT_DAEMON_DOCUMENT_ROOT_PATH/$repo" + then + if test -n "$do_export" + then + : >"$GIT_DAEMON_DOCUMENT_ROOT_PATH/$repo/git-daemon-export-ok" + else + rm -f "$GIT_DAEMON_DOCUMENT_ROOT_PATH/$repo/git-daemon-export-ok" + fi + fi + + test_must_fail git "$cmd" "$GIT_DAEMON_URL/$repo" 2>output && + echo "fatal: remote error: $msg: /$repo" >expect && + test_cmp expect output + ret=$? + chmod +x "$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git" + (exit $ret) +} + +msg="access denied or repository not exported" +test_expect_success 'clone non-existent' "test_remote_error clone nowhere.git '$msg'" +test_expect_success 'push disabled' "test_remote_error push repo.git '$msg'" +test_expect_success 'read access denied' "test_remote_error -x fetch repo.git '$msg'" +test_expect_success 'not exported' "test_remote_error -n fetch repo.git '$msg'" + +stop_git_daemon +start_git_daemon --informative-errors + +test_expect_success 'clone non-existent' "test_remote_error clone nowhere.git 'no such repository'" +test_expect_success 'push disabled' "test_remote_error push repo.git 'service not enabled'" +test_expect_success 'read access denied' "test_remote_error -x fetch repo.git 'no such repository'" +test_expect_success 'not exported' "test_remote_error -n fetch repo.git 'repository not exported'" + +stop_git_daemon +test_done From f6a34cfbb4314105c4dadd88eb42da26aef44dfd Mon Sep 17 00:00:00 2001 From: Clemens Buchacher Date: Sat, 7 Jan 2012 12:42:46 +0100 Subject: [PATCH 4/5] git-daemon: produce output when ready If a client tries to connect after git-daemon starts, but before it opens a listening socket, the connection will fail. Output "[PID] Ready to rumble]" after opening the socket successfully in order to inform the user that the daemon is now ready to receive connections. Signed-off-by: Clemens Buchacher Signed-off-by: Junio C Hamano --- daemon.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/daemon.c b/daemon.c index 15ce918a21..ab21e66b2f 100644 --- a/daemon.c +++ b/daemon.c @@ -1086,6 +1086,8 @@ static int serve(struct string_list *listen_addr, int listen_port, drop_privileges(cred); + loginfo("Ready to rumble"); + return service_loop(&socklist); } @@ -1270,10 +1272,8 @@ int main(int argc, char **argv) if (inetd_mode || serve_mode) return execute(); - if (detach) { + if (detach) daemonize(); - loginfo("Ready to rumble"); - } else sanitize_stdfds(); From 561b133c2c29ccf5e2aeaa5d6b1da3835e660db8 Mon Sep 17 00:00:00 2001 From: Clemens Buchacher Date: Sat, 7 Jan 2012 12:42:47 +0100 Subject: [PATCH 5/5] git-daemon tests: wait until daemon is ready In start_daemon, git-daemon is started as a background process. In theory, the tests may try to connect before the daemon had a chance to open a listening socket. Avoid this race condition by waiting for it to output "Ready to rumble". Any other output is considered an error and the test is aborted. Should git-daemon produce no output at all, lib-git-daemon would block forever. This could be fixed by introducing a timeout. On the other hand, we have no timeout for other git commands which could suffer from the same problem. Since such a mechanism adds some complexity, I have decided against it. Signed-off-by: Clemens Buchacher Signed-off-by: Junio C Hamano --- t/lib-git-daemon.sh | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh index 5e81a25942..ef2d01f369 100644 --- a/t/lib-git-daemon.sh +++ b/t/lib-git-daemon.sh @@ -23,12 +23,27 @@ start_git_daemon() { trap 'code=$?; stop_git_daemon; (exit $code); die' EXIT say >&3 "Starting git daemon ..." + mkfifo git_daemon_output git daemon --listen=127.0.0.1 --port="$LIB_GIT_DAEMON_PORT" \ --reuseaddr --verbose \ --base-path="$GIT_DAEMON_DOCUMENT_ROOT_PATH" \ "$@" "$GIT_DAEMON_DOCUMENT_ROOT_PATH" \ - >&3 2>&4 & + >&3 2>git_daemon_output & GIT_DAEMON_PID=$! + { + read line + echo >&4 "$line" + cat >&4 & + + # Check expected output + if test x"$(expr "$line" : "\[[0-9]*\] \(.*\)")" != x"Ready to rumble" + then + kill "$GIT_DAEMON_PID" + wait "$GIT_DAEMON_PID" + trap 'die' EXIT + error "git daemon failed to start" + fi + }