Re: coredump on C-c

[Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx>]

On Wed, Oct 16, 2013 at 2:40 PM, Eitan Adler <lists@xxxxxxxxxxxxxx> wrote:
>
> I'm hoping to prod about this again, this time with a slightly
> different backtrace.

"Slightly" is a bit of an understatement; that's an almost entirely
different backtrace.

I'm also scratching my head a bit about why you're able to land
interrupts in inconvenient locations so easily when nobody else has
reported this kind of issue in several years.

What follows is somewhat stream-of-consciousness, sorry.

In this new case you appear to have a trap set on the INT signal,
which was invoked because you interrupted your setCurrentPS1
precmd-hook, and then had a CHLD signal come in while the INT trap was
unwinding itself.

All of which appears to have been possible because __vcs_dir was
taking a long time?

Signals are being blocked or queued in most of the appropriate places
-- the zhandler at the top of the backtrace is being run when zfree is
already finished and releases the signal queue, and the zhandler in
the middle of the trace is being invoked while signals are unblocked
to wait for _vcs_dir to exit.

At first I thought execrestore() needs to queue signals just as
endparamscope() does.  Why you (Eitan) seem to be uniquely able to
deliver signals into the middle of functions that are little more than
a few assignment statements must have something to do with your
hardware environment.  I guess both of those functions are a bit more
than that because they free memory in addition to moving pointers
around.

On closer examination I'm not sure that will work, because all of this
is happening inside waitforpid() which prefaces it with an explicit
dont_queue_signals().  So it's not impossible that we're also open to
improper signaling inside zfree() here, but the stack trace makes that
look unlikely.  Still, it seems to be possible to reorder operations
in execrestore() to avoid the immediate problem of exstack pointing to
the wrong thing.  That's at least worthwhile, though the possibility
of an execsave/execrestore pair occurring in the middle of an
execrestore in progress makes my head hurt, so I think perhaps both
changes are in order.

execsave(), incidentally, already does things in a "safe" order, so it
probably doesn't need the signal queuing.

diff --git a/Src/exec.c b/Src/exec.c
index de1b484..8efbbd4 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -5078,29 +5078,33 @@ execsave(void)
 void
 execrestore(void)
 {
-    struct execstack *en;
+    struct execstack *en = exstack;

     DPUTS(!exstack, "BUG: execrestore() without execsave()");
-    list_pipe_pid = exstack->list_pipe_pid;
-    nowait = exstack->nowait;
-    pline_level = exstack->pline_level;
-    list_pipe_child = exstack->list_pipe_child;
-    list_pipe_job = exstack->list_pipe_job;
-    strcpy(list_pipe_text, exstack->list_pipe_text);
-    lastval = exstack->lastval;
-    noeval = exstack->noeval;
-    badcshglob = exstack->badcshglob;
-    cmdoutpid = exstack->cmdoutpid;
-    cmdoutval = exstack->cmdoutval;
-    use_cmdoutval = exstack->use_cmdoutval;
-    trap_return = exstack->trap_return;
-    trap_state = exstack->trap_state;
-    trapisfunc = exstack->trapisfunc;
-    traplocallevel = exstack->traplocallevel;
-    noerrs = exstack->noerrs;
-    setunderscore(exstack->underscore);
-    zsfree(exstack->underscore);
-    en = exstack->next;
-    free(exstack);
-    exstack = en;
+
+    queue_signals();
+    exstack = exstack->next;
+
+    list_pipe_pid = en->list_pipe_pid;
+    nowait = en->nowait;
+    pline_level = en->pline_level;
+    list_pipe_child = en->list_pipe_child;
+    list_pipe_job = en->list_pipe_job;
+    strcpy(list_pipe_text, en->list_pipe_text);
+    lastval = en->lastval;
+    noeval = en->noeval;
+    badcshglob = en->badcshglob;
+    cmdoutpid = en->cmdoutpid;
+    cmdoutval = en->cmdoutval;
+    use_cmdoutval = en->use_cmdoutval;
+    trap_return = en->trap_return;
+    trap_state = en->trap_state;
+    trapisfunc = en->trapisfunc;
+    traplocallevel = en->traplocallevel;
+    noerrs = en->noerrs;
+    setunderscore(en->underscore);
+    zsfree(en->underscore);
+    free(en);
+
+    unqueue_signals();
 }