ash: fix "orwell bug" 1984. Testcase:

    trap_handler() {
        echo trap
    }
    trap trap_handler USR1
    sleep 3600 &
    while true; do wait; done

diff --git a/shell/ash.c b/shell/ash.c
index 8ff5f4c31bf70c851b7f0c4ffdb01506222425d4..a877b5bab49604ec032aecce721f651ef285ab4b 100644 (file)
--- a/shell/ash.c
+++ b/shell/ash.c
@@ -164,7 +164,16 @@ struct globals_misc {
        char *arg0; /* value of $0 */
 
        struct jmploc *exception_handler;
-       int exception;
+
+// disabled by vda: cannot understand how it was supposed to work -
+// cannot fix bugs. That's why you have to explain your non-trivial designs!
+//     /* do we generate EXSIG events */
+//     int exsig; /* counter */
+       volatile int suppressint; /* counter */
+       volatile /*sig_atomic_t*/ smallint intpending; /* 1 = got SIGINT */
+       /* last pending signal */
+       volatile /*sig_atomic_t*/ smallint pendingsig;
+       smallint exception; /* kind of exception (0..5) */
        /* exceptions */
 #define EXINT 0         /* SIGINT received */
 #define EXERROR 1       /* a generic error */
@@ -172,12 +181,6 @@ struct globals_misc {
 #define EXEXEC 3        /* command execution failed */
 #define EXEXIT 4        /* exit the shell */
 #define EXSIG 5         /* trapped signal in wait(1) */
-       volatile int suppressint;
-       volatile sig_atomic_t intpending;
-       /* do we generate EXSIG events */
-       int exsig;
-       /* last pending signal */
-       volatile sig_atomic_t pendingsig;
 
        /* trap handler commands */
        char *trap[NSIG];
@@ -211,7 +214,7 @@ static struct globals_misc *const ptr_to_globals_misc __attribute__ ((section ("
 #define exception         (G_misc.exception        )
 #define suppressint       (G_misc.suppressint      )
 #define intpending        (G_misc.intpending       )
-#define exsig             (G_misc.exsig            )
+//#define exsig             (G_misc.exsig            )
 #define pendingsig        (G_misc.pendingsig       )
 #define trap      (G_misc.trap     )
 #define isloginsh (G_misc.isloginsh)
@@ -281,6 +284,7 @@ raise_interrupt(void)
        i = EXSIG;
        if (gotsig[SIGINT - 1] && !trap[SIGINT]) {
                if (!(rootshell && iflag)) {
+                       /* Kill ourself with SIGINT */
                        signal(SIGINT, SIG_DFL);
                        raise(SIGINT);
                }
@@ -333,15 +337,6 @@ force_int_on(void)
                        raise_interrupt(); \
        } while (0)
 
-#define EXSIGON \
-       do { \
-               exsig++; \
-               xbarrier(); \
-               if (pendingsig) \
-                       raise_exception(EXSIG); \
-       } while (0)
-/* EXSIG is turned off by evalbltin(). */
-
 /*
  * Ignore a signal. Only one usage site - in forkchild()
  */
@@ -363,10 +358,10 @@ onsig(int signo)
        gotsig[signo - 1] = 1;
        pendingsig = signo;
 
-       if (exsig || (signo == SIGINT && !trap[SIGINT])) {
+       if ( /* exsig || */ (signo == SIGINT && !trap[SIGINT])) {
                if (!suppressint) {
                        pendingsig = 0;
-                       raise_interrupt();
+                       raise_interrupt(); /* does not return */
                }
                intpending = 1;
        }
@@ -3256,12 +3251,11 @@ setsignal(int signo)
        struct sigaction act;
 
        t = trap[signo];
+       action = S_IGN;
        if (t == NULL)
                action = S_DFL;
        else if (*t != '\0')
                action = S_CATCH;
-       else
-               action = S_IGN;
        if (rootshell && action == S_DFL) {
                switch (signo) {
                case SIGINT:
@@ -3294,7 +3288,7 @@ setsignal(int signo)
                /*
                 * current setting unknown
                 */
-               if (sigaction(signo, 0, &act) == -1) {
+               if (sigaction(signo, NULL, &act) == -1) {
                        /*
                         * Pretend it worked; maybe we should give a warning
                         * here, but other shells don't. We don't alter
@@ -3302,19 +3296,19 @@ setsignal(int signo)
                         */
                        return;
                }
+               tsig = S_RESET; /* force to be set */
                if (act.sa_handler == SIG_IGN) {
+                       tsig = S_HARD_IGN;
                        if (mflag
                         && (signo == SIGTSTP || signo == SIGTTIN || signo == SIGTTOU)
                        ) {
                                tsig = S_IGN;   /* don't hard ignore these */
-                       } else
-                               tsig = S_HARD_IGN;
-               } else {
-                       tsig = S_RESET; /* force to be set */
+                       }
                }
        }
        if (tsig == S_HARD_IGN || tsig == action)
                return;
+       act.sa_handler = SIG_DFL;
        switch (action) {
        case S_CATCH:
                act.sa_handler = onsig;
@@ -3322,13 +3316,11 @@ setsignal(int signo)
        case S_IGN:
                act.sa_handler = SIG_IGN;
                break;
-       default:
-               act.sa_handler = SIG_DFL;
        }
        *t = action;
        act.sa_flags = 0;
        sigfillset(&act.sa_mask);
-       sigaction(signo, &act, 0);
+       sigaction(signo, &act, NULL);
 }
 
 /* mode flags for set_curjob */
@@ -3763,7 +3755,8 @@ waitproc(int wait_flags, int *status)
        if (jobctl)
                wait_flags |= WUNTRACED;
 #endif
-       return waitpid(-1, status, wait_flags); // safe_waitpid?
+       /* NB: _not_ safe_waitpid, we need to detect EINTR */
+       return waitpid(-1, status, wait_flags);
 }
 
 /*
@@ -3781,8 +3774,14 @@ dowait(int wait_flags, struct job *job)
        TRACE(("dowait(%d) called\n", wait_flags));
        pid = waitproc(wait_flags, &status);
        TRACE(("wait returns pid=%d, status=%d\n", pid, status));
-       if (pid <= 0)
+       if (pid <= 0) {
+               /* If we were doing blocking wait and (probably) got EINTR,
+                * check for pending sigs received while waiting.
+                * (NB: can be moved into callers if needed) */
+               if (wait_flags == DOWAIT_BLOCK && pendingsig)
+                       raise_exception(EXSIG);
                return pid;
+       }
        INT_OFF;
        thisjob = NULL;
        for (jp = curjob; jp; jp = jp->prev_job) {
@@ -4004,7 +4003,10 @@ waitcmd(int argc, char **argv)
        int retval;
        struct job *jp;
 
-       EXSIGON;
+//     exsig++;
+//     xbarrier();
+       if (pendingsig)
+               raise_exception(EXSIG);
 
        nextopt(nullstr);
        retval = 0;
@@ -4015,10 +4017,8 @@ waitcmd(int argc, char **argv)
                for (;;) {
                        jp = curjob;
                        while (1) {
-                               if (!jp) {
-                                       /* no running procs */
-                                       goto out;
-                               }
+                               if (!jp) /* no running procs */
+                                       goto ret;
                                if (jp->state == JOBRUNNING)
                                        break;
                                jp->waited = 1;
@@ -4051,7 +4051,7 @@ waitcmd(int argc, char **argv)
                ;
        } while (*++argv);
 
- out:
+ ret:
        return retval;
 }
 
@@ -4450,7 +4450,7 @@ clear_traps(void)
        char **tp;
 
        for (tp = trap; tp < &trap[NSIG]; tp++) {
-               if (*tp && **tp) {      /* trap not NULL or SIG_IGN */
+               if (*tp && **tp) {      /* trap not NULL or "" (SIG_IGN) */
                        INT_OFF;
                        free(*tp);
                        *tp = NULL;
@@ -4613,8 +4613,8 @@ waitforjob(struct job *jp)
                 * intuit from the subprocess exit status whether a SIGINT
                 * occurred, and if so interrupt ourselves.  Yuck.  - mycroft
                 */
-               if (jp->sigint)
-                       raise(SIGINT);
+               if (jp->sigint) /* TODO: do the same with all signals */
+                       raise(SIGINT); /* ... by raise(jp->sig) instead? */
        }
        if (jp->state == JOBDONE)
 #endif
@@ -6268,7 +6268,7 @@ expmeta(char *enddir, char *name)
                p++;
        if (*p == '.')
                matchdot++;
-       while (! intpending && (dp = readdir(dirp)) != NULL) {
+       while (!intpending && (dp = readdir(dirp)) != NULL) {
                if (dp->d_name[0] == '.' && ! matchdot)
                        continue;
                if (pmatch(start, dp->d_name)) {
@@ -7437,7 +7437,7 @@ dotrap(void)
        char *q;
        int i;
        int savestatus;
-       int skip = 0;
+       int skip;
 
        savestatus = exitstatus;
        pendingsig = 0;
@@ -7454,10 +7454,10 @@ dotrap(void)
                skip = evalstring(p, SKIPEVAL);
                exitstatus = savestatus;
                if (skip)
-                       break;
+                       return skip;
        }
 
-       return skip;
+       return 0;
 }
 
 /* forward declarations - evaluation is fairly recursive business... */
@@ -8434,22 +8434,15 @@ evalcommand(union node *cmd, int flags)
                }
                if (evalbltin(cmdentry.u.cmd, argc, argv)) {
                        int exit_status;
-                       int i, j;
-
-                       i = exception;
+                       int i = exception;
                        if (i == EXEXIT)
                                goto raise;
-
                        exit_status = 2;
-                       j = 0;
                        if (i == EXINT)
-                               j = SIGINT;
+                               exit_status = 128 + SIGINT;
                        if (i == EXSIG)
-                               j = pendingsig;
-                       if (j)
-                               exit_status = j + 128;
+                               exit_status = 128 + pendingsig;
                        exitstatus = exit_status;
-
                        if (i == EXINT || spclbltin > 0) {
  raise:
                                longjmp(exception_handler->loc, 1);
@@ -8499,7 +8492,7 @@ evalbltin(const struct builtincmd *cmd, int argc, char **argv)
        exitstatus |= ferror(stdout);
        clearerr(stdout);
        commandname = savecmdname;
-       exsig = 0;
+//     exsig = 0;
        exception_handler = savehandler;
 
        return i;

diff --git a/shell/ash_doc.txt b/shell/ash_doc.txt
new file mode 100644 (file)
index 0000000..28c5748
--- /dev/null
+++ b/shell/ash_doc.txt
@@ -0,0 +1,31 @@
+       Wait + signals
+
+We had some bugs here which are hard to test in testsuite.
+
+Bug 1280 (http://busybox.net/bugs/view.php?id=1280):
+was misbehaving in interactive ash. Correct behavior:
+
+$ sleep 20 &
+$ wait
+^C
+$ wait
+^C
+$ wait
+^C
+...
+
+Bug 1984 (http://busybox.net/bugs/view.php?id=1984):
+traps were not triggering:
+
+trap_handler_usr () {
+    echo trap usr
+}
+trap_handler_int () {
+    echo trap int
+}
+trap trap_handler_usr USR1
+trap trap_handler_int INT
+sleep 3600 &
+echo "Please do: kill -USR1 $$"
+echo "or: kill -INT $$"
+while true; do wait; echo wait interrupted; done