sheel: improve comments on signal handling

Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>

4 files changed, 45 insertions, 17 deletions

diff --git a/NOFORK_NOEXEC.lst b/NOFORK_NOEXEC.lst
index 12ae1cd55..14019bf7d 100644
--- a/NOFORK_NOEXEC.lst
+++ b/NOFORK_NOEXEC.lst
@@ -4,10 +4,12 @@ Why can't be NOFORK:
 interactive: may wait for user input, ^C has to work
 spawner: "tool PROG ARGS" which changes program's environment - must fork
 changes state: e.g. environment, signal handlers
+alloc+xfunc: xmalloc, then xfunc - leaks memory if xfunc dies
+open+xfunc: opens fd, then calls xfunc - fd is leaked if xfunc dies
 runner: sometimes may run for long(ish) time, and/or works with network:
 	^C has to work (cat BIGFILE, chmod -R, ftpget, nc)
 
-"runners" can become eligible after hush is taught ^C to interrupt NOFORKs!
+"runners" can become eligible after shell is taught ^C to interrupt NOFORKs!
 
 Why can't be NOEXEC:
 suid: runs under different uid - must fork+exec
diff --git a/docs/nofork_noexec.txt b/docs/nofork_noexec.txt
index cfb02145a..b45a4be89 100644
--- a/docs/nofork_noexec.txt
+++ b/docs/nofork_noexec.txt
@@ -64,15 +64,27 @@ This poses much more serious limitations on what applet can do:
 * do not use shared global data, or save/restore shared global data
   (e.g. bb_common_bufsiz1) prior to returning.
   - getopt32() is ok to use. You do not need to save/restore option_mask32,
-    it is already done by core code.
+    xfunc_error_retval, and logmode - it is already done by core code.
 * if you allocate memory, you can use xmalloc() only on the very first
   allocation. All other allocations should use malloc[_or_warn]().
   After first allocation, you cannot use any xfuncs.
   Otherwise, failing xfunc will return to caller applet
   without freeing malloced data!
-* All allocated data, opened files, signal handlers, termios settings,
-  O_NONBLOCK flags etc should be freed/closed/restored prior to return.
-* ...
+* the same applies to other resources, such as open fds: no xfuncs after
+  acquiring them!
+* All allocated data, opened files, signal handlers, termios settings
+  etc should be freed/closed/restored prior to return.
+
+Currently, ash shell signal handling is implemented in a way that signals
+have non-SA_RESTARTed handlers. This means that system calls can
+return EINTR. An example of such problem is "yes" applet:
+it is implemented so that it has a writing loop, this loop is exited on
+any write error, and in the case of user pressing ^C the error was EINTR.
+The problem is, the error causes stdout FILE* object to get into error
+state, needing clearerr() - or else subsequent shell output will also
+not work. ("yes" has been downgraded to NOEXEC, since hush signal handling
+does not have this problem - which makes "yes" to not exit on ^C (bug).
+But stray EINTRs can be seen in any NOFORK under ash, until ash is fixed).
 
 NOFORK applets give the most of speed advantage, but are trickiest
 to implement. In order to minimize amount of bugs and maintenance,
@@ -82,6 +94,8 @@ frequently executed from shell/find/xargs, particularly in shell
 script loops. Applets which mess with signal handlers, termios etc
 are probably not worth the effort.
 
+Applets which must be interruptible by ^C in shells can not be NOFORKs.
+
 Any NOFORK applet is also a NOEXEC applet.
 
 
@@ -94,7 +108,7 @@ API to call NOFORK applets is two functions:
 
 First one is directly used by shells if FEATURE_SH_NOFORK=y.
 Second one is used by many applets, but main users are xargs and find.
-It itself calls run_nofork_applet(), if argv[0] turned out to be a name
+It itself calls run_nofork_applet(), if argv[0] is a name
 of a NOFORK applet.
 
 run_nofork_applet() saves/inits/restores option parsing, xfunc_error_retval,
diff --git a/shell/ash.c b/shell/ash.c
index 8c9f4adc6..ca9926b54 100644
--- a/shell/ash.c
+++ b/shell/ash.c
@@ -3536,9 +3536,12 @@ setsignal(int signo)
 #endif
 		}
 	}
-//TODO: if !rootshell, we reset SIGQUIT to DFL,
-//whereas we have to restore it to what shell got on entry
-//from the parent. See comment above
+	/* if !rootshell, we reset SIGQUIT to DFL,
+	 * whereas we have to restore it to what shell got on entry.
+	 * This is handled by the fact that if signal was IGNored on entry,
+	 * then cur_act is S_HARD_IGN and we never change its sigaction
+	 * (see code below).
+	 */
 
 	if (signo == SIGCHLD)
 		new_act = S_CATCH;
@@ -3566,6 +3569,8 @@ setsignal(int signo)
 	if (cur_act == S_HARD_IGN || cur_act == new_act)
 		return;
 
+	*t = new_act;
+
 	act.sa_handler = SIG_DFL;
 	switch (new_act) {
 	case S_CATCH:
@@ -3575,16 +3580,13 @@ setsignal(int signo)
 		act.sa_handler = SIG_IGN;
 		break;
 	}
-
 	/* flags and mask matter only if !DFL and !IGN, but we do it
 	 * for all cases for more deterministic behavior:
 	 */
-	act.sa_flags = 0;
+	act.sa_flags = 0; //TODO: why not SA_RESTART?
 	sigfillset(&act.sa_mask);
 
 	sigaction_set(signo, &act);
-
-	*t = new_act;
 }
 
 /* mode flags for set_curjob */
@@ -13429,7 +13431,9 @@ readcmd(int argc UNUSED_PARAM, char **argv UNUSED_PARAM)
 	INT_ON;
 
 	if ((uintptr_t)r == 1 && errno == EINTR) {
-		/* to get SIGCHLD: sleep 1 & read x; echo $x */
+		/* To get SIGCHLD: sleep 1 & read x; echo $x
+		 * Correct behavior is to not exit "read"
+		 */
 		if (pending_sig == 0)
 			goto again;
 	}
@@ -13544,13 +13548,14 @@ exitshell(void)
 	/* NOTREACHED */
 }
 
-static void
+/* Don't inline: conserve stack of caller from having our locals too */
+static NOINLINE void
 init(void)
 {
 	/* we will never free this */
 	basepf.next_to_pgetc = basepf.buf = ckmalloc(IBUFSIZ);
 
-	sigmode[SIGCHLD - 1] = S_DFL;
+	sigmode[SIGCHLD - 1] = S_DFL; /* ensure we install handler even if it is SIG_IGNed */
 	setsignal(SIGCHLD);
 
 	/* bash re-enables SIGHUP which is SIG_IGNed on entry.
@@ -13561,7 +13566,6 @@ init(void)
 	{
 		char **envp;
 		const char *p;
-		struct stat st1, st2;
 
 		initvar();
 		for (envp = environ; envp && *envp; envp++) {
@@ -13587,6 +13591,7 @@ init(void)
 #endif
 		p = lookupvar("PWD");
 		if (p) {
+			struct stat st1, st2;
 			if (p[0] != '/' || stat(p, &st1) || stat(".", &st2)
 			 || st1.st_dev != st2.st_dev || st1.st_ino != st2.st_ino
 			) {
diff --git a/shell/hush.c b/shell/hush.c
index b04f793f1..bb80f422c 100644
--- a/shell/hush.c
+++ b/shell/hush.c
@@ -1979,6 +1979,9 @@ static int check_and_run_traps(void)
 			break;
 #if ENABLE_HUSH_JOB
 		case SIGHUP: {
+//TODO: why are we doing this? ash and dash don't do this,
+//they have no handler for SIGHUP at all,
+//they rely on kernel to send SIGHUP+SIGCONT to orphaned process groups
 			struct pipe *job;
 			debug_printf_exec("%s: sig:%d default SIGHUP handler\n", __func__, sig);
 			/* bash is observed to signal whole process groups,
@@ -8646,6 +8649,10 @@ static void install_sighandlers(unsigned mask)
 		 */
 		if (sig == SIGCHLD)
 			continue;
+		/* bash re-enables SIGHUP which is SIG_IGNed on entry.
+		 * Try: "trap '' HUP; bash; echo RET" and type "kill -HUP $$"
+		 */
+		//if (sig == SIGHUP) continue; - TODO?
 		if (old_handler == SIG_IGN) {
 			/* oops... restore back to IGN, and record this fact */
 			install_sighandler(sig, old_handler);