Re: Segfault on "task <Tab><Tab>" with zsh 5.0.2 [PATCH]

[Axel Beckert <abe@deuxchevaux.org>]

Hi,

TL;DR: Below is a patch which fixes the issue for me. Read the whole
mail for the reasoning.

On Tue, Sep 17, 2013 at 08:05:51PM -0700, Bart Schaefer wrote:
> The stack trace from the very first message in the thread indicates
> it dies dereferencing the descr field of a Cvdef structure from the
> cvdef_cache array.  The trace doesn't show the call to zsfree so it
> is the Cvdef pointer itself that is bad, not the struct contents.
> 
> The valgrind output confirms this, which seems to indicate that the
> cvdef_cache array is being scribbled on ... but that's static memory,
> so it would have to be from overflowing some adjacent static storage
> (?) and 0x100000001 looks suspicious as a value that's repeatably so
> scribbled.
> 
> A watchpoint on cvdef_cache[0] might shed some light.

Thanks for that hint. Helped me to get to the right point:

(gdb) break Src/Zle/computil.c:4920
No source file named Src/Zle/computil.c.
Make breakpoint pending on future shared library load? (y or [n]) y
Breakpoint 1 (Src/Zle/computil.c:4920) pending.
(gdb) watch cvdef_cache[0]
No symbol "cvdef_cache" in current context.
(gdb) r
Starting program: /bin/zsh5 -f
warning: Could not load shared library symbols for linux-vdso.so.1.
Do you need "set solib-search-path" or "set sysroot"?
kiva6% autoload -Uz compinit
kiva6% compinit
kiva6% task 
Breakpoint 1, setup_ (m=0x6d6280) at ../../../Src/Zle/computil.c:4924
4924    ../../../Src/Zle/computil.c: No such file or directory.
(gdb) watch cvdef_cache[0]
Hardware watchpoint 2: cvdef_cache[0]
(gdb) c
Continuing.
Hardware watchpoint 2: cvdef_cache[0]

Old value = (Cvdef) 0x0
New value = (Cvdef) 0x8104c0
bin_compvalues (nam=<optimized out>, args=<optimized out>, ops=<optimized out>, func=<optimized out>) at ../../../Src/Zle/computil.c:3348
3348    in ../../../Src/Zle/computil.c
(gdb) c
Continuing.

Program received signal SIGSEGV, Segmentation fault.
freecvdef (d=0x100000001) at ../../../Src/Zle/computil.c:2799
2799    in ../../../Src/Zle/computil.c

Next try with a breakpoint on freecvdef calls:

(gdb) break Src/Zle/computil.c:2797
No source file named Src/Zle/computil.c.
Make breakpoint pending on future shared library load? (y or [n]) y
Breakpoint 1 (Src/Zle/computil.c:2797) pending.
(gdb) r
Starting program: /bin/zsh5 -f
warning: Could not load shared library symbols for linux-vdso.so.1.
Do you need "set solib-search-path" or "set sysroot"?
kiva6% autoload -Uz compinit
kiva6% compinit
kiva6% task 
Breakpoint 1, freecvdef (d=0x100000001) at ../../../Src/Zle/computil.c:2799
2799            zsfree(d->descr);
(gdb) print d
$1 = (Cvdef) 0x100000001
(gdb) print cvdef_cache
$7 = {0x8104c0, 0x809be0, 0x7cbc00, 0x7edde0, 0x7d0c50, 0x7ed680, 0x7cfe80, 0x7ed920}
(gdb) explore 0x7ed920
'0x7ed920' is a scalar value of type 'int'.
0x7ed920 = 8313120
(gdb) explore cvdef_cache
'cvdef_cache' is an array of 'Cvdef'.
Enter the index of the element you want to explore in 'cvdef_cache': 0
The value of 'cvdef_cache[0]' is of type 'Cvdef' which is a typedef of type 'struct cvdef *'
'cvdef_cache[0]' is a pointer to a value of type 'struct cvdef'
Continue exploring it as a pointer to a single value [y/n]: y

  The value of '*(cvdef_cache[0])' is a struct/class of type 'struct cvdef' with the following fields:

   descr = <Enter 0 to explore this field of type 'char *'>
  hassep = 1 .. (Value of type 'int')
     sep = 58 ':' .. (Value of type 'char')
  argsep = 61 '=' .. (Value of type 'char')
    next = <Enter 4 to explore this field of type 'Cvdef'>
    vals = <Enter 5 to explore this field of type 'Cvval'>
    defs = <Enter 6 to explore this field of type 'char **'>
   ndefs = 4 .. (Value of type 'int')
   lastt = 1379534378 .. (Value of type 'int')
   words = 0 .. (Value of type 'int')

Enter the field number of choice: 0
'(*(cvdef_cache[0])).descr' is a pointer to a value of type 'char'
Continue exploring it as a pointer to a single value [y/n]: y
'*((*(cvdef_cache[0])).descr)' is a scalar value of type 'char'.
*((*(cvdef_cache[0])).descr) = 116 't'

Press enter to return to parent value: 

Returning to parent value...

The value of '*(cvdef_cache[0])' is a struct/class of type 'struct cvdef' with the following fields:

   descr = <Enter 0 to explore this field of type 'char *'>
  hassep = 1 .. (Value of type 'int')
     sep = 58 ':' .. (Value of type 'char')
  argsep = 61 '=' .. (Value of type 'char')
    next = <Enter 4 to explore this field of type 'Cvdef'>
    vals = <Enter 5 to explore this field of type 'Cvval'>
    defs = <Enter 6 to explore this field of type 'char **'>
   ndefs = 4 .. (Value of type 'int')
   lastt = 1379534378 .. (Value of type 'int')
   words = 0 .. (Value of type 'int')

Enter the field number of choice: 

Returning to parent value...

'cvdef_cache' is an array of 'Cvdef'.
Enter the index of the element you want to explore in 'cvdef_cache': 
(gdb) print (*(cvdef_cache[0])).descr
$8 = 0x6d6550 "task attributes"
(gdb) print (*(cvdef_cache[1])).descr
$9 = 0x7cd110 "task attributes"
(gdb) print (*(cvdef_cache[2])).descr
$10 = 0x7ec3c0 "task attributes"
(gdb) print (*(cvdef_cache[3])).descr
$11 = 0x7d0390 "task attributes"
(gdb) print (*(cvdef_cache[4])).descr
$12 = 0x7cd810 "task attributes"
(gdb) print (*(cvdef_cache[5])).descr
$13 = 0x7b99d0 "task attributes"
(gdb) print (*(cvdef_cache[6])).descr
$14 = 0x7d09e0 "task attributes"
(gdb) print (*(cvdef_cache[7])).descr
$15 = 0x7d2840 "task attributes"
(gdb) print (*(cvdef_cache[8])).descr
Cannot access memory at address 0x100000001
(gdb) print cvdef_cache[8]
$16 = (Cvdef) 0x100000001

.oO( Bingo! That address looks familiar! )

Looking at the code:

Src/Zle/computil.c:929:#define MAX_CACACHE 8
[...]
Src/Zle/computil.c-2982-static Cvdef
Src/Zle/computil.c-2983-get_cvdef(char *nam, char **args)
Src/Zle/computil.c-2984-{
Src/Zle/computil.c-2985-    Cvdef *p, *min, new;
Src/Zle/computil.c-2986-    int i, na = arrlen(args);
Src/Zle/computil.c-2987-
Src/Zle/computil.c-2988-    for (i = MAX_CVCACHE, p = cvdef_cache, min = NULL; *p && i--; p++)
Src/Zle/computil.c-2989-    if (*p && na == (*p)->ndefs && arrcmp(args, (*p)->defs)) {
Src/Zle/computil.c-2990-        (*p)->lastt = time(0);
Src/Zle/computil.c-2991-
Src/Zle/computil.c-2992-        return *p;
Src/Zle/computil.c-2993-    } else if (!min || !*p || (*p)->lastt < (*min)->lastt)
Src/Zle/computil.c-2994-        min = p;
Src/Zle/computil.c-2995-    if (i)
Src/Zle/computil.c-2996-    min = p;
Src/Zle/computil.c-2997-    if ((new = parse_cvdef(nam, args))) {
Src/Zle/computil.c:2998:    freecvdef(*min);
Src/Zle/computil.c-2999-    *min = new;
Src/Zle/computil.c-3000-    }
Src/Zle/computil.c-3001-    return new;
Src/Zle/computil.c-3002-}

This looks to me as if there's somewehre an off-by-one error which
allows *min to become cvdef_cache[MAX_CVCACHE] aka cvdef_cache[8].

Let's iterate through that for-loop:

First 7 rounds:

i is decremented 7x to 1;
p is incremented 7x to cvdef_cache[7]

"*p && na == (*p)->ndefs && arrcmp(args, (*p)->defs)" never is true,
otherwise the function would have return *p.

Hence the else part runs:

!min is true because min never has been touched and initialised to
NULL, so min becomes set to p.

8th round: *p is still true, i-- is still true (as i is 1 before). p
becomes cvdef_cache[8].

"*p && na == (*p)->ndefs && arrcmp(args, (*p)->defs)" is still false,
otherwise the function would have return *p.

!min is false (as min is now cvdef_cache[7]).

"!*p" is probably false and "(*p)->lastt < (*min)->lastt" probably
too. (In both cases gdb says "Cannot access memory at address", once
0x10000002d and once 0x100000001.) At least that's what my first tries
to patch this issue at this point showed.

So now i is 0 and *p still true.

9th (!) round starting: *p is still true. i is not more true, hence
the body is no more executed. But i-- still is run and hence i is now
-1.

We are now at line 2995: i is -1 and hence true again.

Hence min gets set to p (which is cvdef_cache[8] and hence bogus) and
hence freecvdef(*min) goes boom.

So my patch (against 5.0.2) is as follows:

Index: zsh/Src/Zle/computil.c
===================================================================
--- zsh.orig/Src/Zle/computil.c 2013-09-18 23:09:08.000000000 +0200
+++ zsh/Src/Zle/computil.c      2013-09-18 23:28:55.000000000 +0200
@@ -2992,7 +2992,7 @@
            return *p;
        } else if (!min || !*p || (*p)->lastt < (*min)->lastt)
            min = p;
-    if (i)
+    if (i > 0)
        min = p;
     if ((new = parse_cvdef(nam, args))) {
        freecvdef(*min);

.oO( So much debugging for those two additional characters... )

HTH.

		Kind regards, Axel
-- 
/~\  Plain Text Ribbon Campaign                   | Axel Beckert
\ /  Say No to HTML in E-Mail and News            | abe@deuxchevaux.org  (Mail)
 X   See http://www.asciiribbon.org/              | abe@noone.org (Mail+Jabber)
/ \  I love long mails: http://email.is-not-s.ms/ | http://noone.org/abe/ (Web)