Re: ksh: fc -s and SIGINT
> 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@ I concur. I'm slightly annoyed by the asymmetry between c_fc_depth++ vs c_fc_reset(), but I think we've had enough bikeshedding on this patch. Put it in. ok
Re: ksh: fc -s and SIGINT
On Mon, Nov 12 2018, Martijn van Duren 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 - 1.80
> +++ history.c 12 Nov 2018 15:46:35 -
> @@ -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.c29 Sep 2018 14:13:19 - 1.93
> +++ main.c12 Nov 2018 15:46:35 -
> @@ -549,6 +549,7 @@ shell(Source *volatile s, volatile int t
> case LERROR:
> case LSHELL:
> if (interactive) {
> + c_fc_reset();
>
Re: ksh: fc -s and SIGINT
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.
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 - 1.80
+++ history.c 12 Nov 2018 15:46:35 -
@@ -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 - 1.93
+++ main.c 12 Nov 2018 15:46:35 -
@@ -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.h18
Re: ksh: fc -s and SIGINT
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 - 1.80
> +++ history.c 29 Oct 2018 08:19:12 -
> @@ -48,6 +48,8 @@ static uint32_t line_co;
>
> static struct stat last_sb;
>
> +volatile sig_atomic_tfc_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.c29 Sep 2018 14:13:19 - 1.93
> +++ main.c29 Oct 2018 08:19:12 -
> @@ -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 - 1.73
> +++ sh.h 29 Oct 2018 08:19:12 -
> @@ -249,6 +249,7 @@ externvolatile sig_atomic_t intrsig; /*
> extern volatile sig_atomic_t fatal_trap; /* received a fatal
> signal */
> extern volatile sig_atomic_t got_sigwinch;
> extern Trapsigtraps[NSIG+1];
> +extern volatile sig_atomic_t fc_depth; /* c_fc depth counter */
>
> /*
> *
Re: ksh: fc -s and SIGINT
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.
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 - 1.80
+++ history.c 29 Oct 2018 08:19:12 -
@@ -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 - 1.93
+++ main.c 29 Oct 2018 08:19:12 -
@@ -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.h18 May 2018 13:25:20 - 1.73
+++ sh.h29 Oct 2018 08:19:12 -
@@ -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 Trapsigtraps[NSIG+1];
+extern volatile sig_atomic_t fc_depth; /* c_fc depth counter */
/*
* TMOUT support
ksh: fc -s and SIGINT
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?
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 - 1.80
+++ history.c 28 Oct 2018 19:10:54 -
@@ -36,6 +36,8 @@ static char **hist_get(const char *, i
static char **hist_get_oldest(void);
static voidhistbackup(void);
+static voidhist_sigint(int);
+
static FILE*histfh;
static char **histbase; /* actual start of the history[] allocation */
static char **current; /* current position in history[] */
@@ -46,6 +48,8 @@ static intignorespace;/* ditch lines s
static Source *hist_source;
static uint32_tline_co;
+static volatile sig_atomic_t fc_depth;
+
static struct stat last_sb;
int
@@ -58,9 +62,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;
}
@@ -70,6 +73,8 @@ c_fc(char **wp)
return 1;
}
+ setsig([SIGINT], hist_sigint, SS_ONESHOT);
+
while ((optc = ksh_getopt(wp, _opt,
"e:glnrs0,1,2,3,4,5,6,7,8,9,")) != -1)
switch (optc) {
@@ -146,9 +151,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 +273,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;
}
}
@@ -852,4 +857,10 @@ void
hist_finish(void)
{
history_close();
+}
+
+static void
+hist_sigint(int signo)
+{
+ fc_depth = 0;
}
Index: sh.h
===
RCS file: /cvs/src/bin/ksh/sh.h,v
retrieving revision 1.73
diff -u -p -r1.73 sh.h
--- sh.h18 May 2018 13:25:20 - 1.73
+++ sh.h28 Oct 2018 19:10:54 -
@@ -230,6 +230,7 @@ typedef struct trap {
#define TF_TTY_INTRBIT(7) /* tty generated signal (see j_waitj) */
#define TF_CHANGED BIT(8) /* used by runtrap() to detect trap changes */
#define TF_FATAL BIT(9) /* causes termination if not trapped */
+#define TF_ONESHOT BIT(10) /* temporary trap handler registered */
/* values for setsig()/setexecsig() flags argument */
#define SS_RESTORE_MASK0x3 /* how to restore a signal before an
exec() */
@@ -240,6 +241,7 @@ typedef struct trap {
#define SS_FORCE BIT(3) /* set signal even if original signal ignored */
#define SS_USERBIT(4) /* user is doing the set (ie, trap
command) */
#define SS_SHTRAP BIT(5) /* trap for internal use (CHLD,ALRM,WINCH) */
+#define SS_ONESHOT BIT(6) /* register temporary trap handler */
#define SIGEXIT_ 0 /* for trap EXIT */
#define SIGERR_NSIG/* for trap ERR */
Index: trap.c
===
RCS file: /cvs/src/bin/ksh/trap.c,v
retrieving revision 1.32
diff -u -p -r1.32 trap.c
--- trap.c 15 Mar 2018 16:51:29 - 1.32
+++ trap.c 28 Oct 2018 19:10:54 -
@@ -13,6 +13,8 @@
Trap sigtraps[NSIG + 1];
+static sig_t traphandler(Trap *);
+
static struct sigaction Sigact_ign, Sigact_trap;
void
@@ -117,6 +119,7 @@ void
trapsig(int i)
{
Trap *p = [i];
+ sig_t f;
int errno_ = errno;
trap = p->set = 1;
@@ -126,8 +129,9 @@ trapsig(int i)
fatal_trap = 1;
intrsig = 1;
}
- if (p->shtrap)
- (*p->shtrap)(i);
+ f = traphandler(p);
+ if (f)
+ (*f)(i);
errno = errno_;
}
@@ -200,10 +204,13 @@ runtraps(int flag)
intrsig = 0;
if (flag & TF_FATAL)
fatal_trap = 0;
- for (p = sigtraps, i = NSIG+1; --i >= 0; p++)
+ for (p =
