bug with completion in quotes

bug with completion in quotes

[Oliver Kiddle]

Compiled with --enable-zsh-debug, I get bugs listed when completing
inside $' style quotes whenever there's stuff after the cursor:

<tab> appears at cursor position

% : $'<tab>x
utils.c:5893: BUG: unterminated $' substitution
% : $'<tab>x'
zle_tricky.c:666: BUG: 0 <= wb <= zlemetacs <= we is not true!

The line containing the first message was added in workers/22026 from
about 2006.

Oliver

Re: bug with completion in quotes

[Bart Schaefer]

On Oct 12, 12:37am, Oliver Kiddle wrote:
} Subject: bug with completion in quotes
}
} % : $'<tab>x
} utils.c:5893: BUG: unterminated $' substitution

Hrm.  Comments say:

 * ... If both GETKEY_DOLLAR_QUOTE
 * and GETKEY_UPDATE_OFFSET are present in "how", the string is not
 * expected to be terminated (this is used in completion to parse
 * a partial $'...'-quoted string) and the length passed back is
 * that of the converted string.

And later, right at the point of that DPUTS:

     * When called from completion, where we use GETKEY_UPDATE_OFFSET to
     * update the index into the metafied editor line, we don't necessarily
     * have the end of a $'...' quotation, else we should do.

However, as far as I can tell nothing ever passes GETKEY_UPDATE_OFFSET
in any circumstances, completion or otherwise.

Adding that flag to the call near zle_tricky.c:1733 silences this warning,
but that can't be correct as GETKEY_UPDATE_OFFSET expects to deref the
"misc" parameter which is NULL in that call.

There are also places in utils.c where comments appear to mean that
GETKEY_UPDATE_OFFSET tests need to be added, but there are none.  I
think we've uncovered someone's unfinshed work.

} % : $'<tab>x'
} zle_tricky.c:666: BUG: 0 <= wb <= zlemetacs <= we is not true!

Examining in the debugger we have

(gdb) p wb
$2 = 2
(gdb) p zlemetacs
$3 = 1
(gdb) p we
$4 = 3

I think this is an off-by-one error happening because dollar-tick is
two characters being treated as a solitary quote, but I don't know
where to fix it.

Re: bug with completion in quotes

[Peter Stephenson]

On Sun, 12 Oct 2014 00:37:09 +0200
Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
> Compiled with --enable-zsh-debug, I get bugs listed when completing
> inside $' style quotes whenever there's stuff after the cursor:
> 
> <tab> appears at cursor position
> 
> % : $'<tab>x
> utils.c:5893: BUG: unterminated $' substitution
> % : $'<tab>x'
> zle_tricky.c:666: BUG: 0 <= wb <= zlemetacs <= we is not true!
> 
> The line containing the first message was added in workers/22026 from
> about 2006.

This code is particularly horrible.  2006 is about the last time I
remember looking at it and giving up.  However, most of the really nasty
problems we're with nested quotes e.g. where a quoted argument to a
shell or scripting language itself contained quotes --- which I'm not
sure we whould be trying to do internally anyway.  One set of quotes is
just about tractable.

The chief suspect is set_comp_sep() --- it's very brittle because it's
interacting with all sorts of different parts of the shell in ad hoc
ways.

pws

Re: bug with completion in quotes

[Peter Stephenson]

On Sun, 12 Oct 2014 18:35:47 +0100
Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:

> On Sun, 12 Oct 2014 00:37:09 +0200
> Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
> > Compiled with --enable-zsh-debug, I get bugs listed when completing
> > inside $' style quotes whenever there's stuff after the cursor:
> > 
> > <tab> appears at cursor position
> > 
> > % : $'<tab>x
> > utils.c:5893: BUG: unterminated $' substitution
> > % : $'<tab>x'
> > zle_tricky.c:666: BUG: 0 <= wb <= zlemetacs <= we is not true!
> > 
> > The line containing the first message was added in workers/22026 from
> > about 2006.
> 
> The chief suspect is set_comp_sep()

Although that function is a permanent black stain on my soul, before I
send others on wild goose chases I don't think it can be the culprit
here since I think it's *only* used for recursive quotes.  I'm now
wondering about get_comp_string() from line 1676 in zle_tricky.c,
including the grumpy comment I would guess is from me, since they usually
are, around line 1686.  However, I haven't gone into the nitty gritty.

pws

Re: bug with completion in quotes

[Bart Schaefer]

On Oct 12,  6:51pm, Peter Stephenson wrote:
}
} Although that function is a permanent black stain on my soul, before I
} send others on wild goose chases I don't think it can be the culprit
} here since I think it's *only* used for recursive quotes.  I'm now
} wondering about get_comp_string() from line 1676 in zle_tricky.c,
} including the grumpy comment I would guess is from me, since they usually
} are, around line 1686.  However, I haven't gone into the nitty gritty.

It's definitely get_comp_string(), probably via getkeystring() in utils.c

See my earlier email.

Re: bug with completion in quotes

[Peter Stephenson]

On Sun, 12 Oct 2014 11:29:43 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:

> On Oct 12,  6:51pm, Peter Stephenson wrote:
> }
> } Although that function is a permanent black stain on my soul, before I
> } send others on wild goose chases I don't think it can be the culprit
> } here since I think it's *only* used for recursive quotes.  I'm now
> } wondering about get_comp_string() from line 1676 in zle_tricky.c,
> } including the grumpy comment I would guess is from me, since they usually
> } are, around line 1686.  However, I haven't gone into the nitty gritty.
> 
> It's definitely get_comp_string(), probably via getkeystring() in utils.c
> 
> See my earlier email.

I think this is zsh-workers/23809, commit
e0a3e74b15fd39b21ef1770e67e2f005321b5fb9, going off at the wrong time.
It was supposed to apply to expanding complete $'...' expressions, but
in this case it's being applied to an uncompleted completion.

It looks like the intended purpose is basically OK since completing
after a $'...' works OK, at least in simple cases.  So it may be we
simply need to skip the code added by that change --- simply setting
skipchars to 2 instead looks like it's probably the right thing to do
--- if we're in the middle of it.

I think this is roughly what the "i + (pe - p) < zlemetacs" test is
trying to do at line 1721.  Evidently it's not working.  Presumably one
of the many ad hoc offsets hasn't been taken account of.

pws

Re: bug with completion in quotes

[Peter Stephenson]

On Sun, 12 Oct 2014 20:44:29 +0100
Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> On Sun, 12 Oct 2014 11:29:43 -0700
> Bart Schaefer <schaefer@brasslantern.com> wrote:
> 
> > On Oct 12,  6:51pm, Peter Stephenson wrote:
> > }
> > } Although that function is a permanent black stain on my soul, before I
> > } send others on wild goose chases I don't think it can be the culprit
> > } here since I think it's *only* used for recursive quotes.  I'm now
> > } wondering about get_comp_string() from line 1676 in zle_tricky.c,
> > } including the grumpy comment I would guess is from me, since they usually
> > } are, around line 1686.  However, I haven't gone into the nitty gritty.
> > 
> > It's definitely get_comp_string(), probably via getkeystring() in utils.c
> > 
> > See my earlier email.
> 
> I think this is zsh-workers/23809, commit
> e0a3e74b15fd39b21ef1770e67e2f005321b5fb9, going off at the wrong time.
> It was supposed to apply to expanding complete $'...' expressions, but
> in this case it's being applied to an uncompleted completion.
> 
> It looks like the intended purpose is basically OK since completing
> after a $'...' works OK, at least in simple cases.  So it may be we
> simply need to skip the code added by that change --- simply setting
> skipchars to 2 instead looks like it's probably the right thing to do
> --- if we're in the middle of it.

Sigh.  Nope.  I think it *is* doing the skipchars == 2 branch but that's
not the right thing to do for some reason.  I don't know what is.

pws

Re: bug with completion in quotes

[Bart Schaefer]

On Oct 12,  9:15pm, Peter Stephenson wrote:
}
} > I think this is zsh-workers/23809, commit
} > e0a3e74b15fd39b21ef1770e67e2f005321b5fb9, going off at the wrong time.
} > It was supposed to apply to expanding complete $'...' expressions, but
} > in this case it's being applied to an uncompleted completion.
} > 
} > It looks like the intended purpose is basically OK since completing
} > after a $'...' works OK, at least in simple cases.  So it may be we
} > simply need to skip the code added by that change --- simply setting
} > skipchars to 2 instead looks like it's probably the right thing to do
} > --- if we're in the middle of it.
} 
} Sigh.  Nope.  I think it *is* doing the skipchars == 2 branch but that's
} not the right thing to do for some reason.  I don't know what is.

So below I'm talking about the incomplete $'x expression.  The case of
the $'x' expression with the wb <= zlemetacs may be different.

I've GDB'd through it and it's definitely taking the branch that begins
(zle_tricky.c) with the comment

		/*
		 * Try and substitute the $'...' expression.
		 */

If it took the skipchars = 2 branch it'd never hit the DPUTS that got
this whole thread started.

Comment in utils.c:

 * *misc is used for various purposes:
 * - If GETKEY_UPDATE_OFFSET is set, it is set on input to some
 *   mystical completion offset and is updated to a new offset based
 *   on the converted characters.  All Hail the Completion System
 *   [makes the mystic completion system runic sign in the air].

But nothing ever passes GETKEY_UPDATE_OFFSET.  That branch of zle_tricky
looks like the only reasonable place to use that, but I don't know what
mystical offset is meant to be passed in *misc and returned updated.
Maybe &skipchars ?  But what value should it start with?

Note that both of these bugs only occur when the completion is at the
START of the $'x' expression (before the "x").  When after the x or
after the whole expression, everything works OK.

So maybe the problem is that getkeystring() is reading too far ahead
and returning the length of the entire $'...' (or the length so far,
in the case of the unclosed quote) and really what we want is only
the length up to the cursor position?

Re: bug with completion in quotes

[Peter Stephenson]

See if this helps with some of the $'...' problems.

Note that when the cursor is inside $'...' we deliberately don't try to
expand the $'...'.  That's not a change.  Possibly we could do so, but
we'd need to be careful about where the cursor ended up.

diff --git a/Src/Zle/zle_tricky.c b/Src/Zle/zle_tricky.c
index 499c4ae..5fa625a 100644
--- a/Src/Zle/zle_tricky.c
+++ b/Src/Zle/zle_tricky.c
@@ -662,8 +662,9 @@ docomplete(int lst)
      * NOTE: get_comp_string() calls pushheap(), but not popheap(). */
     noerrs = 1;
     s = get_comp_string();
-    DPUTS(wb < 0 || zlemetacs < wb || zlemetacs > we,
-	  "BUG: 0 <= wb <= zlemetacs <= we is not true!");
+    DPUTS3(wb < 0 || zlemetacs < wb || zlemetacs > we,
+	  "BUG: 0 <= wb (%d) <= zlemetacs (%d) <= we (%d) is not true!",
+	   wb, zlemetacs, we);
     noerrs = ne;
     /* For vi mode, reset the start-of-insertion pointer to the beginning *
      * of the word being completed, if it is currently later.  Vi itself  *
@@ -1720,9 +1721,11 @@ get_comp_string(void)
 	    for (pe = p + 2; *pe && *pe != Snull && i + (pe - p) < zlemetacs;
 		 pe++)
 		;
-	    if (!*pe) {
+	    if (*pe != Snull) {
 		/* no terminating Snull, can't substitute */
 		skipchars = 2;
+		if (*pe)
+		    j = 1;
 	    } else {
 		/*
 		 * Try and substitute the $'...' expression.
@@ -1795,6 +1798,10 @@ get_comp_string(void)
 		     * first clue how the completion system actually works.
 		     */
 		    skipchars = 2;
+		    /*
+		     * Also pretend we're in single quotes.
+		     */
+		    j = 1;
 		}
 	    }
 	}
@@ -1817,7 +1824,7 @@ get_comp_string(void)
 		    ocs = zlemetacs;
 		    zlemetacs = i;
 		    foredel(skipchars, CUT_RAW);
-		    if ((zlemetacs = ocs) > (i -= skipchars))
+		    if ((zlemetacs = ocs) > i--)
 			zlemetacs -= skipchars;
 		    we -= skipchars;
 		}


pws

Re: bug with completion in quotes

[Oliver Kiddle]

Peter Stephenson wrote:
> 
> Note that when the cursor is inside $'...' we deliberately don't try to
> expand the $'...'.  That's not a change.  Possibly we could do so, but
> we'd need to be careful about where the cursor ended up.

By the way, I've been using this patch and it seems to work fine.

I don't think we'd want to expand the $'...': I think it's fine as it
is.

Thanks

Oliver

Re: bug with completion in quotes

[Peter Stephenson]

On Wed, 22 Oct 2014 16:05:01 +0200
Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
> Peter Stephenson wrote:
> > 
> > Note that when the cursor is inside $'...' we deliberately don't try to
> > expand the $'...'.  That's not a change.  Possibly we could do so, but
> > we'd need to be careful about where the cursor ended up.
> 
> By the way, I've been using this patch and it seems to work fine.
> 
> I don't think we'd want to expand the $'...': I think it's fine as it
> is.

Good --- I've been a little tied up (guess which revision control system we've
suddenly decided to move to?) but I'm worried that

-		    if ((zlemetacs = ocs) > (i -= skipchars))
+		    if ((zlemetacs = ocs) > i--)

should actually be

-		    if ((zlemetacs = ocs) > (i -= skipchars))
+		    if ((zlemetacs = ocs) > --i)

In other words, I'm sure that only decrementing once is correct ---
we're deleting characters ahead of the point we're looking at, so the
decrement is simply to cancel out the effect of the ++i in the loop
iterator --- but the --i is more consistent with what was there before.
I'll try and puzzle out the exact meaning of the indices before I commit
this.

pws

Re: bug with completion in quotes

[Peter Stephenson]

On Wed, 22 Oct 2014 15:28:02 +0100
Peter Stephenson <p.stephenson@samsung.com> wrote:
> I'm worried that
> 
> -		    if ((zlemetacs = ocs) > (i -= skipchars))
> +		    if ((zlemetacs = ocs) > i--)
> 
> should actually be
> 
> -		    if ((zlemetacs = ocs) > (i -= skipchars))
> +		    if ((zlemetacs = ocs) > --i)
> 
> In other words, I'm sure that only decrementing once is correct ---
> we're deleting characters ahead of the point we're looking at, so the
> decrement is simply to cancel out the effect of the ++i in the loop
> iterator --- but the --i is more consistent with what was there before.
> I'll try and puzzle out the exact meaning of the indices before I commit
> this.

I can't find a case where this seems to make a difference in practice.
If there's an off-by-one error remaining it's a very subtle one.

Given that, I'll commit this with the second change since it looks more
like what's there but with the definite bug removed.

pws