Zsh Mailing List Archive
Messages sorted by: Reverse Date, Date, Thread, Author

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@xxxxxxxxxxxxxxxxxxxxxxx
Messages sorted by: Reverse Date, Date, Thread, Author