Segfault on completion with interactive mode

Segfault on completion with interactive mode

[Óscar García Amor]

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

Hi!

I found a segfault bug on ZSH 5.2, but is confirmed that can be reproduced
in master.

Steps to reproduce:
1. Run a `zsh -f`
2. Set this config.
```
zstyle :compinstall filename "${HOME}/.zshrc"
autoload -Uz compinit
compinit
zstyle ':completion:*' menu select interactive
zstyle ':completion:*:warnings' format '%BNo matching %b%d'
unsetopt menu_complete auto_list
```
3. Start write a command. For example, write `ba`
4. Press tab to show options (interactive mode appears)
5. Write a letter that not mach with any option (to generate a completion
warning)
6. Press backspace

Tachan! zsh: segmentation fault (core dumped)

danielsh, on IRC, send this coredump:
http://sprunge.us/BXDU

Greetings.

-- 
Óscar García Amor | ogarcia@connectical.com
Connectical | http://connectical.com

Re: Segfault on completion with interactive mode

[Bart Schaefer]

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

On Feb 10, 2016 8:40 AM, "Óscar García Amor" <ogarcia@connectical.com>
wrote:
>
> 4. Press tab to show options (interactive mode appears)
> 5. Write a letter that not mach with any option (to generate a completion
> warning)
> 6. Press backspace
>
> Tachan! zsh: segmentation fault (core dumped)

Hmm.  So, complistmatches() is called with an empty list of completions to
cause the message to be displayed.  It's then called again on backspace to
erase the message and draw a new list, but it chokes on the empty listing.

I can stop the coredump by doing nothing when the list is empty, but then
the message isn't displayed either.

Re: Segfault on completion with interactive mode

[Bart Schaefer]

[ Long diagnosis follows; read just the first and last paragraphs for
  the short version. ]

I think the problem is that "compadd -x" (which is used to show the
warnings) updates the current group of completions to have an empty
match with the warning as its explanation string.  Because interactive
mode is doing its own loop on getkeycmd(), the listing data is not
restored after "compadd -x" clobbers it, and so the next time around
the interactive mode loop, its idea of what is in the set of matches
does not correspond, and chaos ensues.

It comes down to this block of code in domenuselect():

		if (nmessages) {
		    showinglist = -2;
		    zrefresh();
		} else {
		    trashzle();
		    zsetterm();
		    if (tccan(TCCLEAREOD))
			tcout(TCCLEAREOD);
		    fputs("no matches\r", shout);
		    fflush(shout);
		    tcmultout(TCUP, TCMULTUP, nlnct);
		    showinglist = clearlist = 0;
		    clearflag = 1;
		    zrefresh();
		    showinglist = clearlist = 0;
		}
		statusline = NULL;

		goto getk;

And earlier:

    getk:

    	if (!do_last_key) {
	    zmult = 1;
	    cmd = getkeycmd();
	    /*
	     * On interrupt, we'll exit due to cmd being empty.
	     * Don't propagate the interrupt any further, which
	     * can screw up redrawing.
	     */
	    errflag &= ~ERRFLAG_INT;
	    if (mtab_been_reallocated) {
		do_last_key = 1;
		continue;
	    }
    	}
	do_last_key = 0;

When (nmessages) is true, we jump to getk:, do_last_key is false so
we arrive at getkeycmd(), and mtab_been_reallocated is true so after
reading the delete-char we continue back to the top of the "for (;;)"
loop and finally crash inside the call to zrefresh() at line 2550 --
which I *think* is intended to redraw the old list before highlighing
the desired item within it, but the old list has been replaced by the
warning message.

If there are no matches and (nmessages == 0), we go through the other
branch [beginning with trashzle()].  Thereafter everything is the
same except that we successfully redraw the list in zrefresh() and
go on as we were.

If showinglist is set to anything other than -2 in the (nmessages)
true branch, there's no crash but the messages aren't shown either.
If the list is invalidated (force listdat.valid = 0) then there also
isn't a crash but menu selection exits.

The thing is, I think domenuselect() has pushed all the right things
onto its linked list of Menustack structures, it just hasn't ever
restored it again.  But I'm not sure and I haven't figured out the
right place to restore it if so.  Other eyeballs?

Re: Segfault on completion with interactive mode

[Peter Stephenson]

What I saw when I tried this was a crash in singledraw() because
mgtab[0] was 1, suggesting it as a "marked" null pointer.  So presumably
this state is incompatible with state required when we call
singledraw(),

    if (!mnew && inselect && onlnct == nlnct && mlbeg >= 0 && mlbeg == molbeg)
        singledraw();

This appears to be a lot later than anything you're talking about.

Preumably that pile of stuff in complistmatches inside "if (mnew) {" is
the key to what should be consistent.  It sounds like in the code you're
talking about, that code had just run, because mtab_been_reallocated was
1, whereas this time round, when it crashes, it hadn't.
("mtab_been_reallocated" seems to be associated with the 'if you've got
global variables you don't really need a calling convention' programming
style.)

So at the level I'm looking at it, it looks vaguely like it's in a state
as if "mnew" has just been set, in which case we'd skip the
singledraw(), and somehow we've got back too soon before all the stuff
that happens after to fix it up, whatever that is.

Anyway, the summary is I've completely failed to understand even the
most basic points of the logic that sets up the completion list
consistently.

pws

Re: Segfault on completion with interactive mode

[Bart Schaefer]

On Feb 11, 12:49pm, Peter Stephenson wrote:
} Subject: Re: Segfault on completion with interactive mode
}
} What I saw when I tried this was a crash in singledraw() because
} mgtab[0] was 1, suggesting it as a "marked" null pointer.  So presumably
} this state is incompatible with state required when we call
} singledraw(),

Yes, I saw that as well.

} This appears to be a lot later than anything you're talking about.
 
The long description in my previous message was the result of several
(dozen) passes trying to track down WHY we arrive in singledraw() with
mgtab[0] = 1 in the case of

    zstyle ':completion:*:warnings' format '%BNo matching %b%d'

when the analogous case WITHOUT that style works exactly as designed.

The answer to "why?" appears to be that "compadd -x" has clobbered the
completion groups when displaying the warning.  Thus I believe that we
somehow need to save and restore state around the "compadd -x".  The
right place to do that seems to be in domenuselect() in the branch
that uses showinglist = -2, because it's that call to zrefresh() that
ultimately calls the hookdef that eventually does the "compadd -x".

Re: Segfault on completion with interactive mode

[Bart Schaefer]

This seems to do it.  It may be possible to further reconcile the
(nmessages) branch with the print-our-own "no matches" branch, but
I haven't tried yet.  The idea is:

- there's no match for the current input, so there's nothing to be
  selected in the previously displayed set of possible matches
- the listing has been trashed anyway by displaying the warning
- so, don't bother attempting to redraw or to find the (nonexistent)
  selected item in the (trashed) list until after the next keystroke
  has been processed
- at which point the original listing has been redrawn and all is well.


diff --git a/Src/Zle/complist.c b/Src/Zle/complist.c
index 937e1d1..162436b 100644
--- a/Src/Zle/complist.c
+++ b/Src/Zle/complist.c
@@ -34,8 +34,9 @@
 /* Information about the list shown. */
 
 /*
- * noselect: 1 if complistmatches indicated we shouldn't do selection.
- *           Tested in domenuselect.
+ * noselect: 1 if complistmatches indicated we shouldn't do selection;
+ *           -1 if interactive mode needs to reset the selection list.
+ *           Tested in domenuselect, and in complistmatches to skip redraw.
  * mselect:  Local copy of the index of the currently selected match.
  *           Initialised to the gnum entry of the current match for
  *           each completion.
@@ -1980,7 +1981,8 @@ complistmatches(UNUSED(Hookdef dummy), Chdata dat)
     }
 #endif
 
-    noselect = 0;
+    if (noselect > 0)
+	noselect = 0;
 
     if ((minfo.asked == 2 && mselect < 0) || nlnct >= zterm_lines) {
 	showinglist = 0;
@@ -2078,9 +2080,11 @@ complistmatches(UNUSED(Hookdef dummy), Chdata dat)
     last_cap = (char *) zhalloc(max_caplen + 1);
     *last_cap = '\0';
 
-    if (!mnew && inselect && onlnct == nlnct && mlbeg >= 0 && mlbeg == molbeg)
-        singledraw();
-    else if (!compprintlist(mselect >= 0) || !clearflag)
+    if (!mnew && inselect &&
+	onlnct == nlnct && mlbeg >= 0 && mlbeg == molbeg) {
+	if (!noselect)
+	    singledraw();
+    } else if (!compprintlist(mselect >= 0) || !clearflag)
 	noselect = 1;
 
     onlnct = nlnct;
@@ -2093,7 +2097,7 @@ complistmatches(UNUSED(Hookdef dummy), Chdata dat)
     popheap();
     opts[EXTENDEDGLOB] = extendedglob;
 
-    return noselect;
+    return (noselect < 0 ? 0 : noselect);
 }
 
 static int
@@ -2547,14 +2551,23 @@ domenuselect(Hookdef dummy, Chdata dat)
         } else {
             statusline = NULL;
         }
+	if (noselect < 0) {
+	    showinglist = clearlist = 0;
+	    clearflag = 1;
+	}
         zrefresh();
 	statusline = NULL;
         inselect = 1;
+	selected = 1;
         if (noselect) {
+	    if (noselect < 0) {
+		/* no selection until after processing keystroke */
+		noselect = 0;
+		goto getk;
+	    }
             broken = 1;
             break;
         }
-	selected = 1;
 	if (!i) {
 	    i = mcols * mlines;
 	    while (i--)
@@ -2752,6 +2765,7 @@ domenuselect(Hookdef dummy, Chdata dat)
 		if (nmessages) {
 		    showinglist = -2;
 		    zrefresh();
+		    noselect = -1;
 		} else {
 		    trashzle();
 		    zsetterm();