Skip to contentSkip to conversation

Set SA_RESTART flag on SIGCHLD handler

PATCH In review Anders Kaseorg 2 participants 6 messages Jun 21, 2012 – Jul 21, 2012 5 messages not in the archive

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): 20120319025201.GA24778@burratino

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): 20120319033751.GA25237@burratino

Anders Kaseorg

author
#4 lore raw

PATCH 1/1 Set SA_RESTART flag on SIGCHLD handler

It looks like this was never forwarded to the dash mailing list. Any thoughts from upstream?

Context: http://thread.gmane.org/gmane.comp.version-control.git.debian/147
(FTR, it did happen again.)

Anders

Quoted 25 lines · Jonathan Nieder
Quoted 3 lines · Anders Kaseorg

So I guess I’m willing to just hope for now that this doesn’t happen again. (Unless you think it’s a dash bug that EINTR isn’t handled there?)

Yes, I suspect it's a dash >= 0.5.6 bug. On initialization dash sets up a signal handler for SIGCHLD without the SA_RESTART flag, which can cause other dash code to fail at seemingly random times. Similar problems would arise when scripts set up traps.

src/trap.c +1 -1
index 17316c95..cd99596f 100644
@@ -259,7 +259,7 @@ setsignal(int signo)
 		act.sa_handler = SIG_DFL;
 	}
 	*t = action;
-	act.sa_flags = 0;
+	act.sa_flags = SA_RESTART;
 	sigfillset(&act.sa_mask);
 	sigaction(signo, &act, 0);
 }

A message that is not in the archive (sent off-list or not yet received): 20120622003628.GD4730@burratino

author
#6 lore raw
Quoted 1 line · Jonathan Nieder

Thanks. Here's a less invasive patch to experiment with.

Hmm. I’ve kicked off several builds in the git-core PPA, first under dash with the original patch, now under dash with the less invasive patch. Under either dash, almost all of the builds end up trapped in what looks like a deadlock, at various parts of t9010-svn-fe.sh.

https://launchpad.net/~git-core/+archive/ppa/+build/3596228
https://launchpad.net/~git-core/+archive/ppa/+build/3596229
https://launchpad.net/~git-core/+archive/ppa/+build/3597370
https://launchpad.net/~git-core/+archive/ppa/+build/3597371

I’ll try to debug this later by hacking up the package to run that script through strace -fiv during build (since I still can’t reproduce locally), unless you have anything more clever to try.

Anders

author
#7 lore raw

I wrote this greatly simplified skeleton of the failing Git test, and found that I can reproduce what looks like the same problem, fairly reliably:

$ rm -f fifo
$ mkfifo fifo
$ dash -c 'while echo -n .; do : < fifo & : > fifo; wait $!; done'
...............................dash: 1: cannot create fifo: Interrupted system call

After running this through strace, it seems clear what went wrong. The parent’s open("fifo", O_WRONLY) is what causes the child’s open("fifo", O_RDONLY) to return. If the child exits immediately, the parent may receive a SIGCHLD before its own open() returns. Then when its SIGCHLD handler exits, open() returns EINTR.

That also explains why the SA_RESTART and restart-on-EINTR patches only turned it into a deadlock. If the open() were restarted now, it would just block to wait for a _second_ reader of the FIFO, which will never arrive.

But if that’s what happened, it seems like a kernel bug to me! Shouldn’t an open() call only be restarted or return EINTR in the case that it hasn’t had any side effects yet and can safely be restarted?

Anders

author
#8 lore raw
Quoted 3 lines · Anders Kaseorg

But if that’s what happened, it seems like a kernel bug to me! Shouldn’t an open() call only be restarted or return EINTR in the case that it hasn’t had any side effects yet and can safely be restarted?

I submitted a kernel patch:
http://marc.info/?l=linux-kernel&m=134071285509470

Anders

author
#9 lore raw
Quoted 6 lines · Anders Kaseorg
Quoted 3 lines

But if that’s what happened, it seems like a kernel bug to me! Shouldn’t an open() call only be restarted or return EINTR in the case that it hasn’t had any side effects yet and can safely be restarted?

I submitted a kernel patch:
http://marc.info/?l=linux-kernel&m=134071285509470

That’s now merged as v3.5~31, v3.4.6~25, and v3.0.38~17.

Anders

#10 lore raw
In reply to a message not in the archive
Quoted 18 lines · Jonathan Nieder
Quoted 13 lines · Anders Kaseorg
Quoted 11 lines · Jonathan Nieder
src/trap.c +1 -1
@@ -259,7 +259,7 @@ setsignal(int signo)
 		act.sa_handler = SIG_DFL;
 	}
 	*t = action;
-	act.sa_flags = 0;
+	act.sa_flags = SA_RESTART;
 	sigfillset(&act.sa_mask);
 	sigaction(signo, &act, 0);
 }

[...]

Quoted 2 lines

It looks like this was never forwarded to the dash mailing list. Any thoughts from upstream?

Quoted 8 lines
Quoted 2 lines

Context: http://thread.gmane.org/gmane.comp.version-control.git.debian/147
(FTR, it did happen again.)

Thanks. Here's a less invasive patch to experiment with.

POSIX (XCU §2.11 "Signals and Error Handling") requires that the "wait" utility must return immediately when interrupted by a trapped signal, so SA_RESTART is not appropriate for those.

Same thing for "read", even though that does not make much sense (the only thing the trap handler can do is abandon the read, since bytes already read but not part of a complete line may be lost).

On the other hand, this interruption is not specified to happen with signals whose default action is to ignore. Many shells don't do this, though (perhaps because of the difficulty of finding what the default action is).

Quoted 2 lines

It does not seem to give any advice on whether signals interrupt redirection.

For certain situations it certainly does. For simple commands without command name, it says any redirections shall be performed in a subshell environment; hence, trapped signals are reset to the default action. This is what most shells appear to do in all cases. Again, this may not be the most useful behaviour but it matches what the Bourne shell did.

Quoted 7 lines
src/redir.c +0 -0
index f96a76bc..7fb7748f 100644
@@ -206,8 +206,11 @@ openredirect(union node *redir)
 		/* FALLTHROUGH */
 	case NCLOBBER:

src/redir.c openredirect(union node *redir) · line 216

Quoted 10 lines
src/redir.c +4 -1
 		fname = redir->nfile.expfname;
-		if ((f = open64(fname, O_WRONLY|O_CREAT|O_TRUNC, 0666)) < 0)
+		while ((f = open64(fname, O_WRONLY|O_CREAT|O_TRUNC, 0666)) < 0) {
+			if (errno == EINTR)
+				continue;
 			goto ecreate;
+		}
 		break;
 	case NAPPEND:
 		fname = redir->nfile.expfname;

It looks like this patch breaks interrupting (via SIGINT) a blocking open, for example of a fifo. Although the race condition associated with relying on [EINTR] is very hard to close, I don't think that's reason to give up on it entirely.

I think it is better to rethink commit 3800d4934391b144fd261a7957aea72ced7d47ea that caused this bug (and some others that have been fixed). A signal handler for SIGCHLD was added to allow waiting for a trapped signal or a child process at the same time (using sigsuspend, as was done, or sigwait). However, leaving the signal handler installed all the time causes unexpected [EINTR] errors, so I suggest only installing it during the "wait" builtin or specifying SA_RESTART only for this special SIGCHLD handler (while leaving all other signal handlers interrupting). In either case, behaviour for SIGCHLD handlers installed because of CHLD traps should remain unchanged. The former seems safest because SA_RESTART handlers still cause visible effects such as short writes.

In case of a script doing 'trap "" CHLD', it is perhaps undesirable for "wait" to block forever, given that wait(2) doesn't do that either.

Signature · 1 line
Jilles Tjoelker