Obscure zsh history overflow with segfault

Obscure zsh history overflow with segfault

[Christian Neukirchen]

Hi,

I just found this:

% zsh --version    
zsh 4.3.15 (x86_64-unknown-linux-gnu)
% gdb --args zsh -f
(gdb) r
Starting program: /bin/zsh -f
juno% asdfgh
zsh: command not found: asdfgh
juno% r 100 asdfgh
[... lots of repeat]
r 100 asdfgh
asdfgh
r 100 asdfgh
asdfgh
r 100 asdfgh
asdfgh
r 100 asdfgh
asdfgh
r 100 asdfgh
asdfgh
r 100 asdfgh
asdfgh
r 100 asdfgh
[... lots of repeat]
Program received signal SIGSEGV, Segmentation fault.
0x0000000000450792 in zhalloc ()
#0  0x0000000000450792 in zhalloc ()
#1  0x0000000000451b76 in hcalloc ()
#2  0x0000000000449bc7 in ?? ()
#3  0x000000000044aa54 in zshlex ()
#4  0x0000000000465ca7 in ?? ()
#5  0x000000000046685d in ?? ()
#6  0x0000000000466a9c in ?? ()
#7  0x0000000000466b3b in ?? ()
#8  0x0000000000467768 in ?? ()
#9  0x000000000046781b in ?? ()
#10 0x000000000046781b in ?? ()
#11 0x000000000046781b in ?? ()
#12 0x000000000046781b in ?? ()
#13 0x000000000046781b in ?? ()
#14 0x000000000046781b in ?? ()
#15 0x000000000046781b in ?? ()
[... lots of exactly the same function]
#173682 0x000000000046781b in ?? ()
#173683 0x000000000046781b in ?? ()
#173684 0x000000000046781b in ?? ()
#173685 0x00000000004678e1 in parse_event ()
#173686 0x000000000043d3e9 in loop ()
#173687 0x000000000041c4a2 in bin_fc ()
#173688 0x000000000041cc64 in execbuiltin ()
#173689 0x000000000042ac62 in ?? ()
#173690 0x000000000042b2be in ?? ()
#173691 0x000000000042b675 in ?? ()
#173692 0x000000000042c73d in execlist ()
#173693 0x000000000042cccf in execode ()
#173694 0x000000000043d56f in loop ()
#173695 0x000000000041c4a2 in bin_fc ()
#173696 0x000000000041cc64 in execbuiltin ()
[... lots of repeat]
#173820 0x000000000042c73d in execlist ()
#173821 0x000000000042cccf in execode ()
#173822 0x000000000043d56f in loop ()
#173823 0x000000000041c4a2 in bin_fc ()
#173824 0x000000000041cc64 in execbuiltin ()
#173825 0x000000000042ac62 in ?? ()
#173826 0x000000000042b2be in ?? ()
#173827 0x000000000042b675 in ?? ()
#173828 0x000000000042c73d in execlist ()
#173829 0x000000000042cccf in execode ()
#173830 0x000000000043d442 in loop ()
#173831 0x00000000004401de in zsh_main ()
#173832 0x00007ffff710138d in __libc_start_main () from /lib/libc.so.6
#173833 0x000000000040ec11 in _start ()

Any ideas?  TBH, I don't even know what "r 100 asdfgh" should do,
it was a mistyping of "repeat 100 asdfgh".

Cheers,
-- 
Christian Neukirchen  <chneukirchen@gmail.com>  http://chneukirchen.org

Re: Obscure zsh history overflow with segfault

[Daniel Shahaf]

Christian Neukirchen wrote on Fri, Jan 20, 2012 at 20:42:10 +0100:
> Any ideas?  TBH, I don't even know what "r 100 asdfgh" should do,
> it was a mistyping of "repeat 100 asdfgh".

% which r
r: shell built-in command
% man zshall | grep -w r | grep -w fc 
       r      Same as fc -e -.
% run-help fc | head -30
...
% 

Apparently, 'r 100 foo' should re-execute the portion of your history
from event #100 to event 'foo'.

Sounds like a bad idea...

Re: Obscure zsh history overflow with segfault

[Bart Schaefer]

On Jan 21,  4:39am, Daniel Shahaf wrote:
}
} Apparently, 'r 100 foo' should re-execute the portion of your history
} from event #100 to event 'foo'.
} 
} Sounds like a bad idea...

Interesting.  Here we are:

1516                /*
1517                 * Nasty behaviour results if we use the current history
1518                 * line here.  Treat it as if it doesn't exist, unless
1519                 * that gives us an empty range.
1520                 */
1521                if (last >= curhist) {
1522                    last = curhist - 1;
1523                    if (first > last) {
1524                        unqueue_signals();
1525                        zwarnnam("fc",
1526                          "current history line would recurse endlessly, aborted");
1527                        fclose(out);
1528                        unlink(fil);
1529                        return 1;
1530                    }
1531                }

In the situation in this bug, first > last is true but last >= curhist
is false.  I believe that means that even though this is an infinite
loop, we don't detect that it will be.

However, it's worse than that:  If HIST_NO_STORE is not set, then even
when last < curhist and first <= last, its possible for the history to
contain an "r" command, which will be executed, retrieving a history
that contains an "r" command, which ...

One possible approach is for bin_fc to refuse to recursively run the
"r" command (same for "fc -e -").  However, it's then still possible
for the user to shoot himself with "fc -e cat ..." (or any other no-op
editor command); so short of disallowing any sort of recursive entry
into "fc" when -l is not given, this probably has to be treated as a
user error.

Re: Obscure zsh history overflow with segfault

[Peter Stephenson]

On Tue, 24 Jan 2012 12:28:19 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:

> On Jan 21,  4:39am, Daniel Shahaf wrote:
> }
> } Apparently, 'r 100 foo' should re-execute the portion of your history
> } from event #100 to event 'foo'.
> } 
> } Sounds like a bad idea...
> 
> Interesting.  Here we are:
> 
> 1516                /*
> 1517                 * Nasty behaviour results if we use the current history
> 1518                 * line here.  Treat it as if it doesn't exist, unless
> 1519                 * that gives us an empty range.
> 1520                 */
> 1521                if (last >= curhist) {
> 1522                    last = curhist - 1;
> 1523                    if (first > last) {
> 1524                        unqueue_signals();
> 1525                        zwarnnam("fc",
> 1526                          "current history line would recurse endlessly, aborted");
> 1527                        fclose(out);
> 1528                        unlink(fil);
> 1529                        return 1;
> 1530                    }
> 1531                }
> 
> In the situation in this bug, first > last is true but last >= curhist
> is false.  I believe that means that even though this is an infinite
> loop, we don't detect that it will be.

I'm not really following the problem, but do you mean something like
this?

Index: Src/builtin.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/builtin.c,v
retrieving revision 1.254
diff -p -u -r1.254 builtin.c
--- Src/builtin.c	29 Aug 2011 17:21:40 -0000	1.254
+++ Src/builtin.c	26 Jan 2012 20:56:28 -0000
@@ -1652,6 +1652,12 @@ fclist(FILE *f, Options ops, zlong first
 	last = first;
 	first = tmp;
     }
+    if (first > last) {
+	zwarnnam("fc", "history events are in wrong order, aborted");
+	if (f != stdout)
+	    fclose(f);
+	return 1;
+    }
     /* suppress "no substitution" warning if no substitution is requested */
     if (!subs)
 	fclistdone = 1;

-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

Re: Obscure zsh history overflow with segfault

[Bart Schaefer]

On Jan 26,  8:57pm, Peter Stephenson wrote:
} Subject: Re: Obscure zsh history overflow with segfault
}
} On Tue, 24 Jan 2012 12:28:19 -0800
} 
} > 1521                if (last >= curhist) {
} > 1522                    last = curhist - 1;
} > 1523                    if (first > last) {
} > 1529                        return 1;
} > 
} > In the situation in this bug, first > last is true but last >= curhist
} > is false.  I believe that means that even though this is an infinite
} > loop, we don't detect that it will be.
} 
} I'm not really following the problem, but do you mean something like
} this?
} 
} +    if (first > last) {
} +	zwarnnam("fc", "history events are in wrong order, aborted");
} +	if (f != stdout)
} +	    fclose(f);
} +	return 1;
} +    }

That would catch some nonsense inputs, but it doesn't prevent infinite
recursion on other nonsense, so the question is whether to bother with
catching nonsense at all, or just toss up our hands and say "garbage
in, garbage out."

Here's the easiest way to reproduce from "zsh -f":

% echo Start here
Start here
% r 1 echo
Start here
% r 2

At this point "r 2" loads the command "r 1 echo" and executes it.  This
in turn runs all the commands from event 1 to event 3, which happens to
include event 2, which is "r 1 echo" again, and we loop.  Even if the
original event 2 falls off the top due to HISTSIZE, there is always
another "echo" to be found, and always another "r 1 echo" somewhere
between the mininum history number and the "echo", so we never run out
of "r 1 echo" commands until we run out of stack space to keep calling
loop() recursively out of bin_fc().

The case of "r 100 foo" is the same thing except that the "too large"
number 100 is treated as curhist.

Incidentally, in zsh 4.2.x it was possible to stop this recursion with
a tty interrupt, but in 4.3.13 the interrupt is ignored and the loop
runs until stack overflow causes a segfault.  There are definitely some
signal handling "improvements" that have caused edge case regression in
recent revisions.

My other point was that one way to prevent the infinite recursion is to
prevent *any* recursion:

--- Src/builtin.c       20 Dec 2011 17:13:38 -0000      1.52
+++ Src/builtin.c       24 Jan 2012 20:54:08 -0000
@@ -1532,6 +1532,7 @@
            ops->ind['n'] = 1;  /* No line numbers here. */
            if (!fclist(out, ops, first, last, asgf, pprog)) {
                char *editor;
+               static char *loop_editor = 0;
 
                if (func == BIN_R)
                    editor = "-";
@@ -1545,12 +1546,19 @@
                    editor = DEFAULT_FCEDIT;
 
                unqueue_signals();
-               if (fcedit(editor, fil)) {
+               if (loop_editor) {
+                 zwarnnam("fc",
+                          "-e %s would recurse, aborted",
+                          loop_editor);
+                 retval = 1;
+               } else if (fcedit(editor, fil)) {
                    if (stuff(fil))
                        zwarnnam("fc", "%e: %s", errno, s);
                    else {
+                       loop_editor = editor;
                        loop(0,1);
                        retval = lastval;
+                       loop_editor = 0;
                    }
                }
            } else

However, that may be too draconian.  A static counter could be used
to allow some limited recursion depth (as for recursive functions),
but again it may be better just to say PEBKAC.