PATCH: [RFC] crash with weird completer

PATCH: [RFC] crash with weird completer

[Mikael Magnusson]

minimal reproducer .zshrc:

  zstyle ':completion:*' completer _oldlist _complete
  setopt nolistambiguous
  autoload compinit; compinit
  compdef _foo foo;_foo() { compadd -Q -- stash@{{0,1}} }

type "foo " and press tab twice, and it should crash.

The patch prevents the crash but I have no further arguments that it's
correct. Also without 'setopt nolistambiguous' we just get an additional
{ inserted for every tab press which also doesn't seem useful (not fixed
by patch).

(Apologies if I've sent this before, looking through local commits that
could potentially be useful to send.)
---
 Src/Zle/compresult.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Src/Zle/compresult.c b/Src/Zle/compresult.c
index 8b5955819a..12585e3564 100644
--- a/Src/Zle/compresult.c
+++ b/Src/Zle/compresult.c
@@ -608,7 +608,7 @@ instmatch(Cmatch m, int *scs)
     r += l;
     ocs = zlemetacs;
     /* Re-insert the brace beginnings, if any. */
-    if (brbeg) {
+    if (brbeg && m->brpl) {
 	int pcs = zlemetacs;
 
 	l = 0;
-- 
2.15.1

Re: PATCH: [RFC] crash with weird completer

[Bart Schaefer]

On Tue, Mar 8, 2022 at 11:33 AM Mikael Magnusson <mikachu@gmail.com> wrote:
>
> type "foo " and press tab twice, and it should crash.

Alternate patch below.  This differs in that it initializes the
"lastprebr" array, which step is skipped by Mikael's patch.

> The patch prevents the crash but I have no further arguments that it's
> correct.

Ditto.

> Also without 'setopt nolistambiguous' we just get an additional
> { inserted for every tab press which also doesn't seem useful (not fixed
> by patch).

This is unrelated and I think connected to something mentioned
elsewhere; I can't find the reference at the moment, but IIRC the gist
is that because you're completing after an open brace, it's removed
from the prefix in expectation of completing a brace expansion, so the
suffix to be added then appears to begin with a brace, which is added
after the actual prefix on the line, resulting in doubled (etc.)
braces.

You can work around this by quoting the braces that should be treated literally:
    compdef _foo foo;_foo() { compadd -Q -- stash@\\{{0,1}\\} }

The patch:

diff --git a/Src/Zle/compresult.c b/Src/Zle/compresult.c
index 8b5955819..a1f2ec322 100644
--- a/Src/Zle/compresult.c
+++ b/Src/Zle/compresult.c
@@ -614,7 +614,7 @@ instmatch(Cmatch m, int *scs)
        l = 0;
        for (bp = brbeg, brpos = m->brpl,
                 bradd = (m->pre ? strlen(m->pre) : 0);
-            bp; bp = bp->next, brpos++) {
+            bp && brpos; bp = bp->next, brpos++) {
            zlemetacs = a + *brpos + bradd;
            pcs = zlemetacs;
            l = strlen(bp->str);

Re: PATCH: [RFC] crash with weird completer

[Mikael Magnusson]

On 3/10/22, Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Tue, Mar 8, 2022 at 11:33 AM Mikael Magnusson <mikachu@gmail.com> wrote:
>>
>> type "foo " and press tab twice, and it should crash.
>
> Alternate patch below.  This differs in that it initializes the
> "lastprebr" array, which step is skipped by Mikael's patch.
>
>> The patch prevents the crash but I have no further arguments that it's
>> correct.
>
> Ditto.
>
> You can work around this by quoting the braces that should be treated
> literally:
>     compdef _foo foo;_foo() { compadd -Q -- stash@\\{{0,1}\\} }

I don't remember the thought process that led to the test case, but in
that case you could just drop the -Q too, right?

> The patch:

Hmm, I guess this is safer in the case that somehow m->brpl is set but
a subsequent brpos is NULL. It's probably (definitely) not worth
bothering about skipping NULL entries to process other ones (eg,
continue instead of break when !brpos)

I was staring at the original diff in gmail trying to figure out what
I was looking at, and this should be equivalent to your patch but
spare the eyes of future generations slightly (this is directed at the
original code, not your patch),

diff --git a/Src/Zle/compresult.c b/Src/Zle/compresult.c
index 8b5955819a..0fed297b56 100644
--- a/Src/Zle/compresult.c
+++ b/Src/Zle/compresult.c
@@ -612,9 +612,10 @@ instmatch(Cmatch m, int *scs)
        int pcs = zlemetacs;

        l = 0;
-       for (bp = brbeg, brpos = m->brpl,
-                bradd = (m->pre ? strlen(m->pre) : 0);
-            bp; bp = bp->next, brpos++) {
+       bradd = (m->pre ? strlen(m->pre) : 0);
+       for (bp = brbeg, brpos = m->brpl;
+            bp && brpos;
+            bp = bp->next, brpos++) {
            zlemetacs = a + *brpos + bradd;
            pcs = zlemetacs;
            l = strlen(bp->str);

Eg, bradd is not involved with the loop condition at all, so don't set
it in the for-initializer, and put each foo;bar;baz; on separate
lines.

-- 
Mikael Magnusson