ksh: fc -s and SIGINT

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

ksh: fc -s and SIGINT

Anton Lindqvist-2
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 -0000 1.80
+++ history.c 28 Oct 2018 19:10:54 -0000
@@ -36,6 +36,8 @@ static char   **hist_get(const char *, i
 static char   **hist_get_oldest(void);
 static void histbackup(void);
 
+static void hist_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 int ignorespace; /* ditch lines s
 static Source *hist_source;
 static uint32_t line_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(&sigtraps[SIGINT], hist_sigint, SS_ONESHOT);
+
  while ((optc = ksh_getopt(wp, &builtin_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.h 18 May 2018 13:25:20 -0000 1.73
+++ sh.h 28 Oct 2018 19:10:54 -0000
@@ -230,6 +230,7 @@ typedef struct trap {
 #define TF_TTY_INTR BIT(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_MASK 0x3 /* 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_USER BIT(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 -0000 1.32
+++ trap.c 28 Oct 2018 19:10:54 -0000
@@ -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 = &sigtraps[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 = sigtraps, i = NSIG+1; --i >= 0; p++) {
+ (void)traphandler(p); /* clear one shot handler */
+
  if (p->set && (!flag ||
     ((p->flags & flag) && p->trap == NULL)))
  runtrap(p);
+ }
 }
 
 void
@@ -354,6 +361,14 @@ setsig(Trap *p, sig_t f, int flags)
  if (p->signal == SIGEXIT_ || p->signal == SIGERR_)
  return 1;
 
+ if ((flags & SS_ONESHOT) == SS_ONESHOT) {
+ if (p->shtrap != NULL)
+ return 0;
+ p->flags |= TF_ONESHOT;
+ p->shtrap = f;
+ return 1;
+ }
+
  /* First time setting this signal?  If so, get and note the current
  * setting.
  */
@@ -420,4 +435,21 @@ setexecsig(Trap *p, int restore)
  p->flags |= TF_EXEC_IGN;
  break;
  }
+}
+
+/*
+ * Returns the signal handler associated with the given trap.
+ * If the handler is registered as a one shot, it's disassociated with the trap.
+ */
+static sig_t
+traphandler(Trap *p)
+{
+ sig_t f;
+
+ f = p->shtrap;
+ if (p->flags & TF_ONESHOT) {
+ p->flags &= ~TF_ONESHOT;
+ p->shtrap = NULL;
+ }
+ return f;
 }

Reply | Threaded
Open this post in threaded view
|

Re: ksh: fc -s and SIGINT

Martijn van Duren-5
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 -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

Reply | Threaded
Open this post in threaded view
|

Re: ksh: fc -s and SIGINT

Anton Lindqvist-2
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

Reply | Threaded
Open this post in threaded view
|

Re: ksh: fc -s and SIGINT

Martijn van Duren-5
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 *);

Reply | Threaded
Open this post in threaded view
|

Re: ksh: fc -s and SIGINT

Jeremie Courreges-Anglas-2
On Mon, Nov 12 2018, Martijn van Duren <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: ksh: fc -s and SIGINT

Theo Buehler-5
> 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