[Q] what to do when waitpid() returns ECHILD under signal(SIGCHLD, SIG_IGN)?

[Q] what to do when waitpid() returns ECHILD under signal(SIGCHLD, SIG_IGN)?

[Junio C Hamano]

Somebody I met last week in Japan reported that the socks client
he uses to cross the firewall to connect to git:// port from his
company environment seems to do signal(SIGCHLD, SIG_IGN) before
spawning git.  When "git clone" is invoked this way, we get a
mysterious failure.

I can reproduce the problem without using funny socks client
like this:

        : gitster; trap '' SIGCHLD
        : gitster; git clone git://git.kernel.org/pub/scm/git/git.git/ foo.git
        error: waitpid failed (No child processes)
        fetch-pack from 'git://git.kernel.org/pub/scm/git/git.git/' failed.
        : gitster; ls foo.git
        ls: foo.git: No such file or directory

We could work this around by having signal(SIGCHLD, SIG_DFL)
upfront in git.c::main(), but I am wondering what the standard
practice for programs that use waitpid() call.  Do they protect
themselves from this in order to reliably obtain child exit
status?  Or do they simply consider it is a user error to run a
program that use waitpid() with SIGCHLD ignored?

http://www.opengroup.org/onlinepubs/009695399/functions/waitpid.html

explicitly says this is an expected behaviour, so barfing upon
ECHILD sounds like a bug on our part.

Re: [Q] what to do when waitpid() returns ECHILD under signal(SIGCHLD, SIG_IGN)?

[Linus Torvalds]

On Mon, 19 Jun 2006, Junio C Hamano wrote:
>
> Somebody I met last week in Japan reported that the socks client
> he uses to cross the firewall to connect to git:// port from his
> company environment seems to do signal(SIGCHLD, SIG_IGN) before
> spawning git.

Ok, that sounds pretty broken of it.

> We could work this around by having signal(SIGCHLD, SIG_DFL)
> upfront in git.c::main(), but I am wondering what the standard
> practice for programs that use waitpid() call.

We need the status return, so failing on getting ECHILD is absolutely the 
right thing to do, because it implies that we don't know what the status 
could have been.

So we need to reset SIGCHLD back to SIG_DFL (or catch it explicitly).

Whether we want to do that in the main() routine or when we actually do 
the fork() or whatever is a different issue.

		Linus

Re: [Q] what to do when waitpid() returns ECHILD under signal(SIGCHLD, SIG_IGN)?

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> Whether we want to do that in the main() routine or when we actually do 
> the fork() or whatever is a different issue.

I do not offhand think of a place where we do fork() but not
waitpid(), and it is very tempting to cheat and do that in the
main(), since I do not see a downside to it.

Re: [Q] what to do when waitpid() returns ECHILD under signal(SIGCHLD, SIG_IGN)?

[Linus Torvalds]

On Mon, 19 Jun 2006, Junio C Hamano wrote:
>
> Linus Torvalds <torvalds@osdl.org> writes:
> 
> > Whether we want to do that in the main() routine or when we actually do 
> > the fork() or whatever is a different issue.
> 
> I do not offhand think of a place where we do fork() but not
> waitpid(), and it is very tempting to cheat and do that in the
> main(), since I do not see a downside to it.

Yeah, it probably does make sense. That said, there are several "main()" 
functions, so you'd still end up having to verify that we catch all the 
paths.. Are all users of fork() built-in by now? 

			Linus

[PATCH] Restore SIGCHLD to SIG_DFL where we care about waitpid().

[Junio C Hamano]

It was reported that under one implementation of socks client,
"git clone" fails with "error: waitpid failed (No child
processes)", because "git" is spawned after setting SIGCHLD to
SIG_IGN.

Arguably it may be a broken setting, but we should protect
ourselves so that we can get reliable results from waitpid() for
the children we care about.

This patch resets SIGCHLD to SIG_DFL in three places:

 - connect.c::git_connect() - initiators of git native
   protocol transfer are covered with this.

 - daemon.c::main() - obviously.

 - merge-index.c::main() - obviously.

There are other programs that do fork() but do not waitpid():
http-push, imap-send.  upload-pack does not either, but in the
case of that program, each of the forked halves runs exec()
another program, so this change would not have much effect
there.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

  Linus Torvalds <torvalds@osdl.org> writes:

  > On Mon, 19 Jun 2006, Junio C Hamano wrote:
  >
  >> I do not offhand think of a place where we do fork() but not
  >> waitpid(), and it is very tempting to cheat and do that in the
  >> main(), since I do not see a downside to it.
  >
  > Yeah, it probably does make sense. That said, there are several "main()" 
  > functions, so you'd still end up having to verify that we catch all the 
  > paths.. Are all users of fork() built-in by now? 
 
  Not really.  But git native protocol initiators all use
  git_connect() so they are easy, and there are only few
  remaining ones that matter.

 connect.c     |    5 +++++
 daemon.c      |    5 +++++
 merge-index.c |    5 +++++
 3 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/connect.c b/connect.c
index 52d709e..db7342e 100644
--- a/connect.c
+++ b/connect.c
@@ -581,6 +581,11 @@ int git_connect(int fd[2], char *url, co
 	enum protocol protocol = PROTO_LOCAL;
 	int free_path = 0;
 
+	/* Without this we cannot rely on waitpid() to tell
+	 * what happened to our children.
+	 */
+	signal(SIGCHLD, SIG_DFL);
+
 	host = strstr(url, "://");
 	if(host) {
 		*host = '\0';
diff --git a/daemon.c b/daemon.c
index 2f03f99..1067004 100644
--- a/daemon.c
+++ b/daemon.c
@@ -671,6 +671,11 @@ int main(int argc, char **argv)
 	int inetd_mode = 0;
 	int i;
 
+	/* Without this we cannot rely on waitpid() to tell
+	 * what happened to our children.
+	 */
+	signal(SIGCHLD, SIG_DFL);
+
 	for (i = 1; i < argc; i++) {
 		char *arg = argv[i];
 
diff --git a/merge-index.c b/merge-index.c
index 024196e..190e12f 100644
--- a/merge-index.c
+++ b/merge-index.c
@@ -99,6 +99,11 @@ int main(int argc, char **argv)
 {
 	int i, force_file = 0;
 
+	/* Without this we cannot rely on waitpid() to tell
+	 * what happened to our children.
+	 */
+	signal(SIGCHLD, SIG_DFL);
+
 	if (argc < 3)
 		usage("git-merge-index [-o] [-q] <merge-program> (-a | <filename>*)");
 
-- 
1.4.0.g275f

Re: [PATCH] Restore SIGCHLD to SIG_DFL where we care about waitpid().

[Petr Baudis]

Dear diary, on Tue, Jun 20, 2006 at 05:11:40AM CEST, I got a letter
where Junio C Hamano <junkio@cox.net> said that...
> diff --git a/connect.c b/connect.c
> index 52d709e..db7342e 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -581,6 +581,11 @@ int git_connect(int fd[2], char *url, co
>  	enum protocol protocol = PROTO_LOCAL;
>  	int free_path = 0;
>  
> +	/* Without this we cannot rely on waitpid() to tell
> +	 * what happened to our children.
> +	 */
> +	signal(SIGCHLD, SIG_DFL);
> +
>  	host = strstr(url, "://");
>  	if(host) {
>  		*host = '\0';

It would be nice if at this point of Git development we could already
think about libification when doing things like this (I'd like to do
Git.pm as my little summer project this year). I'd make it part of the
API that the calling application must not defer SIGCHLD, and conversely
put the handler to all the main()s that reach git_connect().

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
A person is just about as big as the things that make them angry.

Re: [Q] what to do when waitpid() returns ECHILD under signal(SIGCHLD, SIG_IGN)?

[Edgar Toernig]

Junio C Hamano wrote:
>
> Somebody I met last week in Japan reported that the socks client
> he uses to cross the firewall to connect to git:// port from his
> company environment seems to do signal(SIGCHLD, SIG_IGN) before
> spawning git.

Similar problems occasionally happen with SIGPIPE.  There are apps[1]
that spawn processes with SIGPIPE set to SIG_IGN giving unexpected
results, i.e. child processes that still try to produce output
(getting EPIPE on every printf, but who checks printf errors?) even
if the pipe-reader (e.g. their parent) is long gone.

Ciao, ET.

[1] i.e. KDE had this bug - don't know if it's still there.