zsh 5.3.1 crashes on completion

zsh 5.3.1 crashes on completion

[ChenYao]

Hi,

It seems that zsh is attempting to double free memory when autocompleting `fc` or `r`.  And it causes zsh crash.

% echo $ZSH_VERSION
5.3.1
% `r`<press tab here>
*** glibc detected *** ./zsh: double free or corruption (fasttop): 0x0000000000887940 ***
======= Backtrace: =========
/lib64/libc.so.6[0x390e275e66]
./zsh(hend+0x5d4)[0x445f04]
./zsh(loop+0x6a)[0x4492ba]
./zsh(bin_fc+0x1c27)[0x4223f7]
./zsh(execbuiltin+0x42c)[0x42543c]
./zsh[0x42f251]
./zsh[0x42fcf2]
./zsh[0x430126]
./zsh(execlist+0x962)[0x4322e2]
./zsh(execode+0xa6)[0x432656]
./zsh(getoutput+0x3e9)[0x4353c9]
./zsh[0x489f30]
./zsh(prefork+0x99)[0x48f159]
/root/zsh-test/lib/zsh/5.3.1/zsh/zle.so(+0x339a7)[0x7ffc74e699a7]
/root/zsh-test/lib/zsh/5.3.1/zsh/zle.so(+0x36b10)[0x7ffc74e6cb10]
/root/zsh-test/lib/zsh/5.3.1/zsh/zle.so(completecall+0x40)[0x7ffc74e677c0]
/root/zsh-test/lib/zsh/5.3.1/zsh/zle.so(execzlefunc+0x76e)[0x7ffc74e5638e]
/root/zsh-test/lib/zsh/5.3.1/zsh/zle.so(zlecore+0x126)[0x7ffc74e567c6]
/root/zsh-test/lib/zsh/5.3.1/zsh/zle.so(zleread+0x70f)[0x7ffc74e5739f]
./zsh(zleentry+0xce)[0x44716e]
./zsh(ingetc+0x2ee)[0x44c21e]
./zsh[0x444266]
./zsh(zshlex+0x6e)[0x45637e]
./zsh(parse_event+0x26)[0x479216]
./zsh(loop+0x56)[0x4492a6]
./zsh(zsh_main+0x4ee)[0x44ac4e]
/lib64/libc.so.6(__libc_start_main+0xfd)[0x390e21ed5d]
./zsh[0x40f319]
======= Memory map: ========
00400000-004b9000 r-xp 00000000 ca:01 1093254                            /root/zsh-test/bin/zsh
006b8000-006bf000 rw-p 000b8000 ca:01 1093254                            /root/zsh-test/bin/zsh
006bf000-006d2000 rw-p 00000000 00:00 0 
01884000-019ca000 rw-p 00000000 00:00 0                                  [heap]
339e800000-339e817000 r-xp 00000000 ca:01 303042                         /lib64/libpthread-2.12.so
339e817000-339ea17000 ---p 00017000 ca:01 303042                         /lib64/libpthread-2.12.so
339ea17000-339ea18000 r--p 00017000 ca:01 303042                         /lib64/libpthread-2.12.so
339ea18000-339ea19000 rw-p 00018000 ca:01 303042                         /lib64/libpthread-2.12.so
339ea19000-339ea1d000 rw-p 00000000 00:00 0 
390da00000-390da20000 r-xp 00000000 ca:01 262158                         /lib64/ld-2.12.so
390dc1f000-390dc20000 r--p 0001f000 ca:01 262158                         /lib64/ld-2.12.so
390dc20000-390dc21000 rw-p 00020000 ca:01 262158                         /lib64/ld-2.12.so
390dc21000-390dc22000 rw-p 00000000 00:00 0 
390e200000-390e38a000 r-xp 00000000 ca:01 262162                         /lib64/libc-2.12.so
390e38a000-390e58a000 ---p 0018a000 ca:01 262162                         /lib64/libc-2.12.so
390e58a000-390e58e000 r--p 0018a000 ca:01 262162                         /lib64/libc-2.12.so
390e58e000-390e58f000 rw-p 0018e000 ca:01 262162                         /lib64/libc-2.12.so
390e58f000-390e594000 rw-p 00000000 00:00 0 
390ea00000-390ea02000 r-xp 00000000 ca:01 262166                         /lib64/libdl-2.12.so
390ea02000-390ec02000 ---p 00002000 ca:01 262166                         /lib64/libdl-2.12.so
390ec02000-390ec03000 r--p 00002000 ca:01 262166                         /lib64/libdl-2.12.so
390ec03000-390ec04000 rw-p 00003000 ca:01 262166                         /lib64/libdl-2.12.so
390f200000-390f283000 r-xp 00000000 ca:01 262273                         /lib64/libm-2.12.so
390f283000-390f482000 ---p 00083000 ca:01 262273                         /lib64/libm-2.12.so
390f482000-390f483000 r--p 00082000 ca:01 262273                         /lib64/libm-2.12.so
390f483000-390f484000 rw-p 00083000 ca:01 262273                         /lib64/libm-2.12.so
39d8200000-39d8216000 r-xp 00000000 ca:01 269638                         /lib64/libgcc_s-4.4.7-20120601.so.1
39d8216000-39d8415000 ---p 00016000 ca:01 269638                         /lib64/libgcc_s-4.4.7-20120601.so.1
39d8415000-39d8416000 rw-p 00015000 ca:01 269638                         /lib64/libgcc_s-4.4.7-20120601.so.1
39ed000000-39ed007000 r-xp 00000000 ca:01 303046                         /lib64/librt-2.12.so
39ed007000-39ed206000 ---p 00007000 ca:01 303046                         /lib64/librt-2.12.so
39ed206000-39ed207000 r--p 00006000 ca:01 303046                         /lib64/librt-2.12.so
39ed207000-39ed208000 rw-p 00007000 ca:01 303046                         /lib64/librt-2.12.so
3c7a800000-3c7a81d000 r-xp 00000000 ca:01 274439                         /lib64/libtinfo.so.5.7
3c7a81d000-3c7aa1c000 ---p 0001d000 ca:01 274439                         /lib64/libtinfo.so.5.7
3c7aa1c000-3c7aa20000 rw-p 0001c000 ca:01 274439                         /lib64/libtinfo.so.5.7
3c7aa20000-3c7aa21000 rw-p 00000000 00:00 0 
3c7ac00000-3c7ac2e000 r-xp 00000000 ca:01 262236                         /lib64/libncursesw.so.5.7
3c7ac2e000-3c7ae2d000 ---p 0002e000 ca:01 262236                         /lib64/libncursesw.so.5.7
3c7ae2d000-3c7ae2e000 rw-p 0002d000 ca:01 262236                         /lib64/libncursesw.so.5.7
7ffc73781000-7ffc73791000 r-xp 00000000 ca:01 1093263                    /root/zsh-test/lib/zsh/5.3.1/zsh/computil.so
7ffc73791000-7ffc73990000 ---p 00010000 ca:01 1093263                    /root/zsh-test/lib/zsh/5.3.1/zsh/computil.so
7ffc73990000-7ffc73991000 rw-p 0000f000 ca:01 1093263                    /root/zsh-test/lib/zsh/5.3.1/zsh/computil.so
7ffc73991000-7ffc73992000 rw-p 00000000 00:00 0 
7ffc73992000-7ffc73995000 r-xp 00000000 ca:01 1093255                    /root/zsh-test/lib/zsh/5.3.1/zsh/rlimits.so
7ffc73995000-7ffc73b95000 ---p 00003000 ca:01 1093255                    /root/zsh-test/lib/zsh/5.3.1/zsh/rlimits.so
7ffc73b95000-7ffc73b96000 rw-p 00003000 ca:01 1093255                    /root/zsh-test/lib/zsh/5.3.1/zsh/rlimits.so
7ffc73b96000-7ffc73b98000 r-xp 00000000 ca:01 1093266                    /root/zsh-test/lib/zsh/5.3.1/zsh/zleparameter.so
7ffc73b98000-7ffc73d97000 ---p 00002000 ca:01 1093266                    /root/zsh-test/lib/zsh/5.3.1/zsh/zleparameter.so
7ffc73d97000-7ffc73d98000 rw-p 00001000 ca:01 1093266                    /root/zsh-test/lib/zsh/5.3.1/zsh/zleparameter.so
7ffc73d98000-7ffc73d9a000 r-xp 00000000 ca:01 1093259                    /root/zsh-test/lib/zsh/5.3.1/zsh/terminfo.so
7ffc73d9a000-7ffc73f99000 ---p 00002000 ca:01 1093259                    /root/zsh-test/lib/zsh/5.3.1/zsh/terminfo.so
7ffc73f99000-7ffc73f9a000 rw-p 00001000 ca:01 1093259                    /root/zsh-test/lib/zsh/5.3.1/zsh/terminfo.so
7ffc73f9a000-7ffc73fa1000 r-xp 00000000 ca:01 1093260                    /root/zsh-test/lib/zsh/5.3.1/zsh/zutil.so
7ffc73fa1000-7ffc741a0000 ---p 00007000 ca:01 1093260                    /root/zsh-test/lib/zsh/5.3.1/zsh/zutil.so
7ffc741a0000-7ffc741a1000 rw-p 00006000 ca:01 1093260                    /root/zsh-test/lib/zsh/5.3.1/zsh/zutil.so
7ffc741a1000-7ffc741a4000 r-xp 00000000 ca:01 1093258                    /root/zsh-test/lib/zsh/5.3.1/zsh/stat.so
7ffc741a4000-7ffc743a3000 ---p 00003000 ca:01 1093258                    /root/zsh-test/lib/zsh/5.3.1/zsh/stat.so
7ffc743a3000-7ffc743a4000 rw-p 00002000 ca:01 1093258                    /root/zsh-test/lib/zsh/5.3.1/zsh/stat.so
7ffc743eb000-7ffc743ee000 r-xp 00000000 ca:01 1093256                    /root/zsh-test/lib/zsh/5.3.1/zsh/mathfunc.so
7ffc743ee000-7ffc745ee000 ---p 00003000 ca:01 1093256                    /root/zsh-test/lib/zsh/5.3.1/zsh/mathfunc.so
7ffc745ee000-7ffc745ef000 rw-p 00003000 ca:01 1093256                    /root/zsh-test/lib/zsh/5.3.1/zsh/mathfunc.so
7ffc745ef000-7ffc745f0000 r-xp 00000000 ca:01 1093264                    /root/zsh-test/lib/zsh/5.3.1/zsh/deltochar.so
7ffc745f0000-7ffc747f0000 ---p 00001000 ca:01 1093264                    /root/zsh-test/lib/zsh/5.3.1/zsh/deltochar.so
7ffc747f0000-7ffc747f1000 rw-p 00001000 ca:01 1093264                    /root/zsh-test/lib/zsh/5.3.1/zsh/deltochar.so
7ffc747f1000-7ffc747ff000 r-xp 00000000 ca:01 1093262                    /root/zsh-test/lib/zsh/5.3.1/zsh/complist.so
7ffc747ff000-7ffc749ff000 ---p 0000e000 ca:01 1093262                    /root/zsh-test/lib/zsh/5.3.1/zsh/complist.so

Re: zsh 5.3.1 crashes on completion

[Bart Schaefer]

On May 11, 10:35am, ChenYao wrote:
}
} It seems that zsh is attempting to double free memory when
} autocompleting `fc` or `r`. And it causes zsh crash.

I'm NOT able to reproduce this from "zsh -f" with the most recent git
checkout.

Completing after an expression in backticks causes that expression to
be executed inside the completion system, which means among other things
that `fc` tries to run the editor without any terminal, and then any
terminal control sequences that the editor emits may end up as part
of the command name that is next attempted to run; so it's possible
that the choice of editor affects what ends up getting processed into
the command buffer, which might in turn change whether the crash is
triggered.

Or it might just be that any of several other recent fixes has also
swept up this one.

Re: zsh 5.3.1 crashes on completion

[Daniel Shahaf]

Bart Schaefer wrote on Thu, May 11, 2017 at 14:19:03 -0700:
> On May 11, 10:35am, ChenYao wrote:
> }
> } It seems that zsh is attempting to double free memory when
> } autocompleting `fc` or `r`. And it causes zsh crash.
> 
> I'm NOT able to reproduce this from "zsh -f" with the most recent git
> checkout.
> 
> Completing after an expression in backticks

I parsed ChenYao's backticks as quotation marks, so I thought he was
talking about completion either at

    LBUFFER="r" RBUFFER=""

or at

    LBUFFER="r " RBUFFER=""

.

ChenYao: can you show us the exact partial command that has been entered
when you invoke completion and provoke the crash?  Best show it on
a line by itself so there is no ambiguity.  Thanks.

Re: zsh 5.3.1 crashes on completion

[Bart Schaefer]

On Fri, May 12, 2017 at 9:16 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>
> ChenYao: can you show us the exact partial command that has been entered
> when you invoke completion and provoke the crash?  Best show it on
> a line by itself so there is no ambiguity.  Thanks.

His original message had:

% echo $ZSH_VERSION
5.3.1
% `r`<press tab here>

That seems pretty unambiguous.

Re: zsh 5.3.1 crashes on completion

[ChenYao]

[-- Attachment #1: Type: text/plain, Size: 1135 bytes --]

 I find that "setopt histignorealldups" in my zshrc causes the crash.

Reproduce from "zsh -f”:

# setopt histignorealldups
# echo $ZSH_VERSION
# `r`<press tab here, then crash>

Screenshot: http://imgur.com/hK9CP9R <http://imgur.com/hK9CP9R>

> On May 13, 2017, at 12:16 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> 
> Bart Schaefer wrote on Thu, May 11, 2017 at 14:19:03 -0700:
>> On May 11, 10:35am, ChenYao wrote:
>> }
>> } It seems that zsh is attempting to double free memory when
>> } autocompleting `fc` or `r`. And it causes zsh crash.
>> 
>> I'm NOT able to reproduce this from "zsh -f" with the most recent git
>> checkout.
>> 
>> Completing after an expression in backticks
> 
> I parsed ChenYao's backticks as quotation marks, so I thought he was
> talking about completion either at
> 
>    LBUFFER="r" RBUFFER=""
> 
> or at
> 
>    LBUFFER="r " RBUFFER=""
> 
> .
> 
> ChenYao: can you show us the exact partial command that has been entered
> when you invoke completion and provoke the crash?  Best show it on
> a line by itself so there is no ambiguity.  Thanks.
> 
> 

Re: zsh 5.3.1 crashes on completion

[Bart Schaefer]

On May 13, 10:02am, ChenYao wrote:
} 
}  I find that "setopt histignorealldups" in my zshrc causes the crash.
} 
} Reproduce from "zsh -f":
} 
} # setopt histignorealldups
} # echo $ZSH_VERSION
} # `r`<press tab here, then crash>

For the record, the interactive parent shell doesn't crash, the shell
that crashes is the one executing the "r" command in the subshell
created by the substitution.

Also for the record it's relatively dangerous to run "r" in backticks
in the first place, because if you do it twice in a row (or even do it
once and then follow that with "r" with no backticks) the shell will
end up in an infinite loop.  The failsafe code that keeps "r" from
re-executing itself is defeated by the quotes.

The crash occurs when the history in the subshell is updated to remove
the duplicate of the previous command.  Adding new history events is
disabled in subshells so the new event that is supposed to replace the
old one does not exist, and the code wanders off into neverland.

I'm not exactly sure where to fix this, or whether there's something
that can be done about the infinite loop (other than disabling "r"
in subshells entirely).

Re: zsh 5.3.1 crashes on completion

[Peter Stephenson]

On Sat, 13 May 2017 11:23:13 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> The crash occurs when the history in the subshell is updated to remove
> the duplicate of the previous command.  Adding new history events is
> disabled in subshells so the new event that is supposed to replace the
> old one does not exist, and the code wanders off into neverland.
> 
> I'm not exactly sure where to fix this, or whether there's something
> that can be done about the infinite loop (other than disabling "r"
> in subshells entirely).

I suppose removing duplicates could be disabled in subshells as well?

pws

Re: zsh 5.3.1 crashes on completion

[Bart Schaefer]

On May 15, 10:28am, Peter Stephenson wrote:
} Subject: Re: zsh 5.3.1 crashes on completion
}
} On Sat, 13 May 2017 11:23:13 -0700
} Bart Schaefer <schaefer@brasslantern.com> wrote:
} > The crash occurs when the history in the subshell is updated to remove
} > the duplicate of the previous command.  Adding new history events is
} > disabled in subshells so the new event that is supposed to replace the
} > old one does not exist, and the code wanders off into neverland.
} 
} I suppose removing duplicates could be disabled in subshells as well?

It's a bit more subtle than that.  The current history line (chline)
is actually pointing into the previous event, which is being taken as
a duplicate of *itself*.

One tangent seems to be that in a subshell, "save" in hend() ought to
become zero, because we never (?) want to write out the history of a
subshell.  But without resorting to another hack on zsh_subshell like
I did in entersubsh(), I don't know how to tell in hend() that the
history doesn't need to be saved.

The reason this crashes in completion seems to be that loop() is being
called before ZLE exits, so the parent loop() is still "open" and no
previous call to hend() has been done.  Otherwise this would fail on
*any* use of `r`.  (Expand-word will trip the bug as well.)

So maybe, really, loop() needs an entry point that doesn't use hbegin()
and hend().  Or maybe that's overkill.  Like I said, I don't know at
which of all these places to apply a fix.

Re: zsh 5.3.1 crashes on completion

[Peter Stephenson]

On Mon, 15 May 2017 13:33:54 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> It's a bit more subtle than that.  The current history line (chline)
> is actually pointing into the previous event, which is being taken as
> a duplicate of *itself*.

This is certainly murky.  The loop(0, 1) in bin_fc is a one-off, not
like anything else in the shell, and fc, as you already implied, isn't
really fit for running non-interactively anyway.

zcontext_save() in loop() will run, since this isn't a top-level loop,
which will save and clear chline.  So it should be the case the the loop
we have for fc has a clean history state.  So I wonder if something else to do
with the state (that maybe we should be saving and clearing at the same
time?) is screwy owing to not having finished the top-level history
line.  Either that, or something is running twice in the subshell when
it shouldn't.

I'm not seeing the falling call debug trigger, anyway, though it's quite
hard to see in the case with the memory corruption which is reporting
errors from glibc.  Promoting it to a hard error with a return didn't
seem to make any difference, either.

pws

diff --git a/Src/hist.c b/Src/hist.c
index 350688d..c68b2ae 100644
--- a/Src/hist.c
+++ b/Src/hist.c
@@ -1035,6 +1035,7 @@ hbegin(int dohist)
 	stophist = (!interact || unset(SHINSTDIN)) ? 2 : 0;
     else
 	stophist = 0;
+    DPUTS(chline != NULL, "chline set at start of history");
     /*
      * pws: We used to test for "|| (inbufflags & INP_ALIAS)"
      * in this test, but at this point we don't have input

Re: zsh 5.3.1 crashes on completion

[Peter Stephenson]

On Tue, 16 May 2017 10:48:31 +0100
Peter Stephenson <p.stephenson@samsung.com> wrote:
> zcontext_save() in loop() will run, since this isn't a top-level loop,
> which will save and clear chline.  So it should be the case the the loop
> we have for fc has a clean history state.  So I wonder if something else to do
> with the state (that maybe we should be saving and clearing at the same
> time?) is screwy owing to not having finished the top-level history
> line.

(BTW I needed to use standard malloc for this, it wasn't showing up with
zsh's own.)

Hmm...  I think the bad free comes because curline, which points to
chline, is part of the history ring and is being freed when we free
that.  So at hend() chline is already free.

Background: curline is a static structure with direct references to the
current history line.  We insert it into the history ring for
convenience in searching as necessary.  However, its memory status is
different from that of other history ring entries, which is obviously
a hostage to fortune.

This attempts to be smarter about linking curline into the history ring
and unlinking it.  This seems to have good effects, but definitely needs
a further look over.

pws

diff --git a/Src/hashtable.c b/Src/hashtable.c
index ba5faad..c34744c 100644
--- a/Src/hashtable.c
+++ b/Src/hashtable.c
@@ -1448,6 +1448,7 @@ freehistdata(Histent he, int unlink)
     if (!(he->node.flags & (HIST_DUP | HIST_TMPSTORE)))
 	removehashnode(histtab, he->node.nam);
 
+    DPUTS(he->node.nam == chline, "Attempt to free chline in history data");
     zsfree(he->node.nam);
     if (he->nwords)
 	zfree(he->words, he->nwords*2*sizeof(short));
diff --git a/Src/hist.c b/Src/hist.c
index 350688d..636c315 100644
--- a/Src/hist.c
+++ b/Src/hist.c
@@ -87,6 +87,9 @@ mod_export zlong curhist;
 /**/
 struct histent curline;
 
+/***/
+int curline_linked;
+
 /* current line count of allocated history entries */
 
 /**/
@@ -261,6 +264,9 @@ hist_context_save(struct hist_stack *hs, int toplevel)
      */
     hs->cstack = cmdstack;
     hs->csp = cmdsp;
+    hs->curline_linked = curline_linked;
+
+    unlinkcurline();
 
     stophist = 0;
     chline = NULL;
@@ -300,6 +306,10 @@ hist_context_restore(const struct hist_stack *hs, int toplevel)
 	zfree(cmdstack, CMDSTACKSZ);
     cmdstack = hs->cstack;
     cmdsp = hs->csp;
+    unlinkcurline();
+    curline_linked = hs->curline_linked;
+    if (curline_linked)
+	linkcurline();
 }
 
 /*
@@ -990,6 +1000,7 @@ nohwe(void)
 
 /* these functions handle adding/removing curline to/from the hist_ring */
 
+/**/
 static void
 linkcurline(void)
 {
@@ -1002,11 +1013,15 @@ linkcurline(void)
 	hist_ring = &curline;
     }
     curline.histnum = ++curhist;
+    curline_linked = 1;
 }
 
+/**/
 static void
 unlinkcurline(void)
 {
+    if (!curline_linked)
+	return;
     curline.up->down = curline.down;
     curline.down->up = curline.up;
     if (hist_ring == &curline) {
@@ -1016,6 +1031,7 @@ unlinkcurline(void)
 	    hist_ring = curline.up;
     }
     curhist--;
+    curline_linked = 0;
 }
 
 /* initialize the history mechanism */
@@ -1035,6 +1051,7 @@ hbegin(int dohist)
 	stophist = (!interact || unset(SHINSTDIN)) ? 2 : 0;
     else
 	stophist = 0;
+    DPUTS(chline != NULL, "chline set at start of history");
     /*
      * pws: We used to test for "|| (inbufflags & INP_ALIAS)"
      * in this test, but at this point we don't have input
@@ -1292,11 +1309,10 @@ putoldhistentryontop(short keep_going)
 Histent
 prepnexthistent(void)
 {
-    Histent he; 
-    int curline_in_ring = hist_ring == &curline;
+    Histent he;
+    int relink_curline = curline_linked;
 
-    if (curline_in_ring)
-	unlinkcurline();
+    unlinkcurline();
     if (hist_ring && hist_ring->node.flags & HIST_TMPSTORE) {
 	curhist--;
 	freehistnode(&hist_ring->node);
@@ -1320,7 +1336,7 @@ prepnexthistent(void)
 	he = hist_ring;
     }
     he->histnum = ++curhist;
-    if (curline_in_ring)
+    if (relink_curline)
 	linkcurline();
     return he;
 }
@@ -1395,8 +1411,7 @@ hend(Eprog prog)
     queue_signals();
     if (histdone & HISTFLAG_SETTY)
 	settyinfo(&shttyinfo);
-    if (!(histactive & HA_NOINC))
-	unlinkcurline();
+    unlinkcurline();
     if (histactive & HA_NOINC) {
 	zfree(chline, hlinesz);
 	zfree(chwords, chwordlen*sizeof(short));
@@ -3624,7 +3639,7 @@ int
 pushhiststack(char *hf, zlong hs, zlong shs, int level)
 {
     struct histsave *h;
-    int curline_in_ring = (histactive & HA_ACTIVE) && hist_ring == &curline;
+    int relink_curline = curline_linked;
 
     if (histsave_stack_pos == histsave_stack_size) {
 	histsave_stack_size += 5;
@@ -3632,8 +3647,7 @@ pushhiststack(char *hf, zlong hs, zlong shs, int level)
 			    histsave_stack_size * sizeof (struct histsave));
     }
 
-    if (curline_in_ring)
-	unlinkcurline();
+    unlinkcurline();
 
     h = &histsave_stack[histsave_stack_pos++];
 
@@ -3668,7 +3682,7 @@ pushhiststack(char *hf, zlong hs, zlong shs, int level)
     savehistsiz = shs;
     inithist(); /* sets histtab */
 
-    if (curline_in_ring)
+    if (relink_curline)
 	linkcurline();
 
     return histsave_stack_pos;
@@ -3680,13 +3694,12 @@ int
 pophiststack(void)
 {
     struct histsave *h;
-    int curline_in_ring = (histactive & HA_ACTIVE) && hist_ring == &curline;
+    int relink_curline = curline_linked;
 
     if (histsave_stack_pos == 0)
 	return 0;
 
-    if (curline_in_ring)
-	unlinkcurline();
+    unlinkcurline();
 
     deletehashtable(histtab);
     zsfree(lasthist.text);
@@ -3709,7 +3722,7 @@ pophiststack(void)
     histsiz = h->histsiz;
     savehistsiz = h->savehistsiz;
 
-    if (curline_in_ring)
+    if (relink_curline)
 	linkcurline();
 
     return histsave_stack_pos + 1;
diff --git a/Src/zsh.h b/Src/zsh.h
index 22f73f8..405b274 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -2931,6 +2931,7 @@ struct hist_stack {
     void (*addtoline) _((int));
     unsigned char *cstack;
     int csp;
+    int curline_linked;
 };
 
 /*

Re: zsh 5.3.1 crashes on completion

[Bart Schaefer]

On May 16, 10:48am, Peter Stephenson wrote:
}
} I'm not seeing the falling call debug trigger
} 
} +    DPUTS(chline != NULL, "chline set at start of history");

You wouldn't see that one -- chline *is* set, it's just not set to what
it normally would be (as I think your next message describes).

Re: zsh 5.3.1 crashes on completion

[Bart Schaefer]

On May 16, 12:05pm, Peter Stephenson wrote:
}
}      cmdsp = hs->csp;
} +    unlinkcurline();
} +    curline_linked = hs->curline_linked;
} +    if (curline_linked)
} +	linkcurline();
}  }

Shouldn't that be

    unlinkcurline();
    if (hs->curline_linked)
        linkcurline();

??  unlinkcurline() will set curline_linked = 0 and linkcurline() will
set it back to 1 again?  Don't set curline_linked before linking it?

} +    DPUTS(chline != NULL, "chline set at start of history");

Actual error is that chline NOT set at start?

Otherwise looks OK to me.

Re: zsh 5.3.1 crashes on completion

[Peter Stephenson]

On Wed, 17 May 2017 11:18:36 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On May 16, 12:05pm, Peter Stephenson wrote:
> }
> }      cmdsp = hs->csp;
> } +    unlinkcurline();
> } +    curline_linked = hs->curline_linked;
> } +    if (curline_linked)
> } +	linkcurline();
> }  }
> 
> Shouldn't that be
> 
>     unlinkcurline();
>     if (hs->curline_linked)
>         linkcurline();

It doesn't actually make a difference since linkcurline() doesn't check
the current status, but it's probably more logical that way, in case
someone adds extra checking.

> ??  unlinkcurline() will set curline_linked = 0 and linkcurline() will
> set it back to 1 again?  Don't set curline_linked before linking it?
> 
> } +    DPUTS(chline != NULL, "chline set at start of history");
> 
> Actual error is that chline NOT set at start?

No, chline shouldn't be set here.  If it is, that means we haven't saved
the previous status on the stack, which woulc clear chline, and if we
haven't done that, mayhem may result.  The sense of DPUTS() is confusingly
opposite to assert().

pws

Re: zsh 5.3.1 crashes on completion

[Jun T.]

With the latest git master, expand-history doesn't work correctly:

[1]% echo one
[2]% echo two
[3]% !!<TAB>   (or the key bound to expand-history)

then !! expands to 'echo one' (!!<Return> works as expected).

It seems this is caused by the patch:

2017/05/16 20:05, Peter Stephenson <p.stephenson@samsung.com> wrote:
> --- a/Src/hist.c
> +++ b/Src/hist.c
(snip)
> @@ -261,6 +264,9 @@ hist_context_save(struct hist_stack *hs, int toplevel)
>      */
>     hs->cstack = cmdstack;
>     hs->csp = cmdsp;
> +    hs->curline_linked = curline_linked;
> +
> +    unlinkcurline();

which is included in the commit 94014ff65bc2bdd752717792b156cdfcbc5b5c98

doexpandhist() calles zcontext_save(), which calls hist_context_save(),
which now calls unlinkcurline(). So when the event !! is searched
curline has been unlinked and curhist has been decremented, and
!! expands to event 1 (not 2).

Re: zsh 5.3.1 crashes on completion

[Bart Schaefer]

On May 26,  1:01am, Jun T. wrote:
}
} [1]% echo one
} [2]% echo two
} [3]% !!<TAB>   (or the key bound to expand-history)
} 
} then !! expands to 'echo one' (!!<Return> works as expected).

Yeah, I was noticing that the original test case (completion of `r`)
doesn't work correctly either, but hadn't yet determined whether that
was intentional.

Apparently this needs to be attacked a different way.

Re: zsh 5.3.1 crashes on completion

[Peter Stephenson]

On Fri, 26 May 2017 01:01:16 +0900
Jun T. <takimoto-j@kba.biglobe.ne.jp> wrote:
> With the latest git master, expand-history doesn't work correctly:
> 
> [1]% echo one
> [2]% echo two
> [3]% !!<TAB>   (or the key bound to expand-history)
> 
> then !! expands to 'echo one' (!!<Return> works as expected).

There are two obvious ways to go here.

First, link and unlink the current history line into the ring at
additional points when doing interactive expansion or completion.
Although this might well be safer in terms of structure it's quite hard
to track down the right points.  I haven't looked further.

Second, instead of using link / unlink, directly mark the current
history in some way so that it doesn't get freed.  As curline's not on
the saved history stack, the assumption (whether safe or not is an
entirely different matter) is that it's unique, i.e. there are not going
to be different notions of it at different lexical level. In that case
simply comparing against it so we don't free it will fix the immediate
problem.  This doesn't keep the structure at different nesting levels
as separate, but it looks like we can't. Here's a possible minimal fix
to the original problem based on that --- that means I really don't
know if there are other ramifications that may turn up, but it looks
like we need to track them down one by one rather than assuming
structure the shell doesn't have.

The first change is the actual fix, making the DPUTS as well as
the other new framework redundant.

pws

diff --git a/Src/hashtable.c b/Src/hashtable.c
index c34744c..6ec2ed2 100644
--- a/Src/hashtable.c
+++ b/Src/hashtable.c
@@ -1445,10 +1445,12 @@ freehistdata(Histent he, int unlink)
     if (!he)
 	return;
 
+    if (he == &curline)
+	return;
+
     if (!(he->node.flags & (HIST_DUP | HIST_TMPSTORE)))
 	removehashnode(histtab, he->node.nam);
 
-    DPUTS(he->node.nam == chline, "Attempt to free chline in history data");
     zsfree(he->node.nam);
     if (he->nwords)
 	zfree(he->words, he->nwords*2*sizeof(short));
diff --git a/Src/hist.c b/Src/hist.c
index 4c1039b..350688d 100644
--- a/Src/hist.c
+++ b/Src/hist.c
@@ -87,9 +87,6 @@ mod_export zlong curhist;
 /**/
 struct histent curline;
 
-/***/
-int curline_linked;
-
 /* current line count of allocated history entries */
 
 /**/
@@ -264,9 +261,6 @@ hist_context_save(struct hist_stack *hs, int toplevel)
      */
     hs->cstack = cmdstack;
     hs->csp = cmdsp;
-    hs->curline_linked = curline_linked;
-
-    unlinkcurline();
 
     stophist = 0;
     chline = NULL;
@@ -306,9 +300,6 @@ hist_context_restore(const struct hist_stack *hs, int toplevel)
 	zfree(cmdstack, CMDSTACKSZ);
     cmdstack = hs->cstack;
     cmdsp = hs->csp;
-    unlinkcurline();
-    if (hs->curline_linked)
-	linkcurline();
 }
 
 /*
@@ -999,7 +990,6 @@ nohwe(void)
 
 /* these functions handle adding/removing curline to/from the hist_ring */
 
-/**/
 static void
 linkcurline(void)
 {
@@ -1012,15 +1002,11 @@ linkcurline(void)
 	hist_ring = &curline;
     }
     curline.histnum = ++curhist;
-    curline_linked = 1;
 }
 
-/**/
 static void
 unlinkcurline(void)
 {
-    if (!curline_linked)
-	return;
     curline.up->down = curline.down;
     curline.down->up = curline.up;
     if (hist_ring == &curline) {
@@ -1030,7 +1016,6 @@ unlinkcurline(void)
 	    hist_ring = curline.up;
     }
     curhist--;
-    curline_linked = 0;
 }
 
 /* initialize the history mechanism */
@@ -1050,7 +1035,6 @@ hbegin(int dohist)
 	stophist = (!interact || unset(SHINSTDIN)) ? 2 : 0;
     else
 	stophist = 0;
-    DPUTS(chline != NULL, "chline set at start of history");
     /*
      * pws: We used to test for "|| (inbufflags & INP_ALIAS)"
      * in this test, but at this point we don't have input
@@ -1308,10 +1292,11 @@ putoldhistentryontop(short keep_going)
 Histent
 prepnexthistent(void)
 {
-    Histent he;
-    int relink_curline = curline_linked;
+    Histent he; 
+    int curline_in_ring = hist_ring == &curline;
 
-    unlinkcurline();
+    if (curline_in_ring)
+	unlinkcurline();
     if (hist_ring && hist_ring->node.flags & HIST_TMPSTORE) {
 	curhist--;
 	freehistnode(&hist_ring->node);
@@ -1335,7 +1320,7 @@ prepnexthistent(void)
 	he = hist_ring;
     }
     he->histnum = ++curhist;
-    if (relink_curline)
+    if (curline_in_ring)
 	linkcurline();
     return he;
 }
@@ -1410,7 +1395,8 @@ hend(Eprog prog)
     queue_signals();
     if (histdone & HISTFLAG_SETTY)
 	settyinfo(&shttyinfo);
-    unlinkcurline();
+    if (!(histactive & HA_NOINC))
+	unlinkcurline();
     if (histactive & HA_NOINC) {
 	zfree(chline, hlinesz);
 	zfree(chwords, chwordlen*sizeof(short));
@@ -3638,7 +3624,7 @@ int
 pushhiststack(char *hf, zlong hs, zlong shs, int level)
 {
     struct histsave *h;
-    int relink_curline = curline_linked;
+    int curline_in_ring = (histactive & HA_ACTIVE) && hist_ring == &curline;
 
     if (histsave_stack_pos == histsave_stack_size) {
 	histsave_stack_size += 5;
@@ -3646,7 +3632,8 @@ pushhiststack(char *hf, zlong hs, zlong shs, int level)
 			    histsave_stack_size * sizeof (struct histsave));
     }
 
-    unlinkcurline();
+    if (curline_in_ring)
+	unlinkcurline();
 
     h = &histsave_stack[histsave_stack_pos++];
 
@@ -3681,7 +3668,7 @@ pushhiststack(char *hf, zlong hs, zlong shs, int level)
     savehistsiz = shs;
     inithist(); /* sets histtab */
 
-    if (relink_curline)
+    if (curline_in_ring)
 	linkcurline();
 
     return histsave_stack_pos;
@@ -3693,12 +3680,13 @@ int
 pophiststack(void)
 {
     struct histsave *h;
-    int relink_curline = curline_linked;
+    int curline_in_ring = (histactive & HA_ACTIVE) && hist_ring == &curline;
 
     if (histsave_stack_pos == 0)
 	return 0;
 
-    unlinkcurline();
+    if (curline_in_ring)
+	unlinkcurline();
 
     deletehashtable(histtab);
     zsfree(lasthist.text);
@@ -3721,7 +3709,7 @@ pophiststack(void)
     histsiz = h->histsiz;
     savehistsiz = h->savehistsiz;
 
-    if (relink_curline)
+    if (curline_in_ring)
 	linkcurline();
 
     return histsave_stack_pos + 1;
diff --git a/Src/zsh.h b/Src/zsh.h
index 405b274..22f73f8 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -2931,7 +2931,6 @@ struct hist_stack {
     void (*addtoline) _((int));
     unsigned char *cstack;
     int csp;
-    int curline_linked;
 };
 
 /*

Re: zsh 5.3.1 crashes on completion

[Bart Schaefer]

On May 26, 10:19am, Peter Stephenson wrote:
}
} Second, instead of using link / unlink, directly mark the current
} history in some way so that it doesn't get freed.

This appears to be OK, from some rudimentary poking at it.

Re: zsh 5.3.1 crashes on completion

[Jun T.]

On 2017/05/26, at 18:19, Peter Stephenson <p.stephenson@samsung.com> wrote:
> 
> The first change is the actual fix, making the DPUTS as well as
> the other new framework redundant.

I've been using this for a day or two without any problems.