Bad effect of error in zle-line-pre-redraw

Bad effect of error in zle-line-pre-redraw

[Bart Schaefer]

If you happen to introduce an error into zle-line-pre-redraw,
your shell is pretty badly wrecked -- ZLE resets itself after every
character typed.  Stupid minimal example:

torch% zle-line-pre-redraw() { : ${bad_subscript[missing-bracket} }
torch% zle -N zle-line-pre-redraw
torch% e
zle-line-pre-redraw: invalid subscript
torch% e
torch% c
zle-line-pre-redraw: invalid subscript
torch% c
torch% h
zle-line-pre-redraw: invalid subscript
torch% h
torch% o
zle-line-pre-redraw: invalid subscript
torch% o
torch% 

Why the missing bracket isn't a syntax error at parse time rather
than an evaluation error at run time is left as an exercise; e.g. ksh
complains when defining the function.

Other hook functions (zle-history-line-set, etc.) don't have this side-
effect.

Re: Bad effect of error in zle-line-pre-redraw

[Mikael Magnusson]

On Sat, Nov 12, 2016 at 6:03 PM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> If you happen to introduce an error into zle-line-pre-redraw,
> your shell is pretty badly wrecked -- ZLE resets itself after every
> character typed.  Stupid minimal example:
>
> torch% zle-line-pre-redraw() { : ${bad_subscript[missing-bracket} }
> torch% zle -N zle-line-pre-redraw
> torch% e
> zle-line-pre-redraw: invalid subscript
> torch% e
> torch% c
> zle-line-pre-redraw: invalid subscript
> torch% c
> torch% h
> zle-line-pre-redraw: invalid subscript
> torch% h
> torch% o
> zle-line-pre-redraw: invalid subscript
> torch% o
> torch%
>
> Why the missing bracket isn't a syntax error at parse time rather
> than an evaluation error at run time is left as an exercise; e.g. ksh
> complains when defining the function.
>
> Other hook functions (zle-history-line-set, etc.) don't have this side-
> effect.

% foo() { : ${bad_subscript[missing-bracket} }
% zle -N self-insert foo
{5514|18:54:05|~}%
foo: invalid subscript
{5514|18:54:05|~}%
{5514|18:54:05|~}%
foo: invalid subscript
{5514|18:54:05|~}%
{5514|18:54:05|~}%

There are many many more ways to break a shell session, surely.

-- 
Mikael Magnusson

Re: Bad effect of error in zle-line-pre-redraw

[Bart Schaefer]

On Nov 12,  6:54pm, Mikael Magnusson wrote:
}
} There are many many more ways to break a shell session, surely.

Of course; e.g. just doing

zle -N self-insert this-does-not-exist

My point was more about the difference in behavior from the other hooks
than about the session being broken.  zle-line-pre-redraw does not use
zlecallhook(), but zlecallhook() resets errflag except for interrupts.


Why did you avoid zlecallhook(), again?

Re: Bad effect of error in zle-line-pre-redraw

[Mikael Magnusson]

On Sat, Nov 12, 2016 at 9:41 PM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> On Nov 12,  6:54pm, Mikael Magnusson wrote:
> }
> } There are many many more ways to break a shell session, surely.
>
> Of course; e.g. just doing
>
> zle -N self-insert this-does-not-exist
>
> My point was more about the difference in behavior from the other hooks
> than about the session being broken.  zle-line-pre-redraw does not use
> zlecallhook(), but zlecallhook() resets errflag except for interrupts.
>
>
> Why did you avoid zlecallhook(), again?

IIRC, and looking at the code, it needed to save more stuff around
calling the hook function. I think when I wrote the code,
zlecallhook() didn't exist, and the similar places I borrowed the code
from didn't have the errflag resetting. It does seem pretty likely
that restoring the errflag won't hurt.

-- 
Mikael Magnusson

Re: Bad effect of error in zle-line-pre-redraw

[Bart Schaefer]

On Nov 13,  3:45pm, Mikael Magnusson wrote:
}
} IIRC, and looking at the code, it needed to save more stuff around
} calling the hook function. I think when I wrote the code,
} zlecallhook() didn't exist, and the similar places I borrowed the code
} from didn't have the errflag resetting. It does seem pretty likely
} that restoring the errflag won't hurt.

Well, zlecallhook() has been around since 2010 and git says that
redrawhook() was 2015 ... but, no matter; how about:


diff --git a/Src/Zle/zle_main.c b/Src/Zle/zle_main.c
index 04b9357..1652b7c 100644
--- a/Src/Zle/zle_main.c
+++ b/Src/Zle/zle_main.c
@@ -1041,28 +1041,43 @@ getrestchar(int inchar, char *outstr, int *outcount)
 #endif
 
 /**/
-void redrawhook(void)
+void
+redrawhook(void)
 {
     Thingy initthingy;
     if ((initthingy = rthingy_nocreate("zle-line-pre-redraw"))) {
+	/* Duplicating most of zlecallhook() to save additional state */
+	int saverrflag = errflag, savretflag = retflag;
 	int lastcmd_prev = lastcmd;
 	int old_incompfunc = incompfunc;
 	char *args[2];
 	Thingy lbindk_save = lbindk, bindk_save = bindk;
+
 	refthingy(lbindk_save);
 	refthingy(bindk_save);
 	args[0] = initthingy->nam;
 	args[1] = NULL;
+
+	/* The generic redraw hook cannot be a completion function, so
+	 * temporarily reset state for special variable handling etc.
+	 */
 	incompfunc = 0;
 	execzlefunc(initthingy, args, 1);
 	incompfunc = old_incompfunc;
+
+	/* Restore errflag and retflag as zlecallhook() does */
+	errflag = saverrflag | (errflag & ERRFLAG_INT);
+	retflag = savretflag;
+
 	unrefthingy(initthingy);
 	unrefthingy(lbindk);
 	unrefthingy(bindk);
 	lbindk = lbindk_save;
 	bindk = bindk_save;
+
 	/* we can't set ZLE_NOTCOMMAND since it's not a legit widget, so
-	 * restore lastcmd manually so that we don't mess up the global state */
+	 * restore lastcmd manually so that we don't mess up the global state
+	 */
 	lastcmd = lastcmd_prev;
     }
 }

[PATCH] Widget fallbacks (Re: Bad effect of error in zle-line-pre-redraw)

[Bart Schaefer]

On Nov 12, 12:41pm, Bart Schaefer wrote:
}
} On Nov 12,  6:54pm, Mikael Magnusson wrote:
} }
} } There are many many more ways to break a shell session, surely.
} 
} Of course; e.g. just doing
} 
} zle -N self-insert this-does-not-exist

Seems to me that if we have immortal widgets backing the built-ins,
we should use them in cases like this?

IMO the only controversial bit of this patch is the final hunk.  As
there is no way to tell at what point the user-defined widget failed,
calling the immortal counterpart might duplicate something that was
already done.  In the other cases we know nothing else has happened.

An alternate approach for that final bit is to simply bail out of ZLE
entirely (opts[USEZLE] = 0).  That's a little tricky if we're a couple
of user-defined widgets down the call stack, as localoptions may end
up clobbering whatever we do here.


diff --git a/Src/Zle/zle_main.c b/Src/Zle/zle_main.c
index 96b631e..1652b7c 100644
--- a/Src/Zle/zle_main.c
+++ b/Src/Zle/zle_main.c
@@ -1368,6 +1368,16 @@ zleread(char **lp, char **rp, int flags, int context, char *init, char *finish)
     return s;
 }
 
+/**/
+static int
+execimmortal(Thingy func, char **args)
+{
+    Thingy immortal = rthingy_nocreate(dyncat(".", func->nam));
+    if (immortal)
+	return execzlefunc(immortal, args, 0);
+    return 1;
+}
+
 /*
  * Execute a widget.  The third argument indicates that the global
  * variable bindk should be set temporarily so that WIDGET etc.
@@ -1389,7 +1399,7 @@ execzlefunc(Thingy func, char **args, int set_bindk)
 	remetafy = 1;
     }
 
-    if(func->flags & DISABLED) {
+    if (func->flags & DISABLED) {
 	/* this thingy is not the name of a widget */
 	char *nm = nicedup(func->nam, 0);
 	char *msg = tricat("No such widget `", nm, "'");
@@ -1397,7 +1407,7 @@ execzlefunc(Thingy func, char **args, int set_bindk)
 	zsfree(nm);
 	showmsg(msg);
 	zsfree(msg);
-	ret = 1;
+	ret = execimmortal(func, args);
     } else if((w = func->widget)->flags & (WIDGET_INT|WIDGET_NCOMP)) {
 	int wflags = w->flags;
 
@@ -1461,7 +1471,7 @@ execzlefunc(Thingy func, char **args, int set_bindk)
 	    zsfree(nm);
 	    showmsg(msg);
 	    zsfree(msg);
-	    ret = 1;
+	    ret = execimmortal(func, args);
 	} else {
 	    int osc = sfcontext, osi = movefd(0);
 	    int oxt = isset(XTRACE);
@@ -1483,6 +1493,8 @@ execzlefunc(Thingy func, char **args, int set_bindk)
 	    opts[XTRACE] = oxt;
 	    sfcontext = osc;
 	    endparamscope();
+	    if (errflag == ERRFLAG_ERROR && !(ret = execimmortal(func, args)))
+		errflag &= ~ERRFLAG_ERROR;
 	    lastcmd = w->flags & ~(WIDGET_INUSE|WIDGET_FREE);
 	    if (inuse) {
 		w->flags &= WIDGET_INUSE|WIDGET_FREE;

Re: Bad effect of error in zle-line-pre-redraw

[Mikael Magnusson]

On Sun, Nov 13, 2016 at 7:55 PM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> On Nov 13,  3:45pm, Mikael Magnusson wrote:
> }
> } IIRC, and looking at the code, it needed to save more stuff around
> } calling the hook function. I think when I wrote the code,
> } zlecallhook() didn't exist, and the similar places I borrowed the code
> } from didn't have the errflag resetting. It does seem pretty likely
> } that restoring the errflag won't hurt.
>
> Well, zlecallhook() has been around since 2010 and git says that
> redrawhook() was 2015 ... but, no matter; how about:

Right, but it started life in my personal git tree earlier in 2010
than zlecallhook() :)

The patch looks fine to me, I wish I had added some comments about why
the additonal restores were needed... They kinda grew over the years
until it actually worked reasonably well.


-- 
Mikael Magnusson