Coredump on C-c - followup

[Eitan Adler <lists@xxxxxxxxxxxxxx>]

Hey,

I'm on the list so I missed your last reply to me.  Please keep me on
the CC list ;)


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.

Heh.

> 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

Yes.  The trap is available here:
https://code.google.com/p/config/source/browse/zsh/precmd.zsh#143

My entire config environment is available in that repo.  I did not
mention it in my original emails since I completely forgot about it.

> 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?

My disk is quite slow, so I would not be surprised.

> 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.

Ha!

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

The patch below seems mangled.  Maybe this was the mail archive I
used.  Can you resend as a link or patch?

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();
 }



-- 
Eitan Adler