Bug(?) in builtin r

Bug(?) in builtin r

[Richard Hartmann]

Hi all,

I am sure most of you are aware of this, but it has always
ailed me and should probably be changed.
If you have 100 commands on your stack and call

  r 101

it will recurse endlessly or as long as the box allows it to
do until killing it. Unless there are useful aspects to this
behaviour, it is basically a nice way to kill your shell.


Thoughts?
Richard

Re: Bug(?) in builtin r

[thomasg]

[-- Attachment #1: Type: text/plain, Size: 575 bytes --]

Just tried it - it kills my shell immediately. Definitely doesn't sound like
a feature. :)

On Thu, Jul 24, 2008 at 4:52 PM, Richard Hartmann <
richih.mailinglist@gmail.com> wrote:

> Hi all,
>
> I am sure most of you are aware of this, but it has always
> ailed me and should probably be changed.
> If you have 100 commands on your stack and call
>
>  r 101
>
> it will recurse endlessly or as long as the box allows it to
> do until killing it. Unless there are useful aspects to this
> behaviour, it is basically a nice way to kill your shell.
>
>
> Thoughts?
> Richard
>

[-- Attachment #2: Type: text/html, Size: 910 bytes --]

Re: Bug(?) in builtin r

[Peter Stephenson]

On Thu, 24 Jul 2008 16:52:39 +0200
"Richard Hartmann" <richih.mailinglist@gmail.com> wrote:
> Hi all,
> 
> I am sure most of you are aware of this, but it has always
> ailed me and should probably be changed.
> If you have 100 commands on your stack and call
> 
>   r 101
> 
> it will recurse endlessly or as long as the box allows it to
> do until killing it. Unless there are useful aspects to this
> behaviour, it is basically a nice way to kill your shell.

Hmm... repeating the current line and doing something else with it (rather
than just repeating it) is potentially useful.  However, in zsh this is
handled recursively, so you quickly get into problems on the stack.  (I
have a suspicion in ksh it puts it onto the input stack and this problem is
avoided.)  It would therefore be safer to disallow it.

This disallows the specific case that the range includes the current
history entry and nothing before it, and there's no editor.  Otherwise it
either truncates the history so that the current line isn't included, or if
you've specified an editor it lets you edit the current line, too, if you
really want.  It's not clear that last bit is actually useful and it might
be better consistently to ignore the current history line here.

Index: Src/builtin.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/builtin.c,v
retrieving revision 1.197
diff -u -r1.197 builtin.c
--- Src/builtin.c	17 Jul 2008 11:27:57 -0000	1.197
+++ Src/builtin.c	24 Jul 2008 17:13:39 -0000
@@ -1446,20 +1446,28 @@
 	    unqueue_signals();
 	    zwarnnam("fc", "can't open temp file: %e", errno);
 	} else {
+	    char *editor;
+
+	    if (func == BIN_R)
+		editor = "-";
+	    else if (OPT_HASARG(ops, 'e'))
+		editor = OPT_ARG(ops, 'e');
+	    else
+		editor = getsparam("FCEDIT");
+	    if (!editor)
+		editor = getsparam("EDITOR");
+	    if (!editor)
+		editor = DEFAULT_FCEDIT;
+	    if (last >= curhist && !strcmp(editor, "-")) {
+		last = curhist - 1;
+		if (first > last) {
+		    unqueue_signals();
+		    zwarnnam("fc", "invalid use of current history line");
+		    return 1;
+		}
+	    }
 	    ops->ind['n'] = 1;	/* No line numbers here. */
 	    if (!fclist(out, ops, first, last, asgf, pprog)) {
-		char *editor;
-
-		if (func == BIN_R)
-		    editor = "-";
-		else if (OPT_HASARG(ops, 'e'))
-		    editor = OPT_ARG(ops, 'e');
-		else
-		    editor = getsparam("FCEDIT");
-		if (!editor)
-		    editor = getsparam("EDITOR");
-		if (!editor)
-		    editor = DEFAULT_FCEDIT;
 
 		unqueue_signals();
 		if (fcedit(editor, fil)) {

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070

Re: Bug(?) in builtin r

[Peter Stephenson]

On Thu, 24 Jul 2008 18:16:36 +0100
Peter Stephenson <pws@csr.com> wrote:
> Hmm... repeating the current line and doing something else with it (rather
> than just repeating it) is potentially useful.  However, in zsh this is
> handled recursively, so you quickly get into problems on the stack.

...and also you end up with huge numbers of temporary files that stack up
unless the whole sequence exits cleanly, which is possibly worse in the
long run.

> This disallows the specific case that the range includes the current
> history entry and nothing before it, and there's no editor.  Otherwise it
> either truncates the history so that the current line isn't included, or if
> you've specified an editor it lets you edit the current line, too, if you
> really want.  It's not clear that last bit is actually useful and it might
> be better consistently to ignore the current history line here.

That's what I'll do.  Editing the line with an external editor isn't all
that common and if you do it's likely to be better to reduce surprises
rather than provide the almost certainly unnecessary current history line.

I missed an unlink(fil) last time.

Index: Src/builtin.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/builtin.c,v
retrieving revision 1.197
diff -u -r1.197 builtin.c
--- Src/builtin.c	17 Jul 2008 11:27:57 -0000	1.197
+++ Src/builtin.c	25 Jul 2008 08:48:24 -0000
@@ -1446,6 +1446,20 @@
 	    unqueue_signals();
 	    zwarnnam("fc", "can't open temp file: %e", errno);
 	} else {
+	    /*
+	     * Nasty behaviour results if we use the current history
+	     * line here.  Treat it as if it doesn't exist, unless
+	     * that gives us an empty range.
+	     */
+	    if (last >= curhist) {
+		last = curhist - 1;
+		if (first > last) {
+		    unqueue_signals();
+		    zwarnnam("fc", "invalid use of current history line");
+		    unlink(fil);
+		    return 1;
+		}
+	    }
 	    ops->ind['n'] = 1;	/* No line numbers here. */
 	    if (!fclist(out, ops, first, last, asgf, pprog)) {
 		char *editor;


-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070

Re: Bug(?) in builtin r

[Richard Hartmann]

On Fri, Jul 25, 2008 at 10:52, Peter Stephenson <pws@csr.com> wrote:

> +                   zwarnnam("fc", "invalid use of current history line");

In my personal crusade for errors/warnings that pinpoint the
issue without the need to refer to documentation, I would
like to suggest a replacement/an addition along the lines
of 'using the current command from the history stack
leads to endless recursion'.


Thanks for the patch!
Richard

Re: Bug(?) in builtin r

[Peter Stephenson]

"Richard Hartmann" wrote:
> On Fri, Jul 25, 2008 at 10:52, Peter Stephenson <pws@csr.com> wrote:
> 
> > +                   zwarnnam("fc", "invalid use of current history line");
> 
> In my personal crusade for errors/warnings that pinpoint the
> issue without the need to refer to documentation, I would
> like to suggest a replacement/an addition along the lines
> of 'using the current command from the history stack
> leads to endless recursion'.

We need an error message, not an essay, which is what the documentation
is for, but if there's a more specific short message I can use that,
such as for example "current history line would cause recursion".

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

Re: Bug(?) in builtin r

[Richard Hartmann]

On Sun, Jul 27, 2008 at 19:27, Peter Stephenson
<p.w.stephenson@ntlworld.com> wrote:

> We need an error message, not an essay, which is what the documentation
> is for,

Agreed.


> but if there's a more specific short message I can use that,
> such as for example "current history line would cause recursion".

What about 'Using current history line would recurse endlessly'?
Optionally with 'aborting' before or after.


Richard