* zsh 5.3.1 crashes on completion @ 2017-05-11 2:35 ChenYao 2017-05-11 21:19 ` Bart Schaefer 0 siblings, 1 reply; 18+ messages in thread From: ChenYao @ 2017-05-11 2:35 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: zsh 5.3.1 crashes on completion 2017-05-11 2:35 zsh 5.3.1 crashes on completion ChenYao @ 2017-05-11 21:19 ` Bart Schaefer 2017-05-12 16:16 ` Daniel Shahaf 0 siblings, 1 reply; 18+ messages in thread From: Bart Schaefer @ 2017-05-11 21:19 UTC (permalink / raw) To: ChenYao, zsh-workers 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: zsh 5.3.1 crashes on completion 2017-05-11 21:19 ` Bart Schaefer @ 2017-05-12 16:16 ` Daniel Shahaf 2017-05-12 16:19 ` Bart Schaefer 2017-05-13 2:02 ` ChenYao 0 siblings, 2 replies; 18+ messages in thread From: Daniel Shahaf @ 2017-05-12 16:16 UTC (permalink / raw) To: zsh-workers, ChenYao 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: zsh 5.3.1 crashes on completion 2017-05-12 16:16 ` Daniel Shahaf @ 2017-05-12 16:19 ` Bart Schaefer 2017-05-13 2:02 ` ChenYao 1 sibling, 0 replies; 18+ messages in thread From: Bart Schaefer @ 2017-05-12 16:19 UTC (permalink / raw) To: zsh-workers 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: zsh 5.3.1 crashes on completion 2017-05-12 16:16 ` Daniel Shahaf 2017-05-12 16:19 ` Bart Schaefer @ 2017-05-13 2:02 ` ChenYao 2017-05-13 18:23 ` Bart Schaefer 1 sibling, 1 reply; 18+ messages in thread From: ChenYao @ 2017-05-13 2:02 UTC (permalink / raw) To: Daniel Shahaf; +Cc: zsh-workers, 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. > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: zsh 5.3.1 crashes on completion 2017-05-13 2:02 ` ChenYao @ 2017-05-13 18:23 ` Bart Schaefer 2017-05-15 9:28 ` Peter Stephenson 0 siblings, 1 reply; 18+ messages in thread From: Bart Schaefer @ 2017-05-13 18:23 UTC (permalink / raw) To: zsh-workers, ChenYao 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). ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: zsh 5.3.1 crashes on completion 2017-05-13 18:23 ` Bart Schaefer @ 2017-05-15 9:28 ` Peter Stephenson 2017-05-15 20:33 ` Bart Schaefer 0 siblings, 1 reply; 18+ messages in thread From: Peter Stephenson @ 2017-05-15 9:28 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: zsh 5.3.1 crashes on completion 2017-05-15 9:28 ` Peter Stephenson @ 2017-05-15 20:33 ` Bart Schaefer 2017-05-16 9:48 ` Peter Stephenson 0 siblings, 1 reply; 18+ messages in thread From: Bart Schaefer @ 2017-05-15 20:33 UTC (permalink / raw) To: zsh-workers 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: zsh 5.3.1 crashes on completion 2017-05-15 20:33 ` Bart Schaefer @ 2017-05-16 9:48 ` Peter Stephenson 2017-05-16 11:05 ` Peter Stephenson 2017-05-17 18:07 ` Bart Schaefer 0 siblings, 2 replies; 18+ messages in thread From: Peter Stephenson @ 2017-05-16 9:48 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: zsh 5.3.1 crashes on completion 2017-05-16 9:48 ` Peter Stephenson @ 2017-05-16 11:05 ` Peter Stephenson 2017-05-17 18:18 ` Bart Schaefer 2017-05-25 16:01 ` Jun T. 2017-05-17 18:07 ` Bart Schaefer 1 sibling, 2 replies; 18+ messages in thread From: Peter Stephenson @ 2017-05-16 11:05 UTC (permalink / raw) To: zsh-workers 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; }; /* ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: zsh 5.3.1 crashes on completion 2017-05-16 11:05 ` Peter Stephenson @ 2017-05-17 18:18 ` Bart Schaefer 2017-05-17 18:41 ` Peter Stephenson 2017-05-25 16:01 ` Jun T. 1 sibling, 1 reply; 18+ messages in thread From: Bart Schaefer @ 2017-05-17 18:18 UTC (permalink / raw) To: zsh-workers 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: zsh 5.3.1 crashes on completion 2017-05-17 18:18 ` Bart Schaefer @ 2017-05-17 18:41 ` Peter Stephenson 0 siblings, 0 replies; 18+ messages in thread From: Peter Stephenson @ 2017-05-17 18:41 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: zsh 5.3.1 crashes on completion 2017-05-16 11:05 ` Peter Stephenson 2017-05-17 18:18 ` Bart Schaefer @ 2017-05-25 16:01 ` Jun T. 2017-05-25 18:05 ` Bart Schaefer 2017-05-26 9:19 ` Peter Stephenson 1 sibling, 2 replies; 18+ messages in thread From: Jun T. @ 2017-05-25 16:01 UTC (permalink / raw) To: zsh-workers 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). ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: zsh 5.3.1 crashes on completion 2017-05-25 16:01 ` Jun T. @ 2017-05-25 18:05 ` Bart Schaefer 2017-05-26 9:19 ` Peter Stephenson 1 sibling, 0 replies; 18+ messages in thread From: Bart Schaefer @ 2017-05-25 18:05 UTC (permalink / raw) To: zsh-workers 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: zsh 5.3.1 crashes on completion 2017-05-25 16:01 ` Jun T. 2017-05-25 18:05 ` Bart Schaefer @ 2017-05-26 9:19 ` Peter Stephenson 2017-05-26 23:56 ` Bart Schaefer 2017-05-29 4:04 ` Jun T. 1 sibling, 2 replies; 18+ messages in thread From: Peter Stephenson @ 2017-05-26 9:19 UTC (permalink / raw) To: zsh-workers 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; }; /* ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: zsh 5.3.1 crashes on completion 2017-05-26 9:19 ` Peter Stephenson @ 2017-05-26 23:56 ` Bart Schaefer 2017-05-29 4:04 ` Jun T. 1 sibling, 0 replies; 18+ messages in thread From: Bart Schaefer @ 2017-05-26 23:56 UTC (permalink / raw) To: zsh-workers 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: zsh 5.3.1 crashes on completion 2017-05-26 9:19 ` Peter Stephenson 2017-05-26 23:56 ` Bart Schaefer @ 2017-05-29 4:04 ` Jun T. 1 sibling, 0 replies; 18+ messages in thread From: Jun T. @ 2017-05-29 4:04 UTC (permalink / raw) To: zsh-workers 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: zsh 5.3.1 crashes on completion 2017-05-16 9:48 ` Peter Stephenson 2017-05-16 11:05 ` Peter Stephenson @ 2017-05-17 18:07 ` Bart Schaefer 1 sibling, 0 replies; 18+ messages in thread From: Bart Schaefer @ 2017-05-17 18:07 UTC (permalink / raw) To: zsh-workers 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). ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2017-05-29 4:04 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-05-11 2:35 zsh 5.3.1 crashes on completion ChenYao 2017-05-11 21:19 ` Bart Schaefer 2017-05-12 16:16 ` Daniel Shahaf 2017-05-12 16:19 ` Bart Schaefer 2017-05-13 2:02 ` ChenYao 2017-05-13 18:23 ` Bart Schaefer 2017-05-15 9:28 ` Peter Stephenson 2017-05-15 20:33 ` Bart Schaefer 2017-05-16 9:48 ` Peter Stephenson 2017-05-16 11:05 ` Peter Stephenson 2017-05-17 18:18 ` Bart Schaefer 2017-05-17 18:41 ` Peter Stephenson 2017-05-25 16:01 ` Jun T. 2017-05-25 18:05 ` Bart Schaefer 2017-05-26 9:19 ` Peter Stephenson 2017-05-26 23:56 ` Bart Schaefer 2017-05-29 4:04 ` Jun T. 2017-05-17 18:07 ` Bart Schaefer
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/zsh/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).