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 -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 *);
