PATCH Re: deadlock in free() called from a signal handler

[Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx>]

On Feb 19,  9:23am, Bart Schaefer wrote:
}
} In every case I can remember so far, when this happens it means that we
} ought to be using the signal queuing macros at a scope outside the call
} to the malloc library.
}  [...]
} So ... per my very first remark above, it's probably worth examining the
} context in execcmd() to see if signal queuing is appropriate there, or
} if we just need it in setunderscore().  But it may also make sense to
} replace that one last malloc() and put queuing in zfree()/zsfree() too.

(Above edited for silly typo.)

There doesn't appear to be an appropriate scope for queuing signals in
execcmd().  However, setunderscore() and all of the functions in text.c 
manipulate global state, so they should be considered non-reentrant.

The following patch does not yet put signal queuing into zfree(), but
does wrap it around setunderscore() and the three non-static functions
in text.c, as well as replace that one remaining malloc() with zalloc().
Hopefully this doesn't slow down execution noticeably.


diff --git a/Src/exec.c b/Src/exec.c
index 947b815..1a6149a 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -2337,6 +2337,7 @@ addvars(Estate state, Wordcode pc, int addflags)
 void
 setunderscore(char *str)
 {
+    queue_signals();
     if (str && *str) {
 	int l = strlen(str) + 1, nl = (l + 31) & ~31;
 
@@ -2354,6 +2355,7 @@ setunderscore(char *str)
 	*zunderscore = '\0';
 	underscoreused = 1;
     }
+    unqueue_signals();
 }
 
 /* These describe the type of expansions that need to be done on the words
@@ -5319,7 +5321,7 @@ execsave(void)
 {
     struct execstack *es;
 
-    es = (struct execstack *) malloc(sizeof(struct execstack));
+    es = (struct execstack *) zalloc(sizeof(struct execstack));
     es->list_pipe_pid = list_pipe_pid;
     es->nowait = nowait;
     es->pline_level = pline_level;
diff --git a/Src/text.c b/Src/text.c
index 75f642c..b58c251 100644
--- a/Src/text.c
+++ b/Src/text.c
@@ -173,6 +173,8 @@ getpermtext(Eprog prog, Wordcode c, int start_indent)
 {
     struct estate s;
 
+    queue_signals();
+
     if (!c)
 	c = prog->prog;
 
@@ -193,6 +195,9 @@ getpermtext(Eprog prog, Wordcode c, int start_indent)
     *tptr = '\0';
     freeeprog(prog);		/* mark as unused */
     untokenize(tbuf);
+
+    unqueue_signals();
+
     return tbuf;
 }
 
@@ -206,6 +211,8 @@ getjobtext(Eprog prog, Wordcode c)
 
     struct estate s;
 
+    queue_signals();
+
     if (!c)
 	c = prog->prog;
 
@@ -224,6 +231,9 @@ getjobtext(Eprog prog, Wordcode c)
     *tptr = '\0';
     freeeprog(prog);		/* mark as unused */
     untokenize(jbuf);
+
+    unqueue_signals();
+
     return jbuf;
 }
 
@@ -883,6 +893,9 @@ getredirs(LinkList redirs)
 	">", ">|", ">>", ">>|", "&>", "&>|", "&>>", "&>>|", "<>", "<",
 	"<<", "<<-", "<<<", "<&", ">&", NULL /* >&- */, "<", ">"
     };
+
+    queue_signals();
+
     taddchr(' ');
     for (n = firstnode(redirs); n; incnode(n)) {
 	Redir f = (Redir) getdata(n);
@@ -970,4 +983,6 @@ getredirs(LinkList redirs)
 	}
     }
     tptr--;
+
+    unqueue_signals();
 }

-- 
Barton E. Schaefer