{"thread":{"id":"25476","subject":"git subcommand sigint gotcha","startedAt":"2010-10-19T04:53:00Z","lastAt":"2010-10-19T21:07:53Z","messageCount":10,"participants":["Joey Hess","Dmitry Potapov","Jeff King","Jonathan Nieder","Jakub Narebski"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"153764","messageId":"20101019045300.GA18043@gnu.kitenet.net","threadId":"25476","inReplyTo":null,"subject":"git subcommand sigint gotcha","fromName":"Joey Hess","fromEmail":"joey@kitenet.net","sentAt":"2010-10-19T04:53:00Z","receivedAt":"2010-10-19T04:53:00Z","isPatch":false,"sender":{"key":"joey@kitenet.net","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"I was trying to write a git subcommand, and I noticed that if I ctrl-c'd\nit, git would return, but leave the subcommand running in the\nbackground.\n\nYou can see the problem with this test case. \n\n#!/usr/bin/perl\nprint \"first sleep...\\n\";\n$ret=system(\"sleep\", \"1m\");\nprint \"second sleep...\\n\";\nsystem(\"sleep\", \"1s\");\nprint \"done with second sleep\\n\";\n\nIf you put it in path named git-sleep, then run \"git sleep\" and press ctrl-c,\nit keeps running:\n\njoey@gnu:~>git sleep\nfirst sleep...\n^Csecond sleep...\njoey@gnu:~>done with second sleep\n\nSo what's going on? Well, perl's system() blocks sigint while the child\nprocess is running. So if you run this as git-sleep, and press ctrl-c,\nit will continue on to the second sleep. If the code above checked the\nreturn status of system() it could detect that it was killed by SIGINT\nand itself exit.\n\nWhat I don't understand is, why does git not wait() on the subcommand it\nran? Any subcommand that forgets to check exit codes is liable to exhibit\nthis weird behavior sometimes. \n\nIe, imagine the subcommand was running something like \n\"git config --get core.bare\" instead of sleep. \nIt'd be easy to forget to check the exit status of that for a SIGINT; if\nthe user ctrl-c'd at just the right instant, weird things would happen.\n\n-- \nsee shy jo\n"},{"id":"153779","messageId":"AANLkTi=tvyzyz2xpezufHLFc44HDbtMibkhNEvYxPB2g@mail.gmail.com","threadId":"25476","inReplyTo":"20101019045300.GA18043@gnu.kitenet.net","subject":"Re: git subcommand sigint gotcha","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2010-10-19T09:55:29Z","receivedAt":"2010-10-19T09:55:29Z","isPatch":false,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Tue, Oct 19, 2010 at 8:53 AM, Joey Hess <joey@kitenet.net> wrote:\n> I was trying to write a git subcommand, and I noticed that if I ctrl-c'd\n> it, git would return, but leave the subcommand running in the\n> background.\n\nIt looks like this regression was introduced in v1.6.4, when Jeff tried to fix\none serious issue with a pager. I have bisected the problem to this commit:\nhttp://git.kernel.org/?p=git/git.git;a=commit;h=d8e96fd86d415554a9c2e09ffb929a9e22fdad25\n\nDmitry\n"},{"id":"153782","messageId":"20101019115943.GA8065@dpotapov.dyndns.org","threadId":"25476","inReplyTo":"AANLkTi=tvyzyz2xpezufHLFc44HDbtMibkhNEvYxPB2g@mail.gmail.com","subject":"RFC: [PATCH] ignore SIGINT&QUIT while waiting for external command","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2010-10-19T11:59:43Z","receivedAt":"2010-10-19T11:59:43Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"Before git 1.6.4, we used execvp to run external git dashed commands,\nthus git did not return until this command is finished. With switching to\nrun_command (which was necessary to fix a pager issue; see d8e96fd86d4),\nCTRL-C could cause that git returned before than the git dashed command is\nfinished.\n\nThe solution is to disable SIGINT and SIGQUIT as it is normally done by\nsystem(). Disabling these signals is done only when silent_exec_failure\nis set, which means that the current process is used as a proxy to run\nanother command.\n\nSigned-off-by: Dmitry Potapov <dpotapov@gmail.com>\n---\n run-command.c |   19 +++++++++++++++++++\n 1 files changed, 19 insertions(+), 0 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 2a1041e..14af035 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -93,6 +93,10 @@ static inline void set_cloexec(int fd)\n \t\tfcntl(fd, F_SETFD, flags | FD_CLOEXEC);\n }\n \n+#ifndef WIN32\n+static sighandler_t sigint, sigquit;\n+#endif\n+\n static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)\n {\n \tint status, code = -1;\n@@ -102,6 +106,13 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)\n \twhile ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)\n \t\t;\t/* nothing */\n \n+#ifndef WIN32\n+\tif (silent_exec_failure) {\n+\t\t/* Restore signal handlers */\n+\t\tsignal(SIGINT, sigint);\n+\t\tsignal(SIGQUIT, sigquit);\n+\t}\n+#endif\n \tif (waiting < 0) {\n \t\tfailed_errno = errno;\n \t\terror(\"waitpid for %s failed: %s\", argv0, strerror(errno));\n@@ -202,8 +213,16 @@ fail_pipe:\n \t\tnotify_pipe[0] = notify_pipe[1] = -1;\n \n \tfflush(NULL);\n+\tif (cmd->silent_exec_failure) {\n+\t\tsigint = signal(SIGINT, SIG_IGN);\n+\t\tsigquit = signal(SIGQUIT, SIG_IGN);\n+\t}\n \tcmd->pid = fork();\n \tif (!cmd->pid) {\n+\t\tif (cmd->silent_exec_failure) {\n+\t\t\tsignal(SIGINT, sigint);\n+\t\t\tsignal(SIGQUIT, sigquit);\n+\t\t}\n \t\t/*\n \t\t * Redirect the channel to write syscall error messages to\n \t\t * before redirecting the process's stderr so that all die()\n-- \n1.7.3.1\n"},{"id":"153784","messageId":"20101019133236.GA804@sigill.intra.peff.net","threadId":"25476","inReplyTo":"20101019115943.GA8065@dpotapov.dyndns.org","subject":"Re: RFC: [PATCH] ignore SIGINT&QUIT while waiting for external command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-10-19T13:32:36Z","receivedAt":"2010-10-19T13:32:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 19, 2010 at 03:59:43PM +0400, Dmitry Potapov wrote:\n\n> The solution is to disable SIGINT and SIGQUIT as it is normally done by\n> system(). Disabling these signals is done only when silent_exec_failure\n> is set, which means that the current process is used as a proxy to run\n> another command.\n\nI don't understand why we would only do it for silent_exec_failure. You\nclaim that flag means that the current process is a proxy for another\ncommand, but:\n\n  1. Is that really the case, or do the two things just happen to\n     coincide in the current codebase?\n\n  2. Why do we want to do it only for the proxy-command case? If I have\n     a long-running external diff or merge helper, for example, what\n     should happen on SIGINT? Should we exit with the child still\n     potentially running, or should we actually be reaping the child\n     properly?\n\n> +\tif (cmd->silent_exec_failure) {\n> +\t\tsigint = signal(SIGINT, SIG_IGN);\n> +\t\tsigquit = signal(SIGQUIT, SIG_IGN);\n> +\t}\n>  \tcmd->pid = fork();\n>  \tif (!cmd->pid) {\n> +\t\tif (cmd->silent_exec_failure) {\n> +\t\t\tsignal(SIGINT, sigint);\n> +\t\t\tsignal(SIGQUIT, sigquit);\n> +\t\t}\n\nHow does this interact with the sigchain code? If I do:\n\n  start_command(...);\n  sigchain_push(...);\n  finish_command(...);\n\nwe will overwrite the function pushed in the sigchain_push with a stale\nhandler. I think you could just replace your signal() calls with:\n\n  sigchain_push(SIGINT, SIG_IGN);\n  ...\n  sigchain_pop(SIGINT);\n\nbut I wonder if ignoring is necessarily the right thing. Shouldn't we\njust reap the child and then run the signal handler that was there\nbefore us? That means in general that we will continue to die via SIGINT\nwhen we see SIGINT. With your patch, we will ignore it and (presumably)\nend up dying with a return code indicated that the child had an error.\n\nI think both of these things are not problems for executing dashed\nexternals. But as above, I am not sure that we should be limiting this\nsignal handling to those cases.\n\n-Peff\n"},{"id":"153789","messageId":"20101019134040.GA3956@sigill.intra.peff.net","threadId":"25476","inReplyTo":"20101019133236.GA804@sigill.intra.peff.net","subject":"Re: RFC: [PATCH] ignore SIGINT&QUIT while waiting for external command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-10-19T13:40:40Z","receivedAt":"2010-10-19T13:40:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 19, 2010 at 09:32:36AM -0400, Jeff King wrote:\n\n> How does this interact with the sigchain code? If I do:\n> \n>   start_command(...);\n>   sigchain_push(...);\n>   finish_command(...);\n> \n> we will overwrite the function pushed in the sigchain_push with a stale\n> handler. I think you could just replace your signal() calls with:\n> \n>   sigchain_push(SIGINT, SIG_IGN);\n>   ...\n>   sigchain_pop(SIGINT);\n\nWhich, FWIW, would look like this:\n\ndiff --git a/run-command.c b/run-command.c\nindex 2a1041e..24e0f46 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1,6 +1,7 @@\n #include \"cache.h\"\n #include \"run-command.h\"\n #include \"exec_cmd.h\"\n+#include \"sigchain.h\"\n \n static inline void close_pair(int fd[2])\n {\n@@ -102,6 +103,9 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)\n \twhile ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)\n \t\t;\t/* nothing */\n \n+\tsigchain_pop(SIGINT);\n+\tsigchain_pop(SIGQUIT);\n+\n \tif (waiting < 0) {\n \t\tfailed_errno = errno;\n \t\terror(\"waitpid for %s failed: %s\", argv0, strerror(errno));\n@@ -202,8 +206,12 @@ fail_pipe:\n \t\tnotify_pipe[0] = notify_pipe[1] = -1;\n \n \tfflush(NULL);\n+\tsigchain_push(SIGINT, SIG_IGN);\n+\tsigchain_push(SIGQUIT, SIG_IGN);\n \tcmd->pid = fork();\n \tif (!cmd->pid) {\n+\t\tsigchain_pop(SIGINT);\n+\t\tsigchain_pop(SIGQUIT);\n \t\t/*\n \t\t * Redirect the channel to write syscall error messages to\n \t\t * before redirecting the process's stderr so that all die()\n"},{"id":"153797","messageId":"20101019163124.GB8065@dpotapov.dyndns.org","threadId":"25476","inReplyTo":"20101019133236.GA804@sigill.intra.peff.net","subject":"Re: RFC: [PATCH] ignore SIGINT&QUIT while waiting for external command","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2010-10-19T16:31:24Z","receivedAt":"2010-10-19T16:31:24Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Tue, Oct 19, 2010 at 09:32:36AM -0400, Jeff King wrote:\n> \n>   2. Why do we want to do it only for the proxy-command case? If I have\n>      a long-running external diff or merge helper, for example, what\n>      should happen on SIGINT? Should we exit with the child still\n>      potentially running, or should we actually be reaping the child\n>      properly?\n\nProbably, it should be done in other cases too. However, I am not sure\nif it should be done unconditionally. For instance, when we run a pager,\nI don't think we should ignore the signals just because we started a\npager.\n\nI agree that silent_exec_failure is not the best flag for that -- I was\njust trying to make minimal changes to the existing behavior, and if\nthis flag is set, you seem always want to ignore these signals, but\nthere are some other cases too as you pointed above.\n\nNow, I think we should always ignore these signals when run_command() is\nused (similar to system()), but do not mask signals if start_command()\nis used (or make it optional by adding a new flag).\n\n> \n> we will overwrite the function pushed in the sigchain_push with a stale\n> handler. I think you could just replace your signal() calls with:\n> \n>   sigchain_push(SIGINT, SIG_IGN);\n>   ...\n>   sigchain_pop(SIGINT);\n\nYes, it is certainly better. I was not aware about these functions.\n\n\nDmitry\n"},{"id":"153817","messageId":"20101019191638.GI25139@burratino","threadId":"25476","inReplyTo":"20101019134040.GA3956@sigill.intra.peff.net","subject":"Re: RFC: [PATCH] ignore SIGINT&QUIT while waiting for external command","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-19T19:16:38Z","receivedAt":"2010-10-19T19:16:38Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Tue, Oct 19, 2010 at 09:32:36AM -0400, Jeff King wrote:\n\n>> I think you could just replace your signal() calls with:\n>> \n>>   sigchain_push(SIGINT, SIG_IGN);\n>>   ...\n>>   sigchain_pop(SIGINT);\n>\n> Which, FWIW, would look like this:\n\nSomething in this direction on top?\n\nI think sigchain_push ought to accept a context object.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\ndiff --git a/run-command.c b/run-command.c\nindex 24e0f46..efdac84 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -103,6 +103,7 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)\n \twhile ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)\n \t\t;\t/* nothing */\n \n+\tthe_child = NULL;\n \tsigchain_pop(SIGINT);\n \tsigchain_pop(SIGQUIT);\n \n@@ -139,6 +140,19 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)\n \treturn code;\n }\n \n+static struct child_process *the_child;\n+\n+static void interrupted_with_child(int sig)\n+{\n+\tif (the_child && the_child->pid > 0) {\n+\t\twhile ((waiting = waitpid(pid, NULL, 0)) < 0 && errno == EINTR)\n+\t\t\t;\t/* nothing */\n+\t\tthe_child = NULL;\n+\t}\n+\tsigchain_pop(sig);\n+\traise(sig);\n+}\n+\n int start_command(struct child_process *cmd)\n {\n \tint need_in, need_out, need_err;\n@@ -206,8 +220,11 @@ fail_pipe:\n \t\tnotify_pipe[0] = notify_pipe[1] = -1;\n \n \tfflush(NULL);\n-\tsigchain_push(SIGINT, SIG_IGN);\n-\tsigchain_push(SIGQUIT, SIG_IGN);\n+\tif (the_child)\n+\t\tdie(\"What?  _Two_ children?\");\n+\tthe_child = cmd;\n+\tsigchain_push(SIGINT, interrupted_with_child);\n+\tsigchain_push(SIGQUIT, interrupted_with_child);\n \tcmd->pid = fork();\n \tif (!cmd->pid) {\n \t\tsigchain_pop(SIGINT);\n"},{"id":"153818","messageId":"20101019195022.GA7287@sigill.intra.peff.net","threadId":"25476","inReplyTo":"20101019191638.GI25139@burratino","subject":"Re: RFC: [PATCH] ignore SIGINT&QUIT while waiting for external command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-10-19T19:50:22Z","receivedAt":"2010-10-19T19:50:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 19, 2010 at 02:16:38PM -0500, Jonathan Nieder wrote:\n\n> I think sigchain_push ought to accept a context object.\n\nBut signal() doesn't, so we would have to install a wrapper function\nthat gets the signal and calls the sigchain_pushed callback with the\ncontext object. But we can't always install the wrapper. We need to\ncheck for SIG_IGN and SIG_DFL, and literally install those.\n\nSo I think it's do-able, but I tried to keep the original sigchain as\nsimple as possible.\n\n> +static void interrupted_with_child(int sig)\n> +{\n> +\tif (the_child && the_child->pid > 0) {\n> +\t\twhile ((waiting = waitpid(pid, NULL, 0)) < 0 && errno == EINTR)\n> +\t\t\t;\t/* nothing */\n> +\t\tthe_child = NULL;\n> +\t}\n> +\tsigchain_pop(sig);\n> +\traise(sig);\n> +}\n> +\n>  int start_command(struct child_process *cmd)\n>  {\n>  \tint need_in, need_out, need_err;\n> @@ -206,8 +220,11 @@ fail_pipe:\n>  \t\tnotify_pipe[0] = notify_pipe[1] = -1;\n>  \n>  \tfflush(NULL);\n> -\tsigchain_push(SIGINT, SIG_IGN);\n> -\tsigchain_push(SIGQUIT, SIG_IGN);\n> +\tif (the_child)\n> +\t\tdie(\"What?  _Two_ children?\");\n> +\tthe_child = cmd;\n\nYuck. You can get around that by pushing onto a linked list of children,\nthough.\n\nThinking about it more, though, I don't think we do necessarily want to\nalways wait for the child. There are really two main types of\nrun_command's we do:\n\n  1. The run command is basically the new process. In an ideal world, we\n     would exec into it, but we need the parent to hang around to do\n     some kind of bookkeeping (like waiting for the pager to exit).\n\n     E.g., running external dashed commands.\n\n  2. We are running the command, and if we are killed, the command\n     should go away too (because its point in running is to give us some\n     information).\n\n     E.g., running textconv filters.\n\nAnd there are a few instances that don't fall into either category\n(e.g., running the pager).\n\nIn case (1), we probably want to SIG_IGN, wait for the command to\nfinish, and then die with its exit code. If we do it right, the fact\nthat _it_ was killed by signal will be propagated, and the fact that we\nweren't will be irrelevant.\n\nIn case (2), we probably want to keep a linked list of \"expendable\"\nprocesses, and on signal death and atexit, go through the list and make\nsure all are dead. This is how we handle tempfiles already in diff.c.\n\nGiven that there is only really one instance of (1), we can just code it\nthere. For (2), there are many such callers, but I don't know that the\nmechanism necessarily needs to be included as part of run_command. A\nseparate module to manage the list and set up the signal handler would\nbe fine (though there is a race between fork() and signal death, so it\nperhaps pays to get the newly created pid on the \"expendable\" list as\nsoon as possible, which may mean cooperating from run_command).\n\n-Peff\n"},{"id":"153824","messageId":"m3fww1lwqw.fsf@localhost.localdomain","threadId":"25476","inReplyTo":"20101019191638.GI25139@burratino","subject":"Re: RFC: [PATCH] ignore SIGINT&QUIT while waiting for external command","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-10-19T21:06:03Z","receivedAt":"2010-10-19T21:06:03Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> -\tsigchain_push(SIGINT, SIG_IGN);\n> -\tsigchain_push(SIGQUIT, SIG_IGN);\n> +\tif (the_child)\n> +\t\tdie(\"What?  _Two_ children?\");\n> +\tthe_child = cmd;\n> +\tsigchain_push(SIGINT, interrupted_with_child);\n> +\tsigchain_push(SIGQUIT, interrupted_with_child);\n\nPlease, don't do this.  It is almost as bad as error message as \n\"You don't exist.  Go away\".\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"153826","messageId":"20101019210753.GB32029@burratino","threadId":"25476","inReplyTo":"m3fww1lwqw.fsf@localhost.localdomain","subject":"Re: RFC: [PATCH] ignore SIGINT&QUIT while waiting for external command","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-19T21:07:53Z","receivedAt":"2010-10-19T21:07:53Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jakub Narebski wrote:\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n\n>> -\tsigchain_push(SIGINT, SIG_IGN);\n>> -\tsigchain_push(SIGQUIT, SIG_IGN);\n>> +\tif (the_child)\n>> +\t\tdie(\"What?  _Two_ children?\");\n>> +\tthe_child = cmd;\n>> +\tsigchain_push(SIGINT, interrupted_with_child);\n>> +\tsigchain_push(SIGQUIT, interrupted_with_child);\n>\n> Please, don't do this.  It is almost as bad as error message as \n> \"You don't exist.  Go away\".\n\nHopefully it was clear that the behavior (erroring out) is as\nunacceptable as the message.\n"}]}