49e6bf2db
Denys Vlasenko
2017-08-04
Subject
sheel: improve comments on signal handling
Binary
No binary (build failed)
Architecture & Compiler Flags
—
Build Log
(log yok)
Commit Diff GitHub →
NOFORK_NOEXEC.lst+3 -1
| @@ -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 | |
docs/nofork_noexec.txt+19 -5
| @@ -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, | |
shell/ash.c+16 -11
| @@ -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 | |
| ) { | |
shell/hush.c+7 -0
| @@ -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); | |