Completion in empty double-quotes generates error

Completion in empty double-quotes generates error

[Bart Schaefer]

About six years ago, I wrote:
} If I invoke quote-and-complete-word via execute-named-command on an
} empty buffer, I get this:
} 
} schaefer<525> 11: ../../../zsh-4.0/Src/Zle/zle_tricky.c:661: BUG: 0 <= wb <=
} zlemetacs <= we is not true!
} 
} The same error does not occur when invoking it via a direct binding.

Well, now it DOES occur when invoking via direct binding.  The error
message has gotten a bit more informative.

---------------
torch% autoload quote-and-complete-word 
torch% zle -N quote-and-complete-word
torch% bindkey '\t' quote-and-complete-word
torch% ls <TAB>
torch% ls 10: ../../../zsh-5.0/Src/Zle/zle_tricky.c:658: BUG: 0 <= wb (3) <= zlemetacs (2) <= we (3) is not true!
""     ls ""
(listing here)
---------------

The "10:" would seem to imply that the error comes from the first call to
"zle complete-word" in quote-and-complete-word, but if I simplify the
function down to just that I don't get the error.

Further experimentation reveals that starting from a clean "compinit" I can
generate the error by completing inside empty double quotes"

torch% ls ""<backward-char><complete-word>
torch% ls " ../../../zsh-5.0/Src/Zle/zle_tricky.c:658: BUG: 0 <= wb (3) <= zlemetacs (2) <= we (3) is not true!
       ls ""

If there is even one other character inside the quotes before completing, no
error is reported.

Re: Completion in empty double-quotes generates error

[Daniel Shahaf]

Bart Schaefer wrote on Wed, Mar 30, 2016 at 11:06:52 -0700:
> torch% ls ""<backward-char><complete-word>
> torch% ls " ../../../zsh-5.0/Src/Zle/zle_tricky.c:658: BUG: 0 <= wb (3) <= zlemetacs (2) <= we (3) is not true!
>        ls ""
> 
> If there is even one other character inside the quotes before completing, no
> error is reported.

I ran into this six months ago in 37012; there the reproducer was:

% : 2>1<backward-char><backward-char><<backward-char><complete-word>
zle_tricky.c:658: BUG: 0 <= wb (4) <= zlemetacs (2) <= we (5) is not true!

Re: Completion in empty double-quotes generates error

[Bart Schaefer]

On Apr 1,  5:36am, Daniel Shahaf wrote:
} Subject: Re: Completion in empty double-quotes generates error
}
} Bart Schaefer wrote on Wed, Mar 30, 2016 at 11:06:52 -0700:
} > torch% ls ""<backward-char><complete-word>
} > torch% ls " ../../../zsh-5.0/Src/Zle/zle_tricky.c:658: BUG: 0 <= wb (3) <= zlemetacs (2) <= we (3) is not true!
} >        ls ""
} > 
} > If there is even one other character inside the quotes before completing, no
} > error is reported.
} 
} I ran into this six months ago in 37012

I'm not confident this is the same bug, even though the DPUTS() which
trips is the same one.

} there the reproducer was:
} 
} % : 2>1<backward-char><backward-char><<backward-char><complete-word>
} zle_tricky.c:658: BUG: 0 <= wb (4) <= zlemetacs (2) <= we (5) is not true!

I can't reproduce this any longer, at least not with plain compinit and
no zstyles.

In the empty-parens case, "wb" is set to the position of the first
double-quote and "we" is set to just after the second double-quote.
This happens in gotword() called from the lexer from get_comp_string()
at line 1202, via exalias().

Still in get_comp_string(), we then fall into the for-loop commented
/* While building the quoted form, we also clean up the command line. */

This loop deletes the double quotes with foredel() at line 1851, and
decrements we, but never adjusts wb even though the double-quote
at which it points is being deleted.  There's a similar thing with
backdel() in another branch, which presumably could be the 37021 bug,
but I don't know how to trigger it any more.

However, if I try what I think is obvious and set wb = zlemetacs then
the space before the double-quotes gets deleted and never restored.
So either it's not really a problem for zlemetacs to be less than wb,
or the following is correct (and I suppose its possible we need the
same test for wb > we, but I can't make that happen, so I'm leaving
it for now).

diff --git a/Src/Zle/zle_tricky.c b/Src/Zle/zle_tricky.c
index a89b2a3..b1709c1 100644
--- a/Src/Zle/zle_tricky.c
+++ b/Src/Zle/zle_tricky.c
@@ -1849,8 +1849,12 @@ get_comp_string(void)
 		    ocs = zlemetacs;
 		    zlemetacs = i;
 		    foredel(skipchars, CUT_RAW);
-		    if ((zlemetacs = ocs) > --i)
+		    if ((zlemetacs = ocs) > --i) {
 			zlemetacs -= skipchars;
+			/* do not skip past the beginning of the word */
+			if (wb > zlemetacs)
+			    zlemetacs = wb;
+		    }
 		    we -= skipchars;
 		}
 	    } else {
@@ -1861,6 +1865,9 @@ get_comp_string(void)
 		    zlemetacs = we - skipchars;
 		else
 		    zlemetacs = ocs;
+		/* do not skip past the beginning of the word */
+		if (wb > zlemetacs)
+		    zlemetacs = wb;
 		we -= skipchars;
 	    }
 	    /* we need to get rid of all the quotation bits... */

Re: Completion in empty double-quotes generates error

[Daniel Shahaf]

Bart Schaefer wrote on Fri, Apr 01, 2016 at 18:18:24 -0700:
> On Apr 1,  5:36am, Daniel Shahaf wrote:
> } Subject: Re: Completion in empty double-quotes generates error
> }
> } Bart Schaefer wrote on Wed, Mar 30, 2016 at 11:06:52 -0700:
> } > torch% ls ""<backward-char><complete-word>
> } > torch% ls " ../../../zsh-5.0/Src/Zle/zle_tricky.c:658: BUG: 0 <= wb (3) <= zlemetacs (2) <= we (3) is not true!
> } >        ls ""
> } > 
> } > If there is even one other character inside the quotes before completing, no
> } > error is reported.
> } 
> } I ran into this six months ago in 37012
> 
> I'm not confident this is the same bug, even though the DPUTS() which
> trips is the same one.
> 

Yes, that's all I meant to say.

> } there the reproducer was:
> } 
> } % : 2>1<backward-char><backward-char><<backward-char><complete-word>
> } zle_tricky.c:658: BUG: 0 <= wb (4) <= zlemetacs (2) <= we (5) is not true!
> 
> I can't reproduce this any longer, at least not with plain compinit and
> no zstyles.
> 

I can reproduce it in git b81b275ab30b06867a6ab13f774e90f0955dad88 under
'zsh -f' + 'autoload compinit; compinit'.  (I have a local patch that makes
'zsh -f' default to the emacs keymap unconditionally.)

I could try with your patch later and report the results, but I don't
understand how to change it for the ": 2>1" variant?

Cheers,

Daniel

Re: Completion in empty double-quotes generates error

[Bart Schaefer]

On Apr 2,  3:29am, Daniel Shahaf wrote:
}
} I could try with your patch later and report the results, but I don't
} understand how to change it for the ": 2>1" variant?

Never mind; it's not the same bug -- I just re-read 37012 and realized
I was testing incorrectly; I can still reproduce (even with 38229 in
place).

In the redirection 2>1, gotword() has concluded that the word to be
completed is "1" rather than (a prefix of) "2".  Rather than zlemetacs
moving backward to before wb, wb has moved forward to after zlemetacs.

This is probably because the lexer wants to treat "2>" as a single
token, but again the seemingly obvious thing (keep wb <= zlemetacs)
has bad side effects ("2>" is deleted, leaving "1" on the line).

Re: Completion in empty double-quotes generates error

[Bart Schaefer]

On Apr 1, 11:20pm, Bart Schaefer wrote:
} Subject: Re: Completion in empty double-quotes generates error
}
} In the redirection 2>1, gotword() has concluded that the word to be
} completed is "1" rather than (a prefix of) "2".
} 
} This is probably because the lexer wants to treat "2>" as a single
} token

Indeed, zshlex() consumes "2>" as a single OUTANG token with tokfd = 2
handled as metadata.

This means that in "2>1" the only complete-able "word" that the lexer
recognizes on the line is the "1" following the redirection.

What would one expect to be completed with the cursor on the "2"?  The
-redirect- special context is for what comes *after* the operator.  I
can think of only two choices:

1.  Complete as if there were an implicit space to the right of the
    cursor.  This makes some sense because "2>" is treated as the same
    single token as ">" all alone.

2.  Treat it as an error and simply fail.

Patch below does this, but it ought to also insert a space to the right
of any inserted word, which I haven't done yet.  If someone else knows
how to do that, please jump in.

As a bonus, this patch fixes a case where wordpos was incorrectly left
at 1 near a redirection, causing command completion to incorrectly be
invoked.

diff --git a/Src/Zle/zle_tricky.c b/Src/Zle/zle_tricky.c
index a89b2a3..553721c 100644
--- a/Src/Zle/zle_tricky.c
+++ b/Src/Zle/zle_tricky.c
@@ -1238,6 +1238,15 @@ get_comp_string(void)
 	    /* Record if we haven't had the command word yet */
 	    if (wordpos == redirpos)
 		redirpos++;
+	    if (zlemetacs < (zlemetall - inbufct)) {
+	      /* Cursor is in the middle of a redirection, treat as a word */
+	      DPUTS3(addedx,
+		     "added x in redirect: wb = %d, we = %d, zlemetacs = %d",
+		     wb, we, zlemetacs);
+	      wb = zlemetacs;
+	      we = zlemetall - inbufct;
+	      wordpos++;
+	    }
         }
 	if (tok == DINPAR)
 	    tokstr = NULL;

Re: Completion in empty double-quotes generates error

[Bart Schaefer]

On Apr 2, 11:18am, Bart Schaefer wrote:
} Subject: Re: Completion in empty double-quotes generates error
}
} What would one expect to be completed with the cursor on the "2"?  The
} -redirect- special context is for what comes *after* the operator.  I
} can think of only two choices:
} 
} 1.  Complete as if there were an implicit space to the right of the
}     cursor.  This makes some sense because "2>" is treated as the same
}     single token as ">" all alone.
} 
} 2.  Treat it as an error and simply fail.
} 
} Patch below does this

More specifically, it attempted to do (1).  However, this situation is
really nasty -- get_comp_string() repeatedly calls the lexer looking for
the word containing the cursor, but because the lexer treats redirections
as positionally insignficant, there literally is no word containing the
cursor, so get_comp_string() keeps looking and finds the NEXT word (or
the end of the input), which places both wb and we beyond zlemetacs.

It gets worse still with "{myfd}>", and worse again when completing in
the whitespace between a word and a redirection (because in that last
situation it's correct for the redirection token to be ignored).

I'm still not entirely sure that the below is correct, i.e. that the
right things would happen if the "Should" comments were replaced with
the appropriate actual code.  But this at least handles more cases than
the previous situation.

The change in gotword() is necessary but of a little concern.  AFAICT
we only care about wb when examining the word under the cursor, so it
shouldn't hurt to skip updating it when doing so would violate the
wb <= zlemetacs <= we constraint, but there may be some corner case I
haven't tried that depends on the old behavior.


diff --git a/Src/Zle/zle_tricky.c b/Src/Zle/zle_tricky.c
index b1709c1..1d4e1d2 100644
--- a/Src/Zle/zle_tricky.c
+++ b/Src/Zle/zle_tricky.c
@@ -1161,6 +1161,7 @@ get_comp_string(void)
     inpush(dupstrspace(linptr), 0, NULL);
     strinbeg(0);
     wordpos = cp = rd = ins = oins = linarr = parct = ia = redirpos = 0;
+    we = wb = zlemetacs;
     tt0 = NULLTOK;
 
     /* This loop is possibly the wrong way to do this.  It goes through *
@@ -1238,6 +1239,20 @@ get_comp_string(void)
 	    /* Record if we haven't had the command word yet */
 	    if (wordpos == redirpos)
 		redirpos++;
+	    if (zlemetacs < (zlemetall - inbufct) &&
+		zlemetacs >= wordbeg && wb == we) {
+		/* Cursor is in the middle of a redirection, treat as a word */
+		we = zlemetall - (inbufct + addedx);
+		if (addedx && we > wb) {
+		    /* Assume we are in {param}> form, wb points at "{" */
+		    wb++;
+		    /* Should complete parameter names here */
+		} else {
+		    /* In "2>" form, zlemetacs points at "2" */
+		    wb = zlemetacs;
+		    /* Should insert a space under cursor here */
+		}
+	    }
         }
 	if (tok == DINPAR)
 	    tokstr = NULL;
diff --git a/Src/lex.c b/Src/lex.c
index d4132fe..25b372a 100644
--- a/Src/lex.c
+++ b/Src/lex.c
@@ -1789,9 +1789,13 @@ parse_subst_string(char *s)
 static void
 gotword(void)
 {
-    we = zlemetall + 1 - inbufct + (addedx == 2 ? 1 : 0);
-    if (zlemetacs <= we) {
-	wb = zlemetall - wordbeg + addedx;
+    int nwe = zlemetall + 1 - inbufct + (addedx == 2 ? 1 : 0);
+    if (zlemetacs <= nwe) {
+	int nwb = zlemetall - wordbeg + addedx;
+	if (zlemetacs >= nwb) {
+	    wb = nwb;
+	    we = nwe;
+	}
 	lexflags = 0;
     }
 }

Re: Completion in empty double-quotes generates error

[Daniel Shahaf]

Bart Schaefer wrote on Wed, Apr 06, 2016 at 12:10:53 -0700:
> On Apr 2, 11:18am, Bart Schaefer wrote:
> } Subject: Re: Completion in empty double-quotes generates error
> }
> } What would one expect to be completed with the cursor on the "2"?  The
> } -redirect- special context is for what comes *after* the operator.  I
> } can think of only two choices:
> } 
> } 1.  Complete as if there were an implicit space to the right of the
> }     cursor.  This makes some sense because "2>" is treated as the same
> }     single token as ">" all alone.
> } 
> } 2.  Treat it as an error and simply fail.
> } 
> } Patch below does this
> 
> More specifically, it attempted to do (1).  However, this situation is
> really nasty -- get_comp_string() repeatedly calls the lexer looking for
> the word containing the cursor, but because the lexer treats redirections
> as positionally insignficant, there literally is no word containing the
> cursor, so get_comp_string() keeps looking and finds the NEXT word (or
> the end of the input), which places both wb and we beyond zlemetacs.
> 
> It gets worse still with "{myfd}>", and worse again when completing in
> the whitespace between a word and a redirection (because in that last
> situation it's correct for the redirection token to be ignored).
> 
> I'm still not entirely sure that the below is correct, i.e. that the
> right things would happen if the "Should" comments were replaced with
> the appropriate actual code.  But this at least handles more cases than
> the previous situation.

'tmux new <TAB>' segfaults:

    $ zsh -f
    % echo $ZSH_PATCHLEVEL
    zsh-5.2-dev-1-130-g98f670c
    % autoload compinit
    % compinit
    % tmux new <TAB>5: compcore.c:1657: expecting 'x' at offset -7 of "x"
    
    Program received signal SIGSEGV, Segmentation fault.
    0x00007ffff5a030da in check_param (s=0x7ffff7f90188 "x", set=0, test=1) at compcore.c:1108
    1108		if (*p == String || *p == Qstring) {
    (gdb) p p
    $1 = 0x7ffff7f8ffff <error: Cannot access memory at address 0x7ffff7f8ffff>
    (gdb) bt
    #0  0x00007ffff5a030da in check_param (s=0x7ffff7f90188 "x", set=0, test=1) at compcore.c:1108
    #1  0x00007ffff5a0451f in set_comp_sep () at compcore.c:1677
    #2  0x00007ffff59fece5 in bin_compset (name=0x7ffff7f900d0 "compset", argv=0x7ffff7f900e0, ops=0x7ffffffe4160, func=0) at complete.c:1031

It bisects to 38248 (which I'm replying to).

I didn't have any tmux sessions/processes running while I was
reproducing the bug.

Cheers,

Daniel

P.S. With latest master, «sh -c <TAB>» also segfaults; on the other
hand, after «compdef _cmdstring f», «f <TAB>» doesn't segfault.

> The change in gotword() is necessary but of a little concern.  AFAICT
> we only care about wb when examining the word under the cursor, so it
> shouldn't hurt to skip updating it when doing so would violate the
> wb <= zlemetacs <= we constraint, but there may be some corner case I
> haven't tried that depends on the old behavior.
> 
> 
> diff --git a/Src/Zle/zle_tricky.c b/Src/Zle/zle_tricky.c
> index b1709c1..1d4e1d2 100644
> --- a/Src/Zle/zle_tricky.c
> +++ b/Src/Zle/zle_tricky.c
> @@ -1161,6 +1161,7 @@ get_comp_string(void)
>      inpush(dupstrspace(linptr), 0, NULL);
>      strinbeg(0);
>      wordpos = cp = rd = ins = oins = linarr = parct = ia = redirpos = 0;
> +    we = wb = zlemetacs;
>      tt0 = NULLTOK;
>  
>      /* This loop is possibly the wrong way to do this.  It goes through *
> @@ -1238,6 +1239,20 @@ get_comp_string(void)
>  	    /* Record if we haven't had the command word yet */
>  	    if (wordpos == redirpos)
>  		redirpos++;
> +	    if (zlemetacs < (zlemetall - inbufct) &&
> +		zlemetacs >= wordbeg && wb == we) {
> +		/* Cursor is in the middle of a redirection, treat as a word */
> +		we = zlemetall - (inbufct + addedx);
> +		if (addedx && we > wb) {
> +		    /* Assume we are in {param}> form, wb points at "{" */
> +		    wb++;
> +		    /* Should complete parameter names here */
> +		} else {
> +		    /* In "2>" form, zlemetacs points at "2" */
> +		    wb = zlemetacs;
> +		    /* Should insert a space under cursor here */
> +		}
> +	    }
>          }
>  	if (tok == DINPAR)
>  	    tokstr = NULL;
> diff --git a/Src/lex.c b/Src/lex.c
> index d4132fe..25b372a 100644
> --- a/Src/lex.c
> +++ b/Src/lex.c
> @@ -1789,9 +1789,13 @@ parse_subst_string(char *s)
>  static void
>  gotword(void)
>  {
> -    we = zlemetall + 1 - inbufct + (addedx == 2 ? 1 : 0);
> -    if (zlemetacs <= we) {
> -	wb = zlemetall - wordbeg + addedx;
> +    int nwe = zlemetall + 1 - inbufct + (addedx == 2 ? 1 : 0);
> +    if (zlemetacs <= nwe) {
> +	int nwb = zlemetall - wordbeg + addedx;
> +	if (zlemetacs >= nwb) {
> +	    wb = nwb;
> +	    we = nwe;
> +	}
>  	lexflags = 0;
>      }
>  }
> 

Re: Completion in empty double-quotes generates error

[Bart Schaefer]

On May 10,  6:40pm, Daniel Shahaf wrote:
} Subject: Re: Completion in empty double-quotes generates error
}
} 'tmux new <TAB>' segfaults:
} 
}     $ zsh -f
}     % echo $ZSH_PATCHLEVEL
}     zsh-5.2-dev-1-130-g98f670c

That's odd:

burner% echo $ZSH_PATCHLEVEL
zsh-5.2-403-gb359b80
burner% echo $ZSH_VERSION
5.2-dev-1

I never get the "-dev-1" part in PATCHLEVEL.

}     % autoload compinit
}     % compinit
}     % tmux new <TAB>5: compcore.c:1657: expecting 'x' at offset -7 of "x"
}     
}     Program received signal SIGSEGV, Segmentation fault.

Well, that's got to be because of this:

} > The change in gotword() is necessary but of a little concern.  AFAICT
} > we only care about wb when examining the word under the cursor, so it
} > shouldn't hurt to skip updating it when doing so would violate the
} > wb <= zlemetacs <= we constraint, but there may be some corner case I
} > haven't tried that depends on the old behavior.

Apparently here is a case that depends on wb / we always being updated,
or at least being initialized in some way that has been missed.

I won't have a chance to look at this for at least another 3 weeks, so if
anyone else cares to try deciphering the interaction of the lexer with
compcore, it's all yours.

Re: Completion in empty double-quotes generates error

[Bart Schaefer]

On May 10,  1:13pm, Bart Schaefer wrote:
}
} I won't have a chance to look at this for at least another 3 weeks, so

Having said that, I took a guess and may have been lucky:

diff --git a/Src/lex.c b/Src/lex.c
index 25b372a..e36a01e 100644
--- a/Src/lex.c
+++ b/Src/lex.c
@@ -1795,6 +1795,10 @@ gotword(void)
 	if (zlemetacs >= nwb) {
 	    wb = nwb;
 	    we = nwe;
+	} else {
+	    wb = zlemetacs + addedx;
+	    if (we < wb)
+		we = wb;
 	}
 	lexflags = 0;
     }

This stops the "sh -c <TAB>" warning/coredump for me; I wasn't able to
reproduct the "tmux new <TAB>" error.  The assignment to "we" seems to
be entirely unnecessary, but it looks wrong to omit it.

Re: Completion in empty double-quotes generates error

[Mikael Magnusson]

On Tue, May 10, 2016 at 10:13 PM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> On May 10,  6:40pm, Daniel Shahaf wrote:
> } Subject: Re: Completion in empty double-quotes generates error
> }
> } 'tmux new <TAB>' segfaults:
> }
> }     $ zsh -f
> }     % echo $ZSH_PATCHLEVEL
> }     zsh-5.2-dev-1-130-g98f670c
>
> That's odd:
>
> burner% echo $ZSH_PATCHLEVEL
> zsh-5.2-403-gb359b80
> burner% echo $ZSH_VERSION
> 5.2-dev-1
>
> I never get the "-dev-1" part in PATCHLEVEL.

Try running git fetch --tags. If you just normally do "git pull" it
will only fetch the tag if it existed at the time of the pull iirc.

-- 
Mikael Magnusson

Re: Completion in empty double-quotes generates error

[Daniel Shahaf]

Bart Schaefer wrote on Tue, May 10, 2016 at 14:00:02 -0700:
> This stops the "sh -c <TAB>" warning/coredump for me; I wasn't able to
> reproduct the "tmux new <TAB>" error.

It fixes both of them here.