Re: Obscure zsh history overflow with segfault

[Bart Schaefer <schaefer@brasslantern.com>]

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.