From: Phillip Wood Date: Mon, 11 Sep 2023 09:50:33 GMT Subject: Re: [PATCH] start_command: reset disposition of all signals in child Message-ID: In-Reply-To: On 08/09/2023 18:38, Junio C Hamano wrote: > Phillip Wood writes: > >> Oh I should have looked more carefully at the existing uses. It looks >> like it is only my sequencer patch that does >> >> sigchain_push(SIGINT, SIG_IGN); >> sigchain_push(SIGQUIT, SIG_IGN); >> res = run_command(...); > > Hmph, does it mean this patch would become unnecessary, once you fix > the above sequence to follow the pattern "to spawn and then ignore"? Yes, sorry for the confusion. There are a couple of things that I think we should address though. Firstly we should change the comment in run-command which says execve() resets ignored signals to SIG_DFL to say something like Preserve the set of ignored signals so that running git via a wrapper like nohup works as the user expects The other thing is that we have some instances where we ignore SIGPIPE before calling start_command() which means we're ignoring it in the child process as well. For example in gpg-interface.c we have sigchain_push(SIGPIPE, SIG_IGN); ret = pipe_command(&gpg, sigc->payload, sigc->payload_len, &gpg_stdout, 0, &gpg_stderr, 0); sigchain_pop(SIGPIPE); To fix that one we'd need to change pipe_command() to ignore SIGPIPE after calling start_command() or add a flag to struct child_process to do to that. Another example is in upload-pack.c /* * If the next rev-list --stdin encounters an unknown commit, * it terminates, which will cause SIGPIPE in the write loop * below. */ sigchain_push(SIGPIPE, SIG_IGN); if (start_command(cmd)) goto error; rev-list does not check for errors when writing to stdout unless GIT_FLUSH is set in the environment so if parent process exits early rev-list will keep going until it thinks it has printed everything. I think adding a flag to struct child_process to ignore SIGPIPE in the parent is probably the best way to avoid problems like this. Best Wishes Phillip