From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2811 invoked from network); 1 Nov 2000 09:41:52 -0000 Received: from sunsite.auc.dk (130.225.51.30) by ns1.primenet.com.au with SMTP; 1 Nov 2000 09:41:52 -0000 Received: (qmail 6112 invoked by alias); 1 Nov 2000 09:41:47 -0000 Mailing-List: contact zsh-workers-help@sunsite.auc.dk; run by ezmlm Precedence: bulk X-No-Archive: yes X-Seq: 13108 Received: (qmail 6104 invoked from network); 1 Nov 2000 09:41:47 -0000 Date: Wed, 1 Nov 2000 10:41:45 +0100 (MET) Message-Id: <200011010941.KAA05877@beta.informatik.hu-berlin.de> From: Sven Wischnowsky To: zsh-workers@sunsite.auc.dk In-reply-to: Peter Stephenson's message of Tue, 31 Oct 2000 13:51:35 +0000 Subject: Re: zsh-3.1.9-dev-6 crashes occassionally Peter Stephenson wrote: > Sven wrote: > > + ALLOWTRAPS { > > + while ((r = read(SHTTY, &cc, 1)) != 1) { > > I suppose you've thought this through more than I have, but wouldn't it be > safer just to run traps every time the read returns? I'm assuming a signal > arriving will interrupt the read in any case, so as far as I can see it's > pretty much equivalent in practise. There's nothing too nasty in the block > underneath, but it does call zrefresh() and attachtty() which are probably > best treated as black boxes. Here's the second version, still not-to-be-committed. It uses ALLOWTRAPS { ... } DISALLOWTRAPS only around very short code sequences. It even adds two utility functions `ztrapread' and `ztrapwrite' which call read and write (not surprising) while allowing execution of trap handlers while they are waiting. I've probably used these in too many places, for example in zftp. But it's a problem: we want to allow `concurrent' execution of trap handlers as often as possible when the shell may be waiting but if the trap handler calls the same code the `main thread' is currently waiting at, it may or may not cause problems. Bleh. That's exactly the kind of decision I wanted to avoid. We could try to solve this at some central point, e.g. in execbuiltin() and make that return with a non-zero status if the same builtin is currently being executed, unless the structure describing the builtin contains a flag that says that this builtin can be called from a trap handler even if it is currently being executed by the normal control flow. Bye Sven diff -u -r ../oz/Src/Modules/zftp.c ./Src/Modules/zftp.c --- ../oz/Src/Modules/zftp.c Tue Oct 31 20:59:50 2000 +++ ./Src/Modules/zftp.c Tue Oct 31 21:48:22 2000 @@ -801,7 +801,7 @@ cmdbuf[0] = (char)IAC; cmdbuf[1] = (char)DONT; cmdbuf[2] = ch; - write(zfsess->cfd, cmdbuf, 3); + ztrapwrite(zfsess->cfd, cmdbuf, 3); continue; case DO: @@ -811,7 +811,7 @@ cmdbuf[0] = (char)IAC; cmdbuf[1] = (char)WONT; cmdbuf[2] = ch; - write(zfsess->cfd, cmdbuf, 3); + ztrapwrite(zfsess->cfd, cmdbuf, 3); continue; case EOF: @@ -996,7 +996,7 @@ return 6; } zfalarm(tmout); - ret = write(zfsess->cfd, cmd, strlen(cmd)); + ret = ztrapwrite(zfsess->cfd, cmd, strlen(cmd)); alarm(0); if (ret <= 0) { @@ -1470,7 +1470,7 @@ int ret; if (!tmout) - return read(fd, bf, sz); + return ztrapread(fd, bf, sz); if (setjmp(zfalrmbuf)) { alarm(0); @@ -1479,7 +1479,7 @@ } zfalarm(tmout); - ret = read(fd, bf, sz); + ret = ztrapread(fd, bf, sz); /* we don't bother turning off the whole alarm mechanism here */ alarm(0); @@ -1495,7 +1495,7 @@ int ret; if (!tmout) - return write(fd, bf, sz); + return ztrapwrite(fd, bf, sz); if (setjmp(zfalrmbuf)) { alarm(0); @@ -1504,7 +1504,7 @@ } zfalarm(tmout); - ret = write(fd, bf, sz); + ret = ztrapwrite(fd, bf, sz); /* we don't bother turning off the whole alarm mechanism here */ alarm(0); @@ -2846,7 +2846,7 @@ if (!zfnopen) { /* Write the final status in case this is a subshell */ lseek(zfstatfd, zfsessno*sizeof(int), 0); - write(zfstatfd, zfstatusp+zfsessno, sizeof(int)); + ztrapwrite(zfstatfd, zfstatusp+zfsessno, sizeof(int)); close(zfstatfd); zfstatfd = -1; @@ -3123,7 +3123,7 @@ /* Get the status in case it was set by a forked process */ int oldstatus = zfstatusp[zfsessno]; lseek(zfstatfd, 0, 0); - read(zfstatfd, zfstatusp, sizeof(int)*zfsesscnt); + ztrapread(zfstatfd, zfstatusp, sizeof(int)*zfsesscnt); if (zfsess->cfd != -1 && (zfstatusp[zfsessno] & ZFST_CLOS)) { /* got closed in subshell without us knowing */ zcfinish = 2; @@ -3212,7 +3212,7 @@ * but only for the active session. */ lseek(zfstatfd, zfsessno*sizeof(int), 0); - write(zfstatfd, zfstatusp+zfsessno, sizeof(int)); + ztrapwrite(zfstatfd, zfstatusp+zfsessno, sizeof(int)); } return ret; } diff -u -r ../oz/Src/Modules/zpty.c ./Src/Modules/zpty.c --- ../oz/Src/Modules/zpty.c Tue Oct 31 20:59:50 2000 +++ ./Src/Modules/zpty.c Tue Oct 31 21:49:48 2000 @@ -489,7 +489,7 @@ if (cmd->fin) break; } - if ((ret = read(cmd->fd, buf + used, 1)) == 1) { + if ((ret = ztrapread(cmd->fd, buf + used, 1)) == 1) { if (++used == blen) { buf = hrealloc(buf, blen, blen << 1); blen <<= 1; @@ -520,7 +520,7 @@ setsparam(*args, ztrdup(metafy(buf, used, META_HREALLOC))); else { fflush(stdout); - write(1, buf, used); + ztrapwrite(1, buf, used); } return (used ? 0 : cmd->fin + 1); } @@ -532,7 +532,7 @@ for (; !errflag && !breaks && !retflag && !contflag && len; len -= written, s += written) { - if ((written = write(cmd->fd, s, len)) < 0 && cmd->nblock && + if ((written = ztrapwrite(cmd->fd, s, len)) < 0 && cmd->nblock && #ifdef EWOULDBLOCK errno == EWOULDBLOCK #else @@ -574,7 +574,7 @@ int n; char buf[BUFSIZ]; - while ((n = read(0, buf, BUFSIZ)) > 0) + while ((n = ztrapread(0, buf, BUFSIZ)) > 0) if (ptywritestr(cmd, buf, n)) return 1; } diff -u -r ../oz/Src/Zle/zle_main.c ./Src/Zle/zle_main.c --- ../oz/Src/Zle/zle_main.c Tue Oct 31 20:59:48 2000 +++ ./Src/Zle/zle_main.c Tue Oct 31 21:43:40 2000 @@ -313,11 +313,17 @@ breakread(int fd, char *buf, int n) { fd_set f; + int ret; FD_ZERO(&f); FD_SET(fd, &f); - return (select(fd + 1, (SELECT_ARG_2_T) & f, NULL, NULL, NULL) == -1 ? - EOF : read(fd, buf, n)); + + ALLOWTRAPS { + ret = (select(fd + 1, (SELECT_ARG_2_T) & f, NULL, NULL, NULL) == -1 ? + EOF : read(fd, buf, n)); + } DISALLOWTRAPS; + + return ret; } # define read breakread @@ -388,7 +394,7 @@ # else ioctl(SHTTY, TCSETA, &ti.tio); # endif - r = read(SHTTY, &cc, 1); + r = ztrapread(SHTTY, &cc, 1); # ifdef HAVE_TERMIOS_H tcsetattr(SHTTY, TCSANOW, &shttyinfo.tio); # else @@ -398,7 +404,10 @@ # endif #endif } - while ((r = read(SHTTY, &cc, 1)) != 1) { + for (;;) { + r = ztrapread(SHTTY, &cc, 1); + if (r == 1) + break; if (r == 0) { /* The test for IGNOREEOF was added to make zsh ignore ^Ds that were typed while commands are running. Unfortuantely diff -u -r ../oz/Src/builtin.c ./Src/builtin.c --- ../oz/Src/builtin.c Tue Oct 31 20:59:46 2000 +++ ./Src/builtin.c Tue Oct 31 21:38:41 2000 @@ -3218,7 +3218,7 @@ checkjobs(); /* check if any jobs are running/stopped */ if (stopmsg) { stopmsg = 2; - LASTALLOC_RETURN; + return; } } if (in_exit++ && from_signal) @@ -3240,7 +3240,7 @@ } } if (sigtrapped[SIGEXIT]) - dotrap(SIGEXIT); + dotrap(SIGEXIT, 1); runhookdef(EXITHOOK, NULL); if (mypid != getpid()) _exit(val); @@ -3486,7 +3486,7 @@ *bptr = readchar; val = 1; readchar = -1; - } else if ((val = read(readfd, bptr, nchars)) <= 0) + } else if ((val = ztrapread(readfd, bptr, nchars)) <= 0) break; /* decrement number of characters read from number required */ @@ -3500,7 +3500,7 @@ if (!izle && !ops['u'] && !ops['p']) { /* dispose of result appropriately, etc. */ if (isem) - while (val > 0 && read(SHTTY, &d, 1) == 1 && d != '\n'); + while (val > 0 && ztrapread(SHTTY, &d, 1) == 1 && d != '\n'); else settyinfo(&shttyinfo); if (haso) { @@ -3733,6 +3733,7 @@ zread(int izle, int *readchar) { char cc, retry = 0; + int ret; if (izle) { int c = getkeyptr(0); @@ -3756,7 +3757,8 @@ } for (;;) { /* read a character from readfd */ - switch (read(readfd, &cc, 1)) { + ret = ztrapread(readfd, &cc, 1); + switch (ret) { case 1: /* return the character read */ return STOUC(cc); diff -u -r ../oz/Src/exec.c ./Src/exec.c --- ../oz/Src/exec.c Tue Oct 31 20:59:46 2000 +++ ./Src/exec.c Tue Oct 31 21:00:05 2000 @@ -738,6 +738,7 @@ execsimple(Estate state) { wordcode code = *state->pc++; + int lv; if (errflag) return (lastval = 1); @@ -754,9 +755,13 @@ fputc('\n', xtrerr); fflush(xtrerr); } - return (lastval = (errflag ? errflag : cmdoutval)); + lv = (errflag ? errflag : cmdoutval); } else - return (lastval = (execfuncs[code - WC_CURSH])(state, 0)); + lv = (execfuncs[code - WC_CURSH])(state, 0); + + RUNTRAPS(); + + return lastval = lv; } /* Main routine for executing a list. * @@ -887,19 +892,19 @@ noerrexit = oldnoerrexit; if (sigtrapped[SIGDEBUG]) - dotrap(SIGDEBUG); + dotrap(SIGDEBUG, 1); /* Check whether we are suppressing traps/errexit * * (typically in init scripts) and if we haven't * * already performed them for this sublist. */ if (!noerrexit && !donetrap) { if (sigtrapped[SIGZERR] && lastval) { - dotrap(SIGZERR); + dotrap(SIGZERR, 1); donetrap = 1; } if (lastval && isset(ERREXIT)) { if (sigtrapped[SIGEXIT]) - dotrap(SIGEXIT); + dotrap(SIGEXIT, 1); if (mypid != getpid()) _exit(lastval); else @@ -1181,9 +1186,10 @@ else list_pipe_text[0] = '\0'; } - if (WC_PIPE_TYPE(pcode) == WC_PIPE_END) + if (WC_PIPE_TYPE(pcode) == WC_PIPE_END) { execcmd(state, input, output, how, last1 ? 1 : 2); - else { + RUNTRAPS(); + } else { int old_list_pipe = list_pipe; Wordcode next = state->pc + (*state->pc); wordcode code; @@ -1218,12 +1224,14 @@ entersubsh(how, 2, 0); close(synch[1]); execcmd(state, input, pipes[1], how, 0); + RUNTRAPS(); _exit(lastval); } } else { /* otherwise just do the pipeline normally. */ subsh_close = pipes[0]; execcmd(state, input, pipes[1], how, 0); + RUNTRAPS(); } zclose(pipes[1]); state->pc = next; diff -u -r ../oz/Src/init.c ./Src/init.c --- ../oz/Src/init.c Tue Oct 31 20:59:46 2000 +++ ./Src/init.c Tue Oct 31 21:00:47 2000 @@ -170,7 +170,7 @@ } if (isset(SINGLECOMMAND) && toplevel) { if (sigtrapped[SIGEXIT]) - dotrap(SIGEXIT); + dotrap(SIGEXIT, 1); exit(lastval); } if (justonce) @@ -1107,6 +1107,7 @@ pptbuf = unmetafy(promptexpand(lp, 0, NULL, NULL), &pptlen); write(2, (WRITE_ARG_2_T)pptbuf, pptlen); free(pptbuf); + return (unsigned char *)shingetline(); } diff -u -r ../oz/Src/input.c ./Src/input.c --- ../oz/Src/input.c Tue Oct 31 20:59:46 2000 +++ ./Src/input.c Tue Oct 31 21:01:15 2000 @@ -141,7 +141,9 @@ for (;;) { do { errno = 0; - c = fgetc(bshin); + ALLOWTRAPS { + c = fgetc(bshin); + } DISALLOWTRAPS; } while (c < 0 && errno == EINTR); if (c < 0 || c == '\n') { if (c == '\n') diff -u -r ../oz/Src/jobs.c ./Src/jobs.c --- ../oz/Src/jobs.c Tue Oct 31 20:59:46 2000 +++ ./Src/jobs.c Tue Oct 31 21:09:18 2000 @@ -378,7 +378,7 @@ zrefresh(); } if (sigtrapped[SIGCHLD] && job != thisjob) - dotrap(SIGCHLD); + dotrap(SIGCHLD, 0); /* When MONITOR is set, the foreground process runs in a different * * process group from the shell, so the shell will not receive * @@ -389,7 +389,7 @@ if (sig == SIGINT || sig == SIGQUIT) { if (sigtrapped[sig]) { - dotrap(sig); + dotrap(sig, 0); /* We keep the errflag as set or not by dotrap. * This is to fulfil the promise to carry on * with the jobs if trap returns zero. @@ -878,7 +878,9 @@ else kill(pid, SIGCONT); - child_suspend(SIGINT); + ALLOWTRAPS { + child_suspend(SIGINT); + } DISALLOWTRAPS; child_block(); } child_unblock(); @@ -900,7 +902,9 @@ while (!errflag && jn->stat && !(jn->stat & STAT_DONE) && !(interact && (jn->stat & STAT_STOPPED))) { - child_suspend(sig); + ALLOWTRAPS { + child_suspend(sig); + } DISALLOWTRAPS; /* Commenting this out makes ^C-ing a job started by a function stop the whole function again. But I guess it will stop something else from working properly, we have to find out @@ -1363,7 +1367,7 @@ killjb(jobtab + job, SIGCONT); } if (func == BIN_WAIT) - waitjob(job, SIGINT); + waitjob(job, SIGINT); if (func != BIN_BG) { waitjobs(); retval = lastval2; diff -u -r ../oz/Src/signals.c ./Src/signals.c --- ../oz/Src/signals.c Tue Oct 31 20:59:47 2000 +++ ./Src/signals.c Tue Oct 31 21:00:05 2000 @@ -497,7 +497,7 @@ case SIGHUP: if (sigtrapped[SIGHUP]) - dotrap(SIGHUP); + dotrap(SIGHUP, 0); else { stopmsg = 1; zexit(SIGHUP, 1); @@ -506,7 +506,7 @@ case SIGINT: if (sigtrapped[SIGINT]) - dotrap(SIGINT); + dotrap(SIGINT, 0); else { if ((isset(PRIVILEGED) || isset(RESTRICTED)) && isset(INTERACTIVE) && noerrexit < 0) @@ -523,14 +523,14 @@ case SIGWINCH: adjustwinsize(1); /* check window size and adjust */ if (sigtrapped[SIGWINCH]) - dotrap(SIGWINCH); + dotrap(SIGWINCH, 0); break; #endif case SIGALRM: if (sigtrapped[SIGALRM]) { int tmout; - dotrap(SIGALRM); + dotrap(SIGALRM, 0); if ((tmout = getiparam("TMOUT"))) alarm(tmout); /* reset the alarm */ @@ -549,7 +549,7 @@ break; default: - dotrap(sig); + dotrap(sig, 0); break; } /* end of switch(sig) */ @@ -907,7 +907,9 @@ * function will test for this, but this way we keep status flags * * intact without working too hard. Special cases (e.g. calling * * a trap for SIGINT after the error flag was set) are handled * - * by the calling code. (PWS 1995/06/08). */ + * by the calling code. (PWS 1995/06/08). * + * * + * This test is now replicated in dotrap(). */ if ((*sigtr & ZSIG_IGNORED) || !sigfn || errflag) return; @@ -957,11 +959,56 @@ *sigtr &= ~ZSIG_IGNORED; } -/* Standard call to execute a trap for a given signal */ +/* != 0 if trap handlers can be called immediately */ + +/**/ +mod_export int trapsallowed; + +/* Queued traps and allocated length of queue. */ + +static int *trapqueue, trapqlen; + +/* Number of used slots in trap queue. */ + +/**/ +mod_export int trapqused; + +/* Standard call to execute a trap for a given signal. The second + * argument should be zero if we may need to put the trap on the queue + * and 1 if it may be called immediately. It should never be set to + * anything less than zero, that's used internally. */ /**/ void -dotrap(int sig) +dotrap(int sig, int now) { - dotrapargs(sig, sigtrapped+sig, sigfuncs[sig]); + /* Copied from dotrapargs(). */ + if ((sigtrapped[sig] & ZSIG_IGNORED) || !sigfuncs[sig] || errflag) + return; + + if (now || trapsallowed) { + if (now < 0) + RUNTRAPS(); + dotrapargs(sig, sigtrapped+sig, sigfuncs[sig]); + } else { + if (trapqlen == trapqused) + trapqueue = (int *) zrealloc(trapqueue, (trapqlen += 32)); + trapqueue[trapqused++] = sig; + } +} + +/**/ +mod_export void +doqueuedtraps(void) +{ + int sig, ota = trapsallowed; + + trapsallowed = 1; + while (trapqused) { + trapqused--; + sig = *trapqueue; + memcpy(trapqueue, trapqueue + 1, trapqused * sizeof(int)); + dotrap(sig, -1); + } + trapsallowed = ota; } diff -u -r ../oz/Src/signals.h ./Src/signals.h --- ../oz/Src/signals.h Tue Oct 31 20:59:47 2000 +++ ./Src/signals.h Tue Oct 31 21:00:05 2000 @@ -118,3 +118,8 @@ extern sigset_t signal_unblock _((sigset_t)); #endif /* POSIX_SIGNALS */ +#define RUNTRAPS() do { if (trapqused) doqueuedtraps(); } while (0) +#define ALLOWTRAPS do { RUNTRAPS(); trapsallowed++; do +#define DISALLOWTRAPS while (0); RUNTRAPS(); trapsallowed--; } while (0) +#define ALLOWTRAPS_RETURN(V) \ + do { RUNTRAPS(); trapsallowed--; return (V); } while (0) diff -u -r ../oz/Src/utils.c ./Src/utils.c --- ../oz/Src/utils.c Tue Oct 31 20:59:47 2000 +++ ./Src/utils.c Tue Oct 31 21:46:30 2000 @@ -1419,12 +1419,38 @@ } /**/ +mod_export int +ztrapread(int fd, char *buf, int len) +{ + int ret; + + ALLOWTRAPS { + ret = read(fd, buf, len); + } DISALLOWTRAPS; + + return ret; +} + +/**/ +mod_export int +ztrapwrite(int fd, char *buf, int len) +{ + int ret; + + ALLOWTRAPS { + ret = write(fd, buf, len); + } DISALLOWTRAPS; + + return ret; +} + +/**/ int read1char(void) { char c; - while (read(SHTTY, &c, 1) != 1) { + while (ztrapread(SHTTY, &c, 1) != 1) { if (errno != EINTR || errflag || retflag || breaks || contflag) return -1; } @@ -1441,7 +1467,7 @@ ioctl(SHTTY, FIONREAD, (char *)&val); if (purge) { for (; val; val--) - read(SHTTY, &c, 1); + ztrapread(SHTTY, &c, 1); } #endif diff -u -r ../oz/Src/zsh.h ./Src/zsh.h --- ../oz/Src/zsh.h Tue Oct 31 20:59:47 2000 +++ ./Src/zsh.h Tue Oct 31 21:00:05 2000 @@ -1627,8 +1627,6 @@ #endif ; -# define LASTALLOC_RETURN return - # define NEWHEAPS(h) do { Heap _switch_oldheaps = h = new_heaps(); do # define OLDHEAPS while (0); old_heaps(_switch_oldheaps); } while (0); -- Sven Wischnowsky wischnow@informatik.hu-berlin.de