{"thread":{"id":"60208","subject":"[PATCH] start_command: reset disposition of all signals in child","startedAt":"2023-09-08T10:06:26Z","lastAt":"2023-09-12T03:12:43Z","messageCount":9,"participants":["Phillip Wood via GitGitGadget","Junio C Hamano","Phillip Wood","Eric Wong"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"481550","messageId":"pull.1582.git.1694167540231.gitgitgadget@gmail.com","threadId":"60208","inReplyTo":null,"subject":"[PATCH] start_command: reset disposition of all signals in child","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-09-08T10:05:40Z","receivedAt":"2023-09-08T10:06:26Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nIn order to avoid invoking a signal handler in the child process between\nfork() and exec(), start_command() blocks all signals before calling\nfork and then in the child resets the disposition of all signals that\nare not ignored to SIG_DFL before unblocking them. The exception for\nignored signals seems to been inspired by ruby's process handling[1]\nbased on the misconception that execve() will reset them to\nSIG_DFL. Unfortunately this is not the case [2] and any signals that are\nignored in the parent will default to being ignored by the program\nexecuted by start_command().\n\nWhen git ignores SIGPIPE before forking a child process it is to stop\ngit from being killed if the child exits while git is writing to the\nchild's stdin. We do not want to ignore SIGPIPE in the child. When git\nignores SIGINT and SIGQUIT before forking a child process it is to stop\ngit from being killed if the user interrupts the child with Ctrl-C or\nCtrl-\\ we do not want the child to ignore those signals [3].\nFortunately the fix is easy - reset the disposition of all signals\nregardless of their previous disposition.\n\n[1] https://lore.kernel.org/git/20170413211428.GA5961@whir/\n\n[2] The man page for execve(2) states:\n\n POSIX.1 specifies that the dispositions of any signals that are\n\tignored or set to the default are left unchanged. POSIX.1\n\tspecifies one exception: if SIGCHLD is being ignored, then an\n\timplementation may leave the disposition unchanged or reset it\n\tto the default; Linux does the former.\n\n Page 579 of \"The Linux Programming Interface\" notes:\n\n SUSv3 recommends that signals should not be blokced or ignored\n\tacross an exec() of an arbitrary program. Here, \"arbitrary\"\n\tmeans a program that we did not write.\n\n[3] This is really a work-around for not moving the child into its own\n process group and changing the foreground process group of the\n controlling terminal.\n\nSigned-off-by: Phillip Wood \n---\n start_command: reset disposition of all signals in child\n \n As an aside I wonder if we ought to add an option to ignore SIGPIPE when\n stdin is redirected and possibly turn it on by default.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1582%2Fphillipwood%2Fstart-command-dont-ignore-signals-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1582/phillipwood/start-command-dont-ignore-signals-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1582\n\n run-command.c | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex a558042c876..765775a1f42 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -823,11 +823,8 @@ fail_pipe:\n \t\t * restore default signal handlers here, in case\n \t\t * we catch a signal right before execve below\n \t\t */\n-\t\tfor (sig = 1; sig < NSIG; sig++) {\n-\t\t\t/* ignored signals get reset to SIG_DFL on execve */\n-\t\t\tif (signal(sig, SIG_DFL) == SIG_IGN)\n-\t\t\t\tsignal(sig, SIG_IGN);\n-\t\t}\n+\t\tfor (sig = 1; sig < NSIG; sig++)\n+\t\t\tsignal(sig, SIG_DFL);\n \n \t\tif (sigprocmask(SIG_SETMASK, &as.old, NULL) != 0)\n \t\t\tchild_die(CHILD_ERR_SIGPROCMASK);\n\nbase-commit: 1fc548b2d6a3596f3e1c1f8b1930d8dbd1e30bf3\n-- \ngitgitgadget\n"},{"id":"481555","messageId":"xmqqedj8vfht.fsf@gitster.g","threadId":"60208","inReplyTo":"pull.1582.git.1694167540231.gitgitgadget@gmail.com","subject":"Re: [PATCH] start_command: reset disposition of all signals in child","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-08T15:42:06Z","receivedAt":"2023-09-08T15:42:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" writes:\n\n> [3] This is really a work-around for not moving the child into its own\n> process group and changing the foreground process group of the\n> controlling terminal.\n\nI am puzzled, as I somehow thought that \"does the user conceive a\nsubprocess as external and different-from-git entity, or is it\nmerely an implementation detail? many use of subprocesses in our\ncodebase, it is the latter.\" from Peff was a good argument against\nsuch isolation between spawning \"git\" and spawned subprocesses.\n"},{"id":"481557","messageId":"ba69ab35-3204-4360-a36d-3253680b2479@gmail.com","threadId":"60208","inReplyTo":"xmqqedj8vfht.fsf@gitster.g","subject":"Re: [PATCH] start_command: reset disposition of all signals in child","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-09-08T15:53:21Z","receivedAt":"2023-09-08T15:53:26Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 08/09/2023 16:42, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" writes:\n> \n>> [3] This is really a work-around for not moving the child into its own\n>> process group and changing the foreground process group of the\n>> controlling terminal.\n> \n> I am puzzled, as I somehow thought that \"does the user conceive a\n> subprocess as external and different-from-git entity, or is it\n> merely an implementation detail? many use of subprocesses in our\n> codebase, it is the latter.\" from Peff was a good argument against\n> such isolation between spawning \"git\" and spawned subprocesses.\n\nIt is and in those cases we do not ignore SIGINT and SIGQUIT in the \nparent when we fork the subprocess. What I was trying to say is that in \nthe few cases where we do ignore SIGINT and SIGQUIT in the parent when \nwe fork a subprocess we're working round the child being in the same \nprocess group at the parent.\n\nBest Wishes\n\nPhillip\n"},{"id":"481559","messageId":"xmqqmsxwtyy3.fsf@gitster.g","threadId":"60208","inReplyTo":"ba69ab35-3204-4360-a36d-3253680b2479@gmail.com","subject":"Re: [PATCH] start_command: reset disposition of all signals in child","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-08T16:24:52Z","receivedAt":"2023-09-08T16:26:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> On 08/09/2023 16:42, Junio C Hamano wrote:\n>> \"Phillip Wood via GitGitGadget\" writes:\n>> \n>>> [3] This is really a work-around for not moving the child into its own\n>>> process group and changing the foreground process group of the\n>>> controlling terminal.\n>> I am puzzled, as I somehow thought that \"does the user conceive a\n>> subprocess as external and different-from-git entity, or is it\n>> merely an implementation detail? many use of subprocesses in our\n>> codebase, it is the latter.\" from Peff was a good argument against\n>> such isolation between spawning \"git\" and spawned subprocesses.\n>\n> It is and in those cases we do not ignore SIGINT and SIGQUIT in the\n> parent when we fork the subprocess. What I was trying to say is that\n> in the few cases where we do ignore SIGINT and SIGQUIT in the parent\n> when we fork a subprocess we're working round the child being in the\n> same process group at the parent.\n\nHmph, but picking a few grep hits for 'sigchain_push.*SIG_IGN' at\nrandom, the typical pattern seem to be (this example was taken from\nbuiltin/receive-pack.c):\n\n\tcode = start_command(&proc);\n\tif (code) {\n\t\t...\n\t\treturn code;\n\t}\n\tsigchain_push(SIGPIPE, SIG_IGN);\n\twhile (1) {\n\t\t...\n\t}\n\tclose(proc.in);\n\tsigchain_pop(SIGPIPE);\n\treturn finish_command(&proc);\n\nThe way we spawn an editor in editor.c looks the same:\n\n\t\tp.use_shell = 1;\n\t\tp.trace2_child_class = \"editor\";\n\t\tif (start_command(&p) < 0) {\n\t\t\tstrbuf_release(&realpath);\n\t\t\treturn error(\"unable to start editor '%s'\", editor);\n\t\t}\n\n\t\tsigchain_push(SIGINT, SIG_IGN);\n\t\tsigchain_push(SIGQUIT, SIG_IGN);\n\t\tret = finish_command(&p);\n\nIOW, we do not ignore then spawn. We spawn and ignore only in the\nparent, so there shouldn't be any reason to worry about our child\ninheriting the \"we the parent git process do not want to be killed\nby \\C-c\" settings, should there?\n\nI have a vague recollection that the \"propagate what was already\nignored to be ignored down to the child, too\" was not about signals\nwe ignored, but inherited from the end-user who started git with\ncertain signals ignored, but it is so old a piece of code that the\ndetails of the rationale escapes me.\n"},{"id":"481562","messageId":"376d3ea0-a3eb-4b25-8bf2-ca40c4699e26@gmail.com","threadId":"60208","inReplyTo":"xmqqmsxwtyy3.fsf@gitster.g","subject":"Re: [PATCH] start_command: reset disposition of all signals in child","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-09-08T16:43:33Z","receivedAt":"2023-09-08T16:43:37Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 08/09/2023 17:24, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>> On 08/09/2023 16:42, Junio C Hamano wrote:\n>>> \"Phillip Wood via GitGitGadget\" writes:\n>>>\n>>>> [3] This is really a work-around for not moving the child into its own\n>>>> process group and changing the foreground process group of the\n>>>> controlling terminal.\n>>> I am puzzled, as I somehow thought that \"does the user conceive a\n>>> subprocess as external and different-from-git entity, or is it\n>>> merely an implementation detail? many use of subprocesses in our\n>>> codebase, it is the latter.\" from Peff was a good argument against\n>>> such isolation between spawning \"git\" and spawned subprocesses.\n>>\n>> It is and in those cases we do not ignore SIGINT and SIGQUIT in the\n>> parent when we fork the subprocess. What I was trying to say is that\n>> in the few cases where we do ignore SIGINT and SIGQUIT in the parent\n>> when we fork a subprocess we're working round the child being in the\n>> same process group at the parent.\n> \n> Hmph, but picking a few grep hits for 'sigchain_push.*SIG_IGN' at\n> random, the typical pattern seem to be (this example was taken from\n> builtin/receive-pack.c):\n> \n> \tcode = start_command(&proc);\n> \tif (code) {\n> \t\t...\n> \t\treturn code;\n> \t}\n> \tsigchain_push(SIGPIPE, SIG_IGN);\n> \twhile (1) {\n> \t\t...\n> \t}\n> \tclose(proc.in);\n> \tsigchain_pop(SIGPIPE);\n> \treturn finish_command(&proc);\n> \n> The way we spawn an editor in editor.c looks the same:\n> \n> \t\tp.use_shell = 1;\n> \t\tp.trace2_child_class = \"editor\";\n> \t\tif (start_command(&p) < 0) {\n> \t\t\tstrbuf_release(&realpath);\n> \t\t\treturn error(\"unable to start editor '%s'\", editor);\n> \t\t}\n> \n> \t\tsigchain_push(SIGINT, SIG_IGN);\n> \t\tsigchain_push(SIGQUIT, SIG_IGN);\n> \t\tret = finish_command(&p);\n> \n> IOW, we do not ignore then spawn. We spawn and ignore only in the\n> parent, so there shouldn't be any reason to worry about our child\n> inheriting the \"we the parent git process do not want to be killed\n> by \\C-c\" settings, should there?\n\nOh I should have looked more carefully at the existing uses. It looks \nlike it is only my sequencer patch that does\n\n\tsigchain_push(SIGINT, SIG_IGN);\n\tsigchain_push(SIGQUIT, SIG_IGN);\n\tres = run_command(...);\n\nIn that case the existing behavior is a problem but maybe I should \nchange those call sites to use start_command() and finish_command() \ninstead if we decide that other patch is a good idea.\n\n> I have a vague recollection that the \"propagate what was already\n> ignored to be ignored down to the child, too\" was not about signals\n> we ignored, but inherited from the end-user who started git with\n> certain signals ignored, but it is so old a piece of code that the\n> details of the rationale escapes me.\n\nThe comment\n\n\t/* ignored signals get reset to SIG_DFL on execve */\n\nin start_command() makes it look like the code assumes ignored signals \nwill be reset to SIG_DFL by execve() which is not what happens. Maybe \nthat comment is just wrong and there is a good reason for the current \nbehavior.\n\nBest Wishes\n\nPhillip\n"},{"id":"481564","messageId":"xmqq8r9gtviv.fsf@gitster.g","threadId":"60208","inReplyTo":"376d3ea0-a3eb-4b25-8bf2-ca40c4699e26@gmail.com","subject":"Re: [PATCH] start_command: reset disposition of all signals in child","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-08T17:38:48Z","receivedAt":"2023-09-08T17:38:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Oh I should have looked more carefully at the existing uses. It looks\n> like it is only my sequencer patch that does\n>\n> \tsigchain_push(SIGINT, SIG_IGN);\n> \tsigchain_push(SIGQUIT, SIG_IGN);\n> \tres = run_command(...);\n\nHmph, does it mean this patch would become unnecessary, once you fix\nthe above sequence to follow the pattern \"to spawn and then ignore\"?\n"},{"id":"481574","messageId":"20230908195754.M395591@dcvr","threadId":"60208","inReplyTo":"pull.1582.git.1694167540231.gitgitgadget@gmail.com","subject":"Re: [PATCH] start_command: reset disposition of all signals in child","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2023-09-08T19:57:54Z","receivedAt":"2023-09-08T19:58:17Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Phillip Wood via GitGitGadget wrote:\n> From: Phillip Wood \n> \n> In order to avoid invoking a signal handler in the child process between\n> fork() and exec(), start_command() blocks all signals before calling\n> fork and then in the child resets the disposition of all signals that\n> are not ignored to SIG_DFL before unblocking them. The exception for\n> ignored signals seems to been inspired by ruby's process handling[1]\n> based on the misconception that execve() will reset them to\n> SIG_DFL. Unfortunately this is not the case [2] and any signals that are\n> ignored in the parent will default to being ignored by the program\n> executed by start_command().\n> \n> When git ignores SIGPIPE before forking a child process it is to stop\n> git from being killed if the child exits while git is writing to the\n> child's stdin. We do not want to ignore SIGPIPE in the child. When git\n> ignores SIGINT and SIGQUIT before forking a child process it is to stop\n> git from being killed if the user interrupts the child with Ctrl-C or\n> Ctrl-\\ we do not want the child to ignore those signals [3].\n> Fortunately the fix is easy - reset the disposition of all signals\n> regardless of their previous disposition.\n> \n> [1] https://lore.kernel.org/git/20170413211428.GA5961@whir/\n> \n> [2] The man page for execve(2) states:\n> \n> POSIX.1 specifies that the dispositions of any signals that are\n> \tignored or set to the default are left unchanged. POSIX.1\n> \tspecifies one exception: if SIGCHLD is being ignored, then an\n> \timplementation may leave the disposition unchanged or reset it\n> \tto the default; Linux does the former.\n\nYeah, the old code seems like an error on my part. Oops :x\n\n> diff --git a/run-command.c b/run-command.c\n> index a558042c876..765775a1f42 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -823,11 +823,8 @@ fail_pipe:\n> \t\t * restore default signal handlers here, in case\n> \t\t * we catch a signal right before execve below\n> \t\t */\n> -\t\tfor (sig = 1; sig < NSIG; sig++) {\n> -\t\t\t/* ignored signals get reset to SIG_DFL on execve */\n> -\t\t\tif (signal(sig, SIG_DFL) == SIG_IGN)\n> -\t\t\t\tsignal(sig, SIG_IGN);\n> -\t\t}\n> +\t\tfor (sig = 1; sig < NSIG; sig++)\n> +\t\t\tsignal(sig, SIG_DFL);\n\nLooks good to me and matches what I did in some other (A)GPL-3\nprojects, actually.\n\nAcked-by: Eric Wong \n"},{"id":"481704","messageId":"c64cd85f-f14c-46b7-a0d3-b8e0bfc60053@gmail.com","threadId":"60208","inReplyTo":"xmqq8r9gtviv.fsf@gitster.g","subject":"Re: [PATCH] start_command: reset disposition of all signals in child","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-09-11T09:50:33Z","receivedAt":"2023-09-11T21:39:23Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 08/09/2023 18:38, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>> Oh I should have looked more carefully at the existing uses. It looks\n>> like it is only my sequencer patch that does\n>>\n>> \tsigchain_push(SIGINT, SIG_IGN);\n>> \tsigchain_push(SIGQUIT, SIG_IGN);\n>> \tres = run_command(...);\n> \n> Hmph, does it mean this patch would become unnecessary, once you fix\n> the above sequence to follow the pattern \"to spawn and then ignore\"?\n\nYes, sorry for the confusion. There are a couple of things that I think \nwe should address though. Firstly we should change the comment in \nrun-command which says execve() resets ignored signals to SIG_DFL to say \nsomething like\n\n\tPreserve the set of ignored signals so that running git via a\n\twrapper like nohup works as the user expects\n\nThe other thing is that we have some instances where we ignore SIGPIPE \nbefore calling start_command() which means we're ignoring it in the \nchild process as well. For example in gpg-interface.c we have\n\n\tsigchain_push(SIGPIPE, SIG_IGN);\n\tret = pipe_command(&gpg, sigc->payload, sigc->payload_len, &gpg_stdout, 0,\n\t\t\t &gpg_stderr, 0);\n\tsigchain_pop(SIGPIPE);\n\nTo fix that one we'd need to change pipe_command() to ignore SIGPIPE \nafter calling start_command() or add a flag to struct child_process to \ndo to that.\n\nAnother example is in upload-pack.c\n\n\t/*\n\t * If the next rev-list --stdin encounters an unknown commit,\n\t * it terminates, which will cause SIGPIPE in the write loop\n\t * below.\n\t */\n\tsigchain_push(SIGPIPE, SIG_IGN);\n\n\tif (start_command(cmd))\n\t\tgoto error;\n\nrev-list does not check for errors when writing to stdout unless \nGIT_FLUSH is set in the environment so if parent process exits early \nrev-list will keep going until it thinks it has printed everything.\n\nI think adding a flag to struct child_process to ignore SIGPIPE in the \nparent is probably the best way to avoid problems like this.\n\nBest Wishes\n\nPhillip\n"},{"id":"481736","messageId":"xmqqa5tstkqv.fsf@gitster.g","threadId":"60208","inReplyTo":"c64cd85f-f14c-46b7-a0d3-b8e0bfc60053@gmail.com","subject":"Re: [PATCH] start_command: reset disposition of all signals in child","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-11T22:20:40Z","receivedAt":"2023-09-12T03:12:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> \tPreserve the set of ignored signals so that running git via a\n> \twrapper like nohup works as the user expects\n\nOK.\n\n> The other thing is that we have some instances where we ignore SIGPIPE\n> before calling start_command() which means we're ignoring it in the\n> child process as well. For example in gpg-interface.c we have\n> ...\n> rev-list does not check for errors when writing to stdout unless\n> GIT_FLUSH is set in the environment so if parent process exits early\n> rev-list will keep going until it thinks it has printed everything.\n\nAhh, yeah, that is bad. Thanks for pointing them out.\n\n> I think adding a flag to struct child_process to ignore SIGPIPE in the\n> parent is probably the best way to avoid problems like this.\n\nOK. SIGPIPE is special enough that it may deserve to be singled\nout. A pager reading from us quitting does not have to be \"we\nwanted to write more but got an error\" but just \"ok, if the user\ndoes not want any more output, that is fine\".\n\nTHanks.\n\n"}]}