[PATCH] Fix #87 - Segfault fault when autocompleting after ">" in, "!> ."

[PATCH] Fix #87 - Segfault fault when autocompleting after ">" in, "!> ."

[Raphaël Jakse]

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

Dear zsh developers,

Bug #87 in the SF bug tracker (which is not used anymore if I understood 
correctly) claims that when hitting tab to complete after '>' in the 
string "!> cmake ..", zsh crashes. I could reproduce the bug that also 
appears with the string "!> .".

Here is a patch that works around this bug by checking whether s is null 
in the get_comp_string function (zle_tricky.c) before a code that seems 
to assume that s is not null. The get_comp_string function is full of 
warnings like "abandon all hopes" and "This function is a nightmare" so 
I didn't try to know whether s being null at this point is correct.

It is unclear to me how to send a patch for zsh so please let me know if 
something is wrong or if additional steps are necessary to apply this patch.

Best,
Raphaël


[-- Attachment #2: fix-completion-segfault.patch --]
[-- Type: text/x-patch, Size: 956 bytes --]

>From ced2387dc8dc65aecc34ecde29924810126a48be Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Rapha=C3=ABl=20Jakse?= <raphael.jakse@imag.fr>
Date: Fri, 25 Nov 2016 17:14:39 +0100
Subject: [PATCH] Fix #87 - Segfault when autocompleting after ">" in  "!> ."

The fix is a workaround that checks whether s is null in the get_comp_string
function (zle_tricky.c) before a code that seems to assume that s is not null
get_comp_string.
---
 Src/Zle/zle_tricky.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Src/Zle/zle_tricky.c b/Src/Zle/zle_tricky.c
index c8d3bb3..18ec47a 100644
--- a/Src/Zle/zle_tricky.c
+++ b/Src/Zle/zle_tricky.c
@@ -1518,7 +1518,7 @@ get_comp_string(void)
 	zlemetaline = tmp;
 	zlemetall = strlen(zlemetaline);
     }
-    if (t0 != STRING && t0 != TYPESET && inwhat != IN_MATH) {
+    if (!s || (t0 != STRING && t0 != TYPESET && inwhat != IN_MATH)) {
 	if (tmp) {
 	    tmp = NULL;
 	    linptr = zlemetaline;
-- 
2.9.3

Re: [PATCH] Fix #87 - Segfault fault when autocompleting after ">" in, "!> ."

[Bart Schaefer]

On Mon, Nov 28, 2016 at 5:06 AM, Raphaël Jakse <raphael.jakse@imag.fr> wrote:
>
> Here is a patch that works around this bug by checking whether s is null in
> the get_comp_string function (zle_tricky.c) before a code that seems to
> assume that s is not null.

Thanks for calling our attention to this.

The actual problem seems to be that the code assumed ztrdup(NULL)
would return empty string, when in fact it returns NULL:

diff --git a/Src/Zle/zle_tricky.c b/Src/Zle/zle_tricky.c
index c8d3bb3..d636373 100644
--- a/Src/Zle/zle_tricky.c
+++ b/Src/Zle/zle_tricky.c
@@ -1464,7 +1464,10 @@ get_comp_string(void)
        t0 = STRING;
     } else if (t0 == STRING || t0 == TYPESET) {
        /* We found a simple string. */
-       s = ztrdup(clwords[clwpos]);
+       if (clwords[clwpos])
+           s = ztrdup(clwords[clwpos]);
+       else
+           s = ztrdup("");
     } else if (t0 == ENVSTRING) {
        char sav;
        /* The cursor was inside a parameter assignment. */

The reason "we found a simple string" is because the completion system
inserts a phantom "x" at the cursor to be sure it can split the
current word into before/after substrings, so the parser is actually
handed "!>x" and asked for the token at "x".  The "x" is then removed
again, and clwpos (command line word position) ends up pointing at the
null terminator of the clwords array.

> It is unclear to me how to send a patch for zsh so please let me know if
> something is wrong or if additional steps are necessary to apply this patch.

What you did is fine, though we prefer that the attachment be of type
"text/plain" (which usually means you should not use ".patch" or
".diff" as the file name extension).

Re: [PATCH] Fix #87 - Segfault fault when autocompleting after ">" in, "!> ."

[Peter Stephenson]

On Mon, 28 Nov 2016 14:06:54 +0100
Raphaël Jakse <raphael.jakse@imag.fr> wrote:
> Bug #87 in the SF bug tracker (which is not used anymore if I understood 
> correctly) claims that when hitting tab to complete after '>' in the 
> string "!> cmake ..", zsh crashes. I could reproduce the bug that also 
> appears with the string "!> .".

Thanks, that's still the case and this is indeed a tricky piece of code.
Sending patches here is the right thing to do.

I think the following patch does a little better: I've tracked down the
case where this is failing, and it's in handling something that looks
like a redirection before the command word.  Here it looks a bit like a
redirection because of the > but not enough because of the !.

I haven't fully resolved redirection handling here (not sure exactly
what this mixture should really do at this point), but I have made the
particular aspect of it that was causing the problem a bit safer:
because the code here didn't properly know about the redirection it
thought it could reset the completion word when it found the command
word.  But because the lexer had detected a redirection and we'd
already passed the word where the cursor was, that was no longer safe.
Detecting we've already found the completion word looks like an
excellent reason for leaving the command line word array that we've
built up so far alone whatever else is happening.

I've also added a debug message if we find a string that's NULL, which
should also make it safe in the way you intended but with debug info for
developers if debugging is turned on.

The middle hunk is a bit of extra safety / readability.  Possibly some
extra DPUTS() would be a good idea there.

With a bit of luck (we don't tend to get much in get_comp_string()), the
condition is specific enough that this doesn't make anything else worse.

pws

diff --git a/Src/Zle/zle_tricky.c b/Src/Zle/zle_tricky.c
index c8d3bb3..3d86791 100644
--- a/Src/Zle/zle_tricky.c
+++ b/Src/Zle/zle_tricky.c
@@ -1305,8 +1305,12 @@ get_comp_string(void)
 	    zsfree(cmdstr);
 	    cmdstr = ztrdup(tokstr);
 	    cmdtok = tok;
-	    /* If everything before is a redirection, don't reset the index */
-	    if (wordpos != redirpos)
+	    /*
+	     * If everything before is a redirection, or anything
+	     * complicated enough that we've seen the word the
+	     * cursor is on, don't reset the index.
+	     */
+	    if (wordpos != redirpos && clwpos == -1)
 		wordpos = redirpos = 0;
 	} else if (tok == SEPER) {
 	    /*
@@ -1414,9 +1418,17 @@ get_comp_string(void)
 	/* If this is the word the cursor is in and we added a `x', *
 	 * remove it.                                               */
 	if (clwpos == wordpos++ && addedx) {
+	    int chuck_at, word_diff;
 	    zlemetacs_qsub = zlemetacs - qsub;
-	    chuck(&clwords[wordpos - 1][((zlemetacs_qsub - wb) >= sl) ?
-				 (sl - 1) : (zlemetacs_qsub - wb)]);
+	    word_diff = zlemetacs_qsub - wb;
+	    /* Ensure we chuck within the word... */
+	    if (word_diff >= sl)
+		chuck_at = sl -1;
+	    else if (word_diff < 0)
+		chuck_at = 0;
+	    else
+		chuck_at = word_diff;
+	    chuck(&clwords[wordpos - 1][chuck_at]);
 	}
     } while (tok != LEXERR && tok != ENDINPUT &&
 	     (tok != SEPER || (lexflags && tt0 == NULLTOK)));
@@ -1464,7 +1476,9 @@ get_comp_string(void)
 	t0 = STRING;
     } else if (t0 == STRING || t0 == TYPESET) {
 	/* We found a simple string. */
-	s = ztrdup(clwords[clwpos]);
+	s = clwords[clwpos];
+	DPUTS(!s, "Completion word has disappeared!");
+	s = ztrdup(s ? s : "");
     } else if (t0 == ENVSTRING) {
 	char sav;
 	/* The cursor was inside a parameter assignment. */

Re: [PATCH] Fix #87 - Segfault fault when autocompleting after ">" in, "!> ."

[Raphaël Jakse]

Bart, Peter,

Thanks for taking a look at it. Both of your patches seem more accurate 
than mine.
It would be nice if one of them could make it into zsh.

For your information, looking at this bug was / is part of my research. 
We try to combine monitoring techniques and interactive debugging. If 
you are interested, feel free to ask me questions and look at our our 
tool here: http://check-exec.forge.imag.fr/

Best,
Raphaël

Le 28/11/2016 à 17:57, Peter Stephenson a écrit :
> On Mon, 28 Nov 2016 14:06:54 +0100
> Raphaël Jakse <raphael.jakse@imag.fr> wrote:
>> Bug #87 in the SF bug tracker (which is not used anymore if I understood
>> correctly) claims that when hitting tab to complete after '>' in the
>> string "!> cmake ..", zsh crashes. I could reproduce the bug that also
>> appears with the string "!> .".
> Thanks, that's still the case and this is indeed a tricky piece of code.
> Sending patches here is the right thing to do.
>
> I think the following patch does a little better: I've tracked down the
> case where this is failing, and it's in handling something that looks
> like a redirection before the command word.  Here it looks a bit like a
> redirection because of the > but not enough because of the !.
>
> I haven't fully resolved redirection handling here (not sure exactly
> what this mixture should really do at this point), but I have made the
> particular aspect of it that was causing the problem a bit safer:
> because the code here didn't properly know about the redirection it
> thought it could reset the completion word when it found the command
> word.  But because the lexer had detected a redirection and we'd
> already passed the word where the cursor was, that was no longer safe.
> Detecting we've already found the completion word looks like an
> excellent reason for leaving the command line word array that we've
> built up so far alone whatever else is happening.
>
> I've also added a debug message if we find a string that's NULL, which
> should also make it safe in the way you intended but with debug info for
> developers if debugging is turned on.
>
> The middle hunk is a bit of extra safety / readability.  Possibly some
> extra DPUTS() would be a good idea there.
>
> With a bit of luck (we don't tend to get much in get_comp_string()), the
> condition is specific enough that this doesn't make anything else worse.
>
> pws
>
> diff --git a/Src/Zle/zle_tricky.c b/Src/Zle/zle_tricky.c
> index c8d3bb3..3d86791 100644
> --- a/Src/Zle/zle_tricky.c
> +++ b/Src/Zle/zle_tricky.c
> @@ -1305,8 +1305,12 @@ get_comp_string(void)
>   	    zsfree(cmdstr);
>   	    cmdstr = ztrdup(tokstr);
>   	    cmdtok = tok;
> -	    /* If everything before is a redirection, don't reset the index */
> -	    if (wordpos != redirpos)
> +	    /*
> +	     * If everything before is a redirection, or anything
> +	     * complicated enough that we've seen the word the
> +	     * cursor is on, don't reset the index.
> +	     */
> +	    if (wordpos != redirpos && clwpos == -1)
>   		wordpos = redirpos = 0;
>   	} else if (tok == SEPER) {
>   	    /*
> @@ -1414,9 +1418,17 @@ get_comp_string(void)
>   	/* If this is the word the cursor is in and we added a `x', *
>   	 * remove it.                                               */
>   	if (clwpos == wordpos++ && addedx) {
> +	    int chuck_at, word_diff;
>   	    zlemetacs_qsub = zlemetacs - qsub;
> -	    chuck(&clwords[wordpos - 1][((zlemetacs_qsub - wb) >= sl) ?
> -				 (sl - 1) : (zlemetacs_qsub - wb)]);
> +	    word_diff = zlemetacs_qsub - wb;
> +	    /* Ensure we chuck within the word... */
> +	    if (word_diff >= sl)
> +		chuck_at = sl -1;
> +	    else if (word_diff < 0)
> +		chuck_at = 0;
> +	    else
> +		chuck_at = word_diff;
> +	    chuck(&clwords[wordpos - 1][chuck_at]);
>   	}
>       } while (tok != LEXERR && tok != ENDINPUT &&
>   	     (tok != SEPER || (lexflags && tt0 == NULLTOK)));
> @@ -1464,7 +1476,9 @@ get_comp_string(void)
>   	t0 = STRING;
>       } else if (t0 == STRING || t0 == TYPESET) {
>   	/* We found a simple string. */
> -	s = ztrdup(clwords[clwpos]);
> +	s = clwords[clwpos];
> +	DPUTS(!s, "Completion word has disappeared!");
> +	s = ztrdup(s ? s : "");
>       } else if (t0 == ENVSTRING) {
>   	char sav;
>   	/* The cursor was inside a parameter assignment. */

Re: [PATCH] Fix #87 - Segfault fault when autocompleting after ">" in, "!> ."

[Peter Stephenson]

On Fri, 9 Dec 2016 17:42:57 +0100
Raphaël Jakse <raphael.jakse@imag.fr> wrote:
> Thanks for taking a look at it. Both of your patches seem more accurate 
> than mine.
> It would be nice if one of them could make it into zsh.

This went in as 32daf2a0 --- it's in the test releases.

> For your information, looking at this bug was / is part of my research. 
> We try to combine monitoring techniques and interactive debugging. If 
> you are interested, feel free to ask me questions and look at our our 
> tool here: http://check-exec.forge.imag.fr/

That's certainly an interesting idea.

pws