On Mon, Oct 29, 2018 at 09:55:38AM +0100, Martijn van Duren wrote:
> On 10/28/18 8:13 PM, Anton Lindqvist wrote:
> > Hi,
> > Bug reported by miod@, how to reproduce:
> >
> > $ command -v r
> > alias r='fc -s'
> > $ sleep 5
> > $ r sleep
> > ^C # abort sleep before finishing
> > $ r sleep
> > ksh: fc: history function called recursively
> >
> > The c_fc() function has some internal state used to prevent recursive
> > invocations that gets out of sync when the re-executed command is
> > aborted by SIGINT. My proposal is to temporarily register a SIGINT
> > handler before re-executing the command in order to reset the call depth
> > counter upon signal delivery.
> >
> > Comments? OK?
> >
> You diff always unsets the shtrap pointer, which most likely unsets
> custom SIGINT handlers installed by the user (untested). Next to that
> this feels like special-casing and I'm afraid that there are some cases
> we might be overlooking here.
Thanks for the feedback. A few drive by thoughts, will look more closely
this evening: a user supplied SIGINT handler is never registered as a
one shot handler so it should never be overwritten. The hist_sigint()
one shot handler is registered before executing the command, if the
command does register a SIGINT handler it takes higher precedence and
hist_execute() will return under such circumstances implying that
fc_depth will be correctly decremented. The one shot handler is only
used when the command does not register a SIGINT handler.
>
> The source with this issue is that a SIGINT triggers a list of
> siglongjmps, which don't allow us to return to c_fc and decrement the
> pointer. The diff below detects if we're on the lowest level of the
> unwinding by abusing the interactive variable. This works because
> calling fc from a non-interactive session is prohibited.
>
> I'm not 100% convinced that the placement of fc_depth = 0 should be
> placed in shell(), or that we maybe should place it in unwind(). The
> reason I choose for shell() is because interactive is guaranteed to
> be setup for checking, because it's already there.
>
> This diff is only lightly tested and the code here is very hard to
> untangle, so extreme scrutiny is desirable.
>
> martijn@
>
> Index: history.c
> ===================================================================
> RCS file: /cvs/src/bin/ksh/history.c,v
> retrieving revision 1.80
> diff -u -p -r1.80 history.c
> --- history.c 15 Jan 2018 22:30:38 -0000 1.80
> +++ history.c 29 Oct 2018 08:19:12 -0000
> @@ -48,6 +48,8 @@ static uint32_t line_co;
>
> static struct stat last_sb;
>
> +volatile sig_atomic_t fc_depth;
> +
> int
> c_fc(char **wp)
> {
> @@ -58,9 +60,8 @@ c_fc(char **wp)
> int optc, ret;
> char *first = NULL, *last = NULL;
> char **hfirst, **hlast, **hp;
> - static int depth;
>
> - if (depth != 0) {
> + if (fc_depth != 0) {
> bi_errorf("history function called recursively");
> return 1;
> }
> @@ -146,9 +147,9 @@ c_fc(char **wp)
> hist_get_newest(false);
> if (!hp)
> return 1;
> - depth++;
> + fc_depth++;
> ret = hist_replace(hp, pat, rep, gflag);
> - depth--;
> + fc_depth--;
> return ret;
> }
>
> @@ -268,9 +269,9 @@ c_fc(char **wp)
> shf_close(shf);
> *xp = '\0';
> strip_nuls(Xstring(xs, xp), Xlength(xs, xp));
> - depth++;
> + fc_depth++;
> ret = hist_execute(Xstring(xs, xp));
> - depth--;
> + fc_depth--;
> return ret;
> }
> }
> Index: main.c
> ===================================================================
> RCS file: /cvs/src/bin/ksh/main.c,v
> retrieving revision 1.93
> diff -u -p -r1.93 main.c
> --- main.c 29 Sep 2018 14:13:19 -0000 1.93
> +++ main.c 29 Oct 2018 08:19:12 -0000
> @@ -549,6 +549,7 @@ shell(Source *volatile s, volatile int t
> case LERROR:
> case LSHELL:
> if (interactive) {
> + fc_depth = 0;
> if (i == LINTR)
> shellf("\n");
> /* Reset any eof that was read as part of a
> Index: sh.h
> ===================================================================
> RCS file: /cvs/src/bin/ksh/sh.h,v
> retrieving revision 1.73
> diff -u -p -r1.73 sh.h
> --- sh.h 18 May 2018 13:25:20 -0000 1.73
> +++ sh.h 29 Oct 2018 08:19:12 -0000
> @@ -249,6 +249,7 @@ extern volatile sig_atomic_t intrsig; /*
> extern volatile sig_atomic_t fatal_trap; /* received a fatal
> signal */
> extern volatile sig_atomic_t got_sigwinch;
> extern Trap sigtraps[NSIG+1];
> +extern volatile sig_atomic_t fc_depth; /* c_fc depth counter */
>
> /*
> * TMOUT support
