Bug in ZSH's vi emulation

Bug in ZSH's vi emulation

[Ghjuvan' Lacambre]

Hi.

There is a bug in zsh's vi emulation. Here are the steps to reproduce:

zsh -f
bindkey -v
<ESC>
d..

'd.' isn't a valid action as '.' is not a valid motion, yet 'd.' is
recorded as the last action called. When using '.', 'd.' will
recursively call itself, locking the command line and eating from 50 to
100% of available cpu resources. Other actions that depend on a motion
(such as 'c' or 'y') also suffer from this problem.

-- 
Ghjuvan' Lacambre

Re: Bug in ZSH's vi emulation

[Bart Schaefer]

On Oct 5, 10:09am, Ghjuvan' Lacambre wrote:
}
} 'd.' isn't a valid action as '.' is not a valid motion, yet 'd.' is
} recorded as the last action called. When using '.', 'd.' will
} recursively call itself

Confirmed.

Seems to be a problem with calling startvichange(1) before calling
getvirange(), so the input is captured as a change event whether it
succeeds or fails.  Unfortunately, I'm not familiar enough with the
vi emulation code to see how to back out of startvichange().

Re: Bug in ZSH's vi emulation

[Daniel Shahaf]

Bart Schaefer wrote on Wed, Oct 05, 2016 at 10:19:38 -0700:
> On Oct 5, 10:09am, Ghjuvan' Lacambre wrote:
> }
> } 'd.' isn't a valid action as '.' is not a valid motion, yet 'd.' is
> } recorded as the last action called. When using '.', 'd.' will
> } recursively call itself
> 
> Confirmed.
> 
> Seems to be a problem with calling startvichange(1) before calling
> getvirange(), so the input is captured as a change event whether it
> succeeds or fails.  Unfortunately, I'm not familiar enough with the
> vi emulation code to see how to back out of startvichange().

Is it simply a matter of setting vichgbufptr to 0 if getvirange()
returns -1?  I've tried that; it fixes the problem and passes
tests.

Also here's a docs patch with my understanding some related globals.

diff --git a/Src/Zle/zle_vi.c b/Src/Zle/zle_vi.c
index baa2064..2cddea7 100644
--- a/Src/Zle/zle_vi.c
+++ b/Src/Zle/zle_vi.c
@@ -47,10 +47,19 @@ int vilinerange;
 
 /* last vi change buffer, for vi change repetition */
 
+/*
+ * vichgbufsz: Allocated size of vichgbuf.
+ * vichgbufptr: Length in use.
+ * vichgflag: true whilst inputting a vi normal mode; causes it to be
+ *   accumulated in vichgbuf, incrementing vichgbufptr.
+ */
+
 /**/
 int vichgbufsz, vichgbufptr, vichgflag;
 
 /*
+ * The bytes that make up the current vi command.  See vichgbuf* above.
+ *
  * Examination of the code suggests vichgbuf is consistently tied
  * to raw byte input, so it is left as a character array rather
  * than turned into wide characters.  In particular, when we replay

Re: Bug in ZSH's vi emulation

[Oliver Kiddle]

Daniel Shahaf wrote:
> > On Oct 5, 10:09am, Ghjuvan' Lacambre wrote:
> > } 'd.' isn't a valid action as '.' is not a valid motion, yet 'd.' is
> > } recorded as the last action called. When using '.', 'd.' will
> > } recursively call itself

> Is it simply a matter of setting vichgbufptr to 0 if getvirange()
> returns -1?  I've tried that; it fixes the problem and passes
> tests.

Not really. If we want a quick fix for 5.3 then either that or checking
virangeflag at the top of virepeatchange will do the job.

For a correct vi compatible fix, anything that causes the widget -
vi-delete or whatever to return failure should abort the vi change
and the previous value of vichgbuf should be restored. So most
occurences of vichgflag = 0 could change to call a new endvichange() which
would do the restore. Or that might be factored out into the caller.

I'm also pondering the interaction with user-defined widgets.
Note that from vi, with a key mapping of :nmap \d dwdw
Then, \d followed by dot will remove three words not four.
dot only repeats the final change. In practice this isn't especially
useful so things like the repeat.vim plugin exist. I'm not sure
that we should even strive for vi compatibility here but I think
we're missing some logic in cases where calls to startvichange occur
as part of a user-defined widget.

We've also got a separate issue of only lastchar being stuffed into
vichgbuf so repeating, e.g. gU doesn't work. Why is keybuflen only
1 in startvichange? That, along with what the general point of lastchar
is, has me fairly puzzled.

Oliver

Re: Bug in ZSH's vi emulation

[Peter Stephenson]

On Wed, 02 Nov 2016 17:51:23 +0100
Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
> We've also got a separate issue of only lastchar being stuffed into
> vichgbuf so repeating, e.g. gU doesn't work. Why is keybuflen only
> 1 in startvichange? That, along with what the general point of lastchar
> is, has me fairly puzzled.

The thing that gets stuffed into vichgbuf[0] in startvichange() is the
last byte of the binding that caused the change to start, not the range
of the change.  So if you issue "c" then the "c" goes into the buffer
here and the target gets handled later.  I haven't traced it further
than that, but this would only screw up with atypical bindings for
vi-change, vi-delete, etc. Presumably extracting the full set of bytes
for the command would work.

This wouldn't work with a multibyte binding, for example, e.g. if you
had ¢ on your keyboard and thought it would be a great alternative to c
then only the last byte of the character would be stored.  At some
point I think it got fixed up so the keybuf is correct for cases
like this.

pws

Re: Bug in ZSH's vi emulation

[Bart Schaefer]

On Nov 2,  5:51pm, Oliver Kiddle wrote:
} Subject: Re: Bug in ZSH's vi emulation
}
} Daniel Shahaf wrote:
} > Is it simply a matter of setting vichgbufptr to 0 if getvirange()
} > returns -1?
} 
} Not really. If we want a quick fix for 5.3 then either that or checking
} virangeflag at the top of virepeatchange will do the job.

It'd be nice to quash the infinite loop, even if the result isn't
entirely identical to vi behavior.

Re: Bug in ZSH's vi emulation

[Bart Schaefer]

On Nov 2,  5:51pm, Oliver Kiddle wrote:
}
} checking virangeflag at the top of virepeatchange will do the job.

Regardless of what else eventually happens, this seems like a wise idea.


diff --git a/Src/Zle/zle_vi.c b/Src/Zle/zle_vi.c
index baa2064..1e0402d 100644
--- a/Src/Zle/zle_vi.c
+++ b/Src/Zle/zle_vi.c
@@ -780,7 +780,7 @@ int
 virepeatchange(UNUSED(char **args))
 {
     /* make sure we have a change to repeat */
-    if (!vichgbuf || vichgflag)
+    if (!vichgbuf || vichgflag || virangeflag)
 	return 1;
     /* restore or update the saved count and buffer */
     if (zmod.flags & MOD_MULT) {

Re: Bug in ZSH's vi emulation

[Daniel Shahaf]

Bart Schaefer wrote on Wed, Nov 02, 2016 at 16:19:26 -0700:
> On Nov 2,  5:51pm, Oliver Kiddle wrote:
> }
> } checking virangeflag at the top of virepeatchange will do the job.
> 
> Regardless of what else eventually happens, this seems like a wise idea.
> 

In this case, and given Oliver's comments, I'll table the idea from
39806 to reset vichgbufptr: I only intended that as a stopgap to plug
the infinite loop.

Re: Bug in ZSH's vi emulation

[Bart Schaefer]

On Nov 2,  5:51pm, Oliver Kiddle wrote:
}
} We've also got a separate issue of only lastchar being stuffed into
} vichgbuf so repeating, e.g. gU doesn't work. Why is keybuflen only
} 1 in startvichange? That, along with what the general point of lastchar
} is, has me fairly puzzled.

It's because of getkeymapcmd():

    if(lastlen != keybuflen) {
	unmetafy(keybuf + lastlen, &keybuflen);		<-- here
	ungetbytes(keybuf+lastlen, keybuflen);
	if(vichgflag)
	    vichgbufptr -= keybuflen;
	keybuf[lastlen] = 0;
    }

Seems like it ought to do the following?  Or else use a temporary instead
of stomping on keybuflen.

diff --git a/Src/Zle/zle_keymap.c b/Src/Zle/zle_keymap.c
index 3db4207..b5244b5 100644
--- a/Src/Zle/zle_keymap.c
+++ b/Src/Zle/zle_keymap.c
@@ -1622,7 +1622,7 @@ getkeymapcmd(Keymap km, Thingy *funcp, char **strp)
 	ungetbytes(keybuf+lastlen, keybuflen);
 	if(vichgflag)
 	    vichgbufptr -= keybuflen;
-	keybuf[lastlen] = 0;
+	keybuf[keybuflen = lastlen] = 0;
     }
     *funcp = func;
     *strp = str;

Re: Bug in ZSH's vi emulation

[Peter Stephenson]

On Wed, 02 Nov 2016 21:12:43 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Nov 2,  5:51pm, Oliver Kiddle wrote:
> }
> } We've also got a separate issue of only lastchar being stuffed into
> } vichgbuf so repeating, e.g. gU doesn't work. Why is keybuflen only
> } 1 in startvichange? That, along with what the general point of lastchar
> } is, has me fairly puzzled.
> 
> It's because of getkeymapcmd():
> 
>     if(lastlen != keybuflen) {
> 	unmetafy(keybuf + lastlen, &keybuflen);		<-- here
> 	ungetbytes(keybuf+lastlen, keybuflen);
> 	if(vichgflag)
> 	    vichgbufptr -= keybuflen;
> 	keybuf[lastlen] = 0;
>     }
> 
> Seems like it ought to do the following?  Or else use a temporary instead
> of stomping on keybuflen.
> 
> diff --git a/Src/Zle/zle_keymap.c b/Src/Zle/zle_keymap.c
> index 3db4207..b5244b5 100644
> --- a/Src/Zle/zle_keymap.c
> +++ b/Src/Zle/zle_keymap.c
> @@ -1622,7 +1622,7 @@ getkeymapcmd(Keymap km, Thingy *funcp, char **strp)
>  	ungetbytes(keybuf+lastlen, keybuflen);
>  	if(vichgflag)
>  	    vichgbufptr -= keybuflen;
> -	keybuf[lastlen] = 0;
> +	keybuf[keybuflen = lastlen] = 0;
>      }
>      *funcp = func;
>      *strp = str;
> 

Yes, I stared at this for a while but I think the intention is to take
the (keybuflen-lastlen) null-terminated characters off the end and make
them available for re-reading as input, leaving only the first lastlen
in the key buffer.

That means keybuflen needs updating at the end anyway, to get rid of
those characters from the key buffer, so use of a temporary is
immaterial.

Comments when this was originally written probably wouldn't have hurt.

pws

Re: Bug in ZSH's vi emulation

[Bart Schaefer]

On Nov 2,  5:51pm, Oliver Kiddle wrote:
}
} We've also got a separate issue of only lastchar being stuffed into
} vichgbuf so repeating, e.g. gU doesn't work.

So, given 39813, this seems to work:

diff --git a/Src/Zle/zle_vi.c b/Src/Zle/zle_vi.c
index 1e0402d..bd692af 100644
--- a/Src/Zle/zle_vi.c
+++ b/Src/Zle/zle_vi.c
@@ -91,14 +91,15 @@ startvichange(int im)
 	lastmod = zmod;
 	if (vichgbuf)
 	    free(vichgbuf);
-	vichgbuf = (char *)zalloc(vichgbufsz = 16);
+	vichgbuf = (char *)zalloc(vichgbufsz = 16 + keybuflen);
 	if (im == -2) {
 	    vichgbuf[0] =
 		zlell ? (insmode ? (zlecs < zlell ? 'i' : 'a') : 'R') : 'o';
+	    vichgbufptr = 1;
 	} else {
-	    vichgbuf[0] = lastchar;
+	    strcpy(vichgbuf, keybuf);
+	    unmetafy(vichgbuf, &vichgbufptr);
 	}
-	vichgbufptr = 1;
 	vichgrepeat = 0;
     }
 }


All X02zlevi tests pass, and I'm even able to do e.g. gUfx to uppercase
everything through the next "x".

Re: Bug in ZSH's vi emulation

[Oliver Kiddle]

Bart wrote:
> So, given 39813, this seems to work:

That, along with 39813, looks good. As a further tweak, I had to
make keybuflen not be declared static. It might also be good to
tweak the comment leading up to this function (startvichange) to
reference keybuf instead of lastchar.

> All X02zlevi tests pass, and I'm even able to do e.g. gUfx to uppercase
> everything through the next "x".

Works well in my testing too. We should add a new testcase too such
as that below.

Oliver

diff --git a/Test/X02zlevi.ztst b/Test/X02zlevi.ztst
index ced7030..aa3587e 100644
--- a/Test/X02zlevi.ztst
+++ b/Test/X02zlevi.ztst
@@ -244,6 +244,12 @@
 >BUFFER: binging
 >CURSOR: 3
 
+# for vi compatibility, this should repeat the previous change
+  zletest $'worm\erdhd..'
+0:use of vi-repeat as the motion and repeat after a failed change
+>BUFFER: word
+>CURSOR: 2
+
   zpty_run 'bindkey "^_" undo'
   zletest $'undoc\037e'
 0:use of undo in vi insert mode

Re: Bug in ZSH's vi emulation

[Bart Schaefer]

On Nov 3,  1:43pm, Oliver Kiddle wrote:
}
} That, along with 39813, looks good. As a further tweak, I had to
} make keybuflen not be declared static.

I snuck that into 39813 ...

} It might also be good to
} tweak the comment leading up to this function (startvichange) to
} reference keybuf instead of lastchar.

Indeed.

} Works well in my testing too. We should add a new testcase too such
} as that below.

Shall I commit that or will you?

Re: Bug in ZSH's vi emulation

[Bart Schaefer]

On Nov 3,  1:43pm, Oliver Kiddle wrote:
}
} It might also be good to
} tweak the comment leading up to this function (startvichange) to
} reference keybuf instead of lastchar.

When making this change I also noticed that vichgbuf was not being
nul-terminated in the (im == -2) case.

Updated patch (replaces 39814):

diff --git a/Src/Zle/zle_vi.c b/Src/Zle/zle_vi.c
index 1e0402d..95646a9 100644
--- a/Src/Zle/zle_vi.c
+++ b/Src/Zle/zle_vi.c
@@ -71,7 +71,7 @@ static int inrepeat, vichgrepeat;
  * im: >= 0: is an insertmode
  *    -1: skip setting insert mode
  *    -2: entering viins at start of editing from clean --- don't use
- *        inrepeat or lastchar, synthesise an i to enter insert mode.
+ *        inrepeat or keybuf, synthesise an entery to insert mode.
  */
 
 /**/
@@ -91,14 +91,16 @@ startvichange(int im)
 	lastmod = zmod;
 	if (vichgbuf)
 	    free(vichgbuf);
-	vichgbuf = (char *)zalloc(vichgbufsz = 16);
+	vichgbuf = (char *)zalloc(vichgbufsz = 16 + keybuflen);
 	if (im == -2) {
 	    vichgbuf[0] =
 		zlell ? (insmode ? (zlecs < zlell ? 'i' : 'a') : 'R') : 'o';
+	    vichgbuf[1] = '\0';
+	    vichgbufptr = 1;
 	} else {
-	    vichgbuf[0] = lastchar;
+	    strcpy(vichgbuf, keybuf);
+	    unmetafy(vichgbuf, &vichgbufptr);
 	}
-	vichgbufptr = 1;
 	vichgrepeat = 0;
     }
 }

Re: Bug in ZSH's vi emulation

[Bart Schaefer]

On Nov 3,  7:55am, Bart Schaefer wrote:
}
} + *        inrepeat or keybuf, synthesise an entery to insert mode.

I'll commit with that typo fixed.

Re: Bug in ZSH's vi emulation

[Bart Schaefer]

On Nov 3,  1:43pm, Oliver Kiddle wrote:
}
} Works well in my testing too. We should add a new testcase too such
} as that below.

Revised somewhat:

diff --git a/Test/X02zlevi.ztst b/Test/X02zlevi.ztst
index ced7030..70fef4c 100644
--- a/Test/X02zlevi.ztst
+++ b/Test/X02zlevi.ztst
@@ -244,6 +244,14 @@
 >BUFFER: binging
 >CURSOR: 3
 
+  print -u $ZTST_fd 'This test may hang the shell when it fails...'
+  zletest $'worm\erdhd..'
+0:use of vi-repeat as the motion and repeat after a failed change
+>BUFFER: word
+>CURSOR: 2
+F:For vi compatibility, "." should repeat the "rd" change after "d."
+F:Update result to ">BUFFER: wodd" if compatibility is repaired.
+
   zpty_run 'bindkey "^_" undo'
   zletest $'undoc\037e'
 0:use of undo in vi insert mode