compctl bug with beta17 on Linux

compctl bug with beta17 on Linux

[Hrvoje Niksic]

Hi!

I've found a serious bug that causes zsh-2.6beta17 (but also beta14 and
probably lots of others) to dump core on Linux box with uname -a output:
Linux gnjilux.cc.fer.hr 1.3.98 #1 Wed May 8 03:52:40 MET DST 1996 i486


In some cases zsh coredumps when programmable completion is set, like in:
compctl -g '*(D-/)' cd

and
cd /home/p[TAB]

where the file/dir beginning with 'p' doesn't exist, it coredumps. It fails
always on this one, but in other cases it doesn't (like cd /tmp/x[TAB] when
there's no 'x' in /tmp). The problem repeats in some other cases, like in
cd /proc/<something>[TAB], which should mean that the problem is not at
Linux's side. The problem disappears if cd is not compctl-ed.

gdb shows that the shell fails in zle_tricky.c, in the second call of
get_ccompctl, where the cmdstr contains garbage (instead of "cd"), and
causes declaration char *cmd = dupstring(cmdstr) to coredump in strcpy.

Further analysis shows that cmdstr (or at least the contents of the memory
it points to) gets garbled when newlinklist() is called in zle_tricky, line
2174,
fmatches = newlinklist()

The contents cmdstr points to is garbled in alloc, which might show the bug
in allocation routines.

Zsh allocation routines were not used.

Re: compctl bug with beta17 on Linux

[Thorsten Meinecke]

In <zsh-workers@math.gatech.edu> archive/latest/1007,
Hrvoje Niksic <hniksic@gnjilux.cc.fer.hr> wrote:

> compctl -g '*(D-/)' cd
> cd /home/p[TAB]

[beta17 dumps core, if there's no file/dir named p* in /home]

I was able to reproduce this bug on my Linux-box.

> gdb shows that the shell fails in zle_tricky.c, in the second call of
> get_ccompctl, where the cmdstr contains garbage (instead of "cd"), and
> causes declaration char *cmd = dupstring(cmdstr) to coredump in strcpy.

Is this condition likely to occur?  SIGSEGV caused by overlapping
strings in strcpy().  That may explain why this bug went by unnoticed
for such a long time.

> Further analysis shows that cmdstr (or at least the contents of the memory
> it points to) gets garbled when newlinklist() is called

That's close.  You were lucky that the memory cmdstr pointed to wasn't
corrupted in the first place.  I do not claim that I understand even a
small percentage of what is going on in zle_tricky, but it looks like
there went something wrong with heap allocation in regard to cmdstr.

Assignments to cmdstr happen one or more times in get_comp_string(), de-
pending on the lexer's output.  The command strings found are stored onto
the current heap, and only the last (that is, innermost) one found will
be used subsequently.  But before anything useful can be done with cmdstr,
the heap will be released! 

That prompts my idea of the fix: simply make the string that cmdstr points
to permanent and free it when we are done.  Can somebody more proficient in
zle_tricky hacking comment on the risk of introducing a memory leak here?

Regards, Thorsten


*** 2.25	1996/05/05 10:06:07
--- zle_tricky.c	1996/05/08 19:00:49
***************
*** 575,582 ****
--- 575,583 ----
  	we -= chl;
  	if (wb < 0 || we < 0)
  	    return;
      }
+     cmdstr = ztrdup(cmdstr);
      freeheap();
      /* Save the lexer state, in case the completion code uses the lexer
         somewhere (e.g. when processing a compctl -s flag). */
      lexsave();
***************
*** 733,740 ****
--- 734,742 ----
      }
      /* Reset the lexer state, pop the heap. */
      lexrestore();
      popheap();
+     zsfree(cmdstr);
      zsfree(qword);
      menuce = complexpect;
  }
  

Re: compctl bug with beta17 on Linux

[Zoltan Hidvegi]

[-- Attachment #1: Type: application/pgp, Size: 2725 bytes --]

Re: compctl bug with beta17 on Linux

[Thorsten Meinecke]

In <zsh-workers@math.gatech.edu> archive/latest/1024,
Zoltan Hidvegi <hzoli@cs.elte.hu> wrote:

> The problem is that cmdstr is initialised in the middle of get_comp_string
> using dupstring() and there is a pushheap() before this and a pophheap()
> after this.  This means that cmdstr will be freed before returning from
> get_comp_string().

Methinks cmdstr is freed by freeheap() in docomplete(), in this case.

> [...] After that, the fix is trivial.

I beg to differ :-)  Stylistical issues aside (freeing a char* before
ever using it, even if it's guaranteed to be initialised with NULL),
Zoltan's patch will fall over, when for whatever reason there's more than
one command position considered in the do/while-loop in get_comp_string().
Invariably this will lead to memory leaks.  For example, when quotes and
newlines are involved:

% somecommand "
> anystring "
            ^ here's the cursor, and completion is requested

cmdstr will point to "anystring", but the memory occupied previously by
"somecommand" won't never be released.  That "anystring" is the result
might be seen as a bug in its own right, but who says that something like
this will never happen?

Your humble servant,
Thorsten

Re: compctl bug with beta17 on Linux

[Zoltan Hidvegi]

[-- Attachment #1: Type: application/pgp, Size: 2722 bytes --]