On Mon, Nov 12 2018, Martijn van Duren <openbsd+t...@list.imperialat.at> wrote:
> On 10/29/18 12:01 PM, Anton Lindqvist wrote:
>> 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@
>>>
> After some back and forth with anton@ we came up with the following
> diff. I would like to have some extra scrutiny on this one before
> actually committing this.
Even if this looks more like an ad-hoc solution, I find it easier to
grasp and review than the trap-based diff. Works fine here and seems to
solve miod's problem.
ok jca@
> 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 12 Nov 2018 15:46:35 -0000
> @@ -48,6 +48,8 @@ static uint32_t line_co;
>
> static struct stat last_sb;
>
> +static volatile sig_atomic_t c_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 (c_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++;
> + c_fc_depth++;
> ret = hist_replace(hp, pat, rep, gflag);
> - depth--;
> + c_fc_reset();
> return ret;
> }
>
> @@ -268,11 +269,20 @@ c_fc(char **wp)
> shf_close(shf);
> *xp = '\0';
> strip_nuls(Xstring(xs, xp), Xlength(xs, xp));
> - depth++;
> + c_fc_depth++;
> ret = hist_execute(Xstring(xs, xp));
> - depth--;
> + c_fc_reset();
> return ret;
> }
> +}
> +
> +/* Reset the c_fc depth counter.
> + * Made available for when an fc call is interrupted.
> + */
> +void
> +c_fc_reset(void)
> +{
> + c_fc_depth = 0;
> }
>
> /* Save cmd in history, execute cmd (cmd gets trashed) */
> 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 12 Nov 2018 15:46:35 -0000
> @@ -549,6 +549,7 @@ shell(Source *volatile s, volatile int t
> case LERROR:
> case LSHELL:
> if (interactive) {
> + c_fc_reset();
> 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 12 Nov 2018 15:46:35 -0000
> @@ -449,6 +449,7 @@ void hist_init(Source *);
> void hist_finish(void);
> void histsave(int, const char *, int);
> int c_fc(char **);
> +void c_fc_reset(void);
> void sethistcontrol(const char *);
> void sethistsize(int);
> void sethistfile(const char *);
>
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
