Re: Atomic signal flags for vi(1)
On Thu, 2021-09-02 at 13:25 +0200, Ingo Schwarze wrote:
> Hi Tim,
>
> trondd wrote on Wed, Sep 01, 2021 at 08:46:28PM -0400:
> > Ingo Schwarze wrote:
> > > Ingo Schwarze wrote on Wed, Sep 01, 2021 at 04:38:51PM +0200:
>
> > > > Note that the h_hup() and h_term() signal handlers are still unsafe
> > > > after this commit because they also set the "killersig" (how fitting!)
> > > > field in a global struct.
>
> > > I like it when fixing two bugs only amounts to minus: minus 14 LOC,
> > > 1 function, 1 global variable, 2 automatic variables, 1 struct member,
> > > 9 pointer dereferences, 1 #define, and minus 1 #undef.
> > >
> > > The argument that only one single GS exists applies unchanged.
>
> > Great. This is the direction I was going to go in with it. I hadn't
> > thought of collapsing h_hup an h_term, though.
> >
> > This makes sense as I understand the signal/event handling and a quick
> > test through the signals shows no difference in behavior.
>
> Thank you (and martijn@) for reviewing and testing.
> This is now committed.
>
> > Should h_term() and cl_sigterm be named something more general to
> > indicate that they also handle SIGHUP? Or is it good enough since it
> > includes all the signals that 'term'inate vi? It's not hard to follow
> > as-is.
>
> That is what i was thinking: why re-paint a bike shed if the existing
> paint is already pretty and not yet falling off?
Luckily the paint is pretty. :-)
>
> Yours,
> Ingo
>
>
> > > Index: cl/cl.h
> > > ===
> > > RCS file: /cvs/src/usr.bin/vi/cl/cl.h,v
> > > retrieving revision 1.11
> > > diff -u -p -r1.11 cl.h
> > > --- cl/cl.h 1 Sep 2021 14:28:15 - 1.11
> > > +++ cl/cl.h 1 Sep 2021 17:15:34 -
> > > @@ -11,7 +11,6 @@
> > > * @(#)cl.h10.19 (Berkeley) 9/24/96
> > > */
> > >
> > > -extern volatile sig_atomic_t cl_sighup;
> > > extern volatile sig_atomic_t cl_sigint;
> > > extern volatile sig_atomic_t cl_sigterm;
> > > extern volatile sig_atomic_t cl_sigwinch;
> > > @@ -31,7 +30,6 @@ typedef struct _cl_private {
> > > char*rmso, *smso; /* Inverse video terminal strings. */
> > > char*smcup, *rmcup; /* Terminal start/stop strings. */
> > >
> > > - int killersig; /* Killer signal. */
> > > #define INDX_HUP0
> > > #define INDX_INT1
> > > #define INDX_TERM 2
> > > Index: cl/cl_funcs.c
> > > ===
> > > RCS file: /cvs/src/usr.bin/vi/cl/cl_funcs.c,v
> > > retrieving revision 1.21
> > > diff -u -p -r1.21 cl_funcs.c
> > > --- cl/cl_funcs.c 1 Sep 2021 14:28:15 - 1.21
> > > +++ cl/cl_funcs.c 1 Sep 2021 17:15:34 -
> > > @@ -437,7 +437,7 @@ cl_refresh(SCR *sp, int repaint)
> > >* If we received a killer signal, we're done, there's no point
> > >* in refreshing the screen.
> > >*/
> > > - if (clp->killersig)
> > > + if (cl_sigterm)
> > > return (0);
> > >
> > > /*
> > > @@ -582,7 +582,7 @@ cl_suspend(SCR *sp, int *allowedp)
> > >* unchanged. In addition, the terminal has already been reset
> > >* correctly, so leave it alone.
> > >*/
> > > - if (clp->killersig) {
> > > + if (cl_sigterm) {
> > > F_CLR(clp, CL_SCR_EX_INIT | CL_SCR_VI_INIT);
> > > return (0);
> > > }
> > > Index: cl/cl_main.c
> > > ===
> > > RCS file: /cvs/src/usr.bin/vi/cl/cl_main.c,v
> > > retrieving revision 1.34
> > > diff -u -p -r1.34 cl_main.c
> > > --- cl/cl_main.c 1 Sep 2021 14:28:15 - 1.34
> > > +++ cl/cl_main.c 1 Sep 2021 17:15:34 -
> > > @@ -33,7 +33,6 @@
> > >
> > > GS *__global_list; /* GLOBAL: List of
> > > screens. */
> > >
> > > -volatile sig_atomic_t cl_sighup;
> > > volatile sig_atomic_t cl_sigint;
> > > volatile sig_atomic_t cl_sigterm;
> > > volatile sig_atomic_t cl_sigwinch;
> > > @@ -120,9 +119,9 @@ main(int argc, char *argv[])
> > > }
> > >
> > > /* If a killer signal arrived, pretend we just got it. */
> > > - if (clp->killersig) {
> > > - (void)signal(clp->killersig, SIG_DFL);
> > > - (void)kill(getpid(), clp->killersig);
> > > + if (cl_sigterm) {
> > > + (void)signal(cl_sigterm, SIG_DFL);
> > > + (void)kill(getpid(), cl_sigterm);
> > > /* NOTREACHED */
> > > }
> > >
> > > @@ -215,17 +214,6 @@ term_init(char *ttype)
> > > }
> > > }
> > >
> > > -#define GLOBAL_CLP \
> > > - CL_PRIVATE *clp = GCLP(__global_list);
> > > -static void
> > > -h_hup(int signo)
> > > -{
> > > - GLOBAL_CLP;
> > > -
> > > - cl_sighup = 1;
> > > - clp->killersig = SIGHUP;
> > > -}
> > > -
> > > static void
> > > h_int(int signo)
> > > {
> > > @@ -235,10 +223,7 @@ h_int(int signo)
> > > static void
> > > h_term(int signo)
> > > {
> > > - GLOBAL_CLP;
> > > -
> > > - cl_sigterm = 1;
> > > -
Re: Atomic signal flags for vi(1)
Hi Tim,
trondd wrote on Wed, Sep 01, 2021 at 08:46:28PM -0400:
> Ingo Schwarze wrote:
>> Ingo Schwarze wrote on Wed, Sep 01, 2021 at 04:38:51PM +0200:
>>> Note that the h_hup() and h_term() signal handlers are still unsafe
>>> after this commit because they also set the "killersig" (how fitting!)
>>> field in a global struct.
>> I like it when fixing two bugs only amounts to minus: minus 14 LOC,
>> 1 function, 1 global variable, 2 automatic variables, 1 struct member,
>> 9 pointer dereferences, 1 #define, and minus 1 #undef.
>>
>> The argument that only one single GS exists applies unchanged.
> Great. This is the direction I was going to go in with it. I hadn't
> thought of collapsing h_hup an h_term, though.
>
> This makes sense as I understand the signal/event handling and a quick
> test through the signals shows no difference in behavior.
Thank you (and martijn@) for reviewing and testing.
This is now committed.
> Should h_term() and cl_sigterm be named something more general to
> indicate that they also handle SIGHUP? Or is it good enough since it
> includes all the signals that 'term'inate vi? It's not hard to follow
> as-is.
That is what i was thinking: why re-paint a bike shed if the existing
paint is already pretty and not yet falling off?
Yours,
Ingo
>> Index: cl/cl.h
>> ===
>> RCS file: /cvs/src/usr.bin/vi/cl/cl.h,v
>> retrieving revision 1.11
>> diff -u -p -r1.11 cl.h
>> --- cl/cl.h 1 Sep 2021 14:28:15 - 1.11
>> +++ cl/cl.h 1 Sep 2021 17:15:34 -
>> @@ -11,7 +11,6 @@
>> * @(#)cl.h10.19 (Berkeley) 9/24/96
>> */
>>
>> -extern volatile sig_atomic_t cl_sighup;
>> extern volatile sig_atomic_t cl_sigint;
>> extern volatile sig_atomic_t cl_sigterm;
>> extern volatile sig_atomic_t cl_sigwinch;
>> @@ -31,7 +30,6 @@ typedef struct _cl_private {
>> char*rmso, *smso; /* Inverse video terminal strings. */
>> char*smcup, *rmcup; /* Terminal start/stop strings. */
>>
>> -int killersig; /* Killer signal. */
>> #define INDX_HUP0
>> #define INDX_INT1
>> #define INDX_TERM 2
>> Index: cl/cl_funcs.c
>> ===
>> RCS file: /cvs/src/usr.bin/vi/cl/cl_funcs.c,v
>> retrieving revision 1.21
>> diff -u -p -r1.21 cl_funcs.c
>> --- cl/cl_funcs.c1 Sep 2021 14:28:15 - 1.21
>> +++ cl/cl_funcs.c1 Sep 2021 17:15:34 -
>> @@ -437,7 +437,7 @@ cl_refresh(SCR *sp, int repaint)
>> * If we received a killer signal, we're done, there's no point
>> * in refreshing the screen.
>> */
>> -if (clp->killersig)
>> +if (cl_sigterm)
>> return (0);
>>
>> /*
>> @@ -582,7 +582,7 @@ cl_suspend(SCR *sp, int *allowedp)
>> * unchanged. In addition, the terminal has already been reset
>> * correctly, so leave it alone.
>> */
>> -if (clp->killersig) {
>> +if (cl_sigterm) {
>> F_CLR(clp, CL_SCR_EX_INIT | CL_SCR_VI_INIT);
>> return (0);
>> }
>> Index: cl/cl_main.c
>> ===
>> RCS file: /cvs/src/usr.bin/vi/cl/cl_main.c,v
>> retrieving revision 1.34
>> diff -u -p -r1.34 cl_main.c
>> --- cl/cl_main.c 1 Sep 2021 14:28:15 - 1.34
>> +++ cl/cl_main.c 1 Sep 2021 17:15:34 -
>> @@ -33,7 +33,6 @@
>>
>> GS *__global_list; /* GLOBAL: List of screens. */
>>
>> -volatile sig_atomic_t cl_sighup;
>> volatile sig_atomic_t cl_sigint;
>> volatile sig_atomic_t cl_sigterm;
>> volatile sig_atomic_t cl_sigwinch;
>> @@ -120,9 +119,9 @@ main(int argc, char *argv[])
>> }
>>
>> /* If a killer signal arrived, pretend we just got it. */
>> -if (clp->killersig) {
>> -(void)signal(clp->killersig, SIG_DFL);
>> -(void)kill(getpid(), clp->killersig);
>> +if (cl_sigterm) {
>> +(void)signal(cl_sigterm, SIG_DFL);
>> +(void)kill(getpid(), cl_sigterm);
>> /* NOTREACHED */
>> }
>>
>> @@ -215,17 +214,6 @@ term_init(char *ttype)
>> }
>> }
>>
>> -#define GLOBAL_CLP \
>> -CL_PRIVATE *clp = GCLP(__global_list);
>> -static void
>> -h_hup(int signo)
>> -{
>> -GLOBAL_CLP;
>> -
>> -cl_sighup = 1;
>> -clp->killersig = SIGHUP;
>> -}
>> -
>> static void
>> h_int(int signo)
>> {
>> @@ -235,10 +223,7 @@ h_int(int signo)
>> static void
>> h_term(int signo)
>> {
>> -GLOBAL_CLP;
>> -
>> -cl_sigterm = 1;
>> -clp->killersig = SIGTERM;
>> +cl_sigterm = signo;
>> }
>>
>> static void
>> @@ -246,7 +231,6 @@ h_winch(int signo)
>> {
>> cl_sigwinch = 1;
>> }
>> -#undef GLOBAL_CLP
>>
>> /*
>> * sig_init --
>> @@ -261,20 +245,19 @@ sig_init(GS *gp, SCR *sp)
>>
>> clp = GCLP(gp);
>>
>> -cl_sighup = 0;
>> cl_sigint = 0;
>> cl_sigterm = 0;
>>
Re: Atomic signal flags for vi(1)
Ingo Schwarze wrote:
> Hi,
>
> Ingo Schwarze wrote on Wed, Sep 01, 2021 at 04:38:51PM +0200:
>
> > Note that the h_hup() and h_term() signal handlers are still unsafe
> > after this commit because they also set the "killersig" (how fitting!)
> > field in a global struct.
>
> I like it when fixing two bugs only amounts to minus: minus 14 LOC,
> 1 function, 1 global variable, 2 automatic variables, 1 struct member,
> 9 pointer dereferences, 1 #define, and minus 1 #undef.
>
> The argument that only one single GS exists applies unchanged.
>
> OK?
> Ingo
Great. This is the direction I was going to go in with it. I hadn't
thought of collapsing h_hup an h_term, though.
This makes sense as I understand the signal/event handling and a quick
test through the signals shows no difference in behavior.
Should h_term() and cl_sigterm be named something more general to
indicate that they also handle SIGHUP? Or is it good enough since it
includes all the signals that 'term'inate vi? It's not hard to follow
as-is.
Tim. (not @ :P )
>
>
> Index: cl/cl.h
> ===
> RCS file: /cvs/src/usr.bin/vi/cl/cl.h,v
> retrieving revision 1.11
> diff -u -p -r1.11 cl.h
> --- cl/cl.h 1 Sep 2021 14:28:15 - 1.11
> +++ cl/cl.h 1 Sep 2021 17:15:34 -
> @@ -11,7 +11,6 @@
> * @(#)cl.h10.19 (Berkeley) 9/24/96
> */
>
> -extern volatile sig_atomic_t cl_sighup;
> extern volatile sig_atomic_t cl_sigint;
> extern volatile sig_atomic_t cl_sigterm;
> extern volatile sig_atomic_t cl_sigwinch;
> @@ -31,7 +30,6 @@ typedef struct _cl_private {
> char*rmso, *smso; /* Inverse video terminal strings. */
> char*smcup, *rmcup; /* Terminal start/stop strings. */
>
> - int killersig; /* Killer signal. */
> #define INDX_HUP0
> #define INDX_INT1
> #define INDX_TERM 2
> Index: cl/cl_funcs.c
> ===
> RCS file: /cvs/src/usr.bin/vi/cl/cl_funcs.c,v
> retrieving revision 1.21
> diff -u -p -r1.21 cl_funcs.c
> --- cl/cl_funcs.c 1 Sep 2021 14:28:15 - 1.21
> +++ cl/cl_funcs.c 1 Sep 2021 17:15:34 -
> @@ -437,7 +437,7 @@ cl_refresh(SCR *sp, int repaint)
>* If we received a killer signal, we're done, there's no point
>* in refreshing the screen.
>*/
> - if (clp->killersig)
> + if (cl_sigterm)
> return (0);
>
> /*
> @@ -582,7 +582,7 @@ cl_suspend(SCR *sp, int *allowedp)
>* unchanged. In addition, the terminal has already been reset
>* correctly, so leave it alone.
>*/
> - if (clp->killersig) {
> + if (cl_sigterm) {
> F_CLR(clp, CL_SCR_EX_INIT | CL_SCR_VI_INIT);
> return (0);
> }
> Index: cl/cl_main.c
> ===
> RCS file: /cvs/src/usr.bin/vi/cl/cl_main.c,v
> retrieving revision 1.34
> diff -u -p -r1.34 cl_main.c
> --- cl/cl_main.c 1 Sep 2021 14:28:15 - 1.34
> +++ cl/cl_main.c 1 Sep 2021 17:15:34 -
> @@ -33,7 +33,6 @@
>
> GS *__global_list; /* GLOBAL: List of screens. */
>
> -volatile sig_atomic_t cl_sighup;
> volatile sig_atomic_t cl_sigint;
> volatile sig_atomic_t cl_sigterm;
> volatile sig_atomic_t cl_sigwinch;
> @@ -120,9 +119,9 @@ main(int argc, char *argv[])
> }
>
> /* If a killer signal arrived, pretend we just got it. */
> - if (clp->killersig) {
> - (void)signal(clp->killersig, SIG_DFL);
> - (void)kill(getpid(), clp->killersig);
> + if (cl_sigterm) {
> + (void)signal(cl_sigterm, SIG_DFL);
> + (void)kill(getpid(), cl_sigterm);
> /* NOTREACHED */
> }
>
> @@ -215,17 +214,6 @@ term_init(char *ttype)
> }
> }
>
> -#define GLOBAL_CLP \
> - CL_PRIVATE *clp = GCLP(__global_list);
> -static void
> -h_hup(int signo)
> -{
> - GLOBAL_CLP;
> -
> - cl_sighup = 1;
> - clp->killersig = SIGHUP;
> -}
> -
> static void
> h_int(int signo)
> {
> @@ -235,10 +223,7 @@ h_int(int signo)
> static void
> h_term(int signo)
> {
> - GLOBAL_CLP;
> -
> - cl_sigterm = 1;
> - clp->killersig = SIGTERM;
> + cl_sigterm = signo;
> }
>
> static void
> @@ -246,7 +231,6 @@ h_winch(int signo)
> {
> cl_sigwinch = 1;
> }
> -#undef GLOBAL_CLP
>
> /*
> * sig_init --
> @@ -261,20 +245,19 @@ sig_init(GS *gp, SCR *sp)
>
> clp = GCLP(gp);
>
> - cl_sighup = 0;
> cl_sigint = 0;
> cl_sigterm = 0;
> cl_sigwinch = 0;
>
> if (sp == NULL) {
> - if (setsig(SIGHUP, >oact[INDX_HUP], h_hup) ||
> + if (setsig(SIGHUP, >oact[INDX_HUP], h_term) ||
> setsig(SIGINT, >oact[INDX_INT], h_int) ||
> setsig(SIGTERM, >oact[INDX_TERM], h_term) ||
>
Re: Atomic signal flags for vi(1)
Hi,
Ingo Schwarze wrote on Wed, Sep 01, 2021 at 04:38:51PM +0200:
> Note that the h_hup() and h_term() signal handlers are still unsafe
> after this commit because they also set the "killersig" (how fitting!)
> field in a global struct.
I like it when fixing two bugs only amounts to minus: minus 14 LOC,
1 function, 1 global variable, 2 automatic variables, 1 struct member,
9 pointer dereferences, 1 #define, and minus 1 #undef.
The argument that only one single GS exists applies unchanged.
OK?
Ingo
Index: cl/cl.h
===
RCS file: /cvs/src/usr.bin/vi/cl/cl.h,v
retrieving revision 1.11
diff -u -p -r1.11 cl.h
--- cl/cl.h 1 Sep 2021 14:28:15 - 1.11
+++ cl/cl.h 1 Sep 2021 17:15:34 -
@@ -11,7 +11,6 @@
* @(#)cl.h10.19 (Berkeley) 9/24/96
*/
-extern volatile sig_atomic_t cl_sighup;
extern volatile sig_atomic_t cl_sigint;
extern volatile sig_atomic_t cl_sigterm;
extern volatile sig_atomic_t cl_sigwinch;
@@ -31,7 +30,6 @@ typedef struct _cl_private {
char*rmso, *smso; /* Inverse video terminal strings. */
char*smcup, *rmcup; /* Terminal start/stop strings. */
- int killersig; /* Killer signal. */
#defineINDX_HUP0
#defineINDX_INT1
#defineINDX_TERM 2
Index: cl/cl_funcs.c
===
RCS file: /cvs/src/usr.bin/vi/cl/cl_funcs.c,v
retrieving revision 1.21
diff -u -p -r1.21 cl_funcs.c
--- cl/cl_funcs.c 1 Sep 2021 14:28:15 - 1.21
+++ cl/cl_funcs.c 1 Sep 2021 17:15:34 -
@@ -437,7 +437,7 @@ cl_refresh(SCR *sp, int repaint)
* If we received a killer signal, we're done, there's no point
* in refreshing the screen.
*/
- if (clp->killersig)
+ if (cl_sigterm)
return (0);
/*
@@ -582,7 +582,7 @@ cl_suspend(SCR *sp, int *allowedp)
* unchanged. In addition, the terminal has already been reset
* correctly, so leave it alone.
*/
- if (clp->killersig) {
+ if (cl_sigterm) {
F_CLR(clp, CL_SCR_EX_INIT | CL_SCR_VI_INIT);
return (0);
}
Index: cl/cl_main.c
===
RCS file: /cvs/src/usr.bin/vi/cl/cl_main.c,v
retrieving revision 1.34
diff -u -p -r1.34 cl_main.c
--- cl/cl_main.c1 Sep 2021 14:28:15 - 1.34
+++ cl/cl_main.c1 Sep 2021 17:15:34 -
@@ -33,7 +33,6 @@
GS *__global_list; /* GLOBAL: List of screens. */
-volatile sig_atomic_t cl_sighup;
volatile sig_atomic_t cl_sigint;
volatile sig_atomic_t cl_sigterm;
volatile sig_atomic_t cl_sigwinch;
@@ -120,9 +119,9 @@ main(int argc, char *argv[])
}
/* If a killer signal arrived, pretend we just got it. */
- if (clp->killersig) {
- (void)signal(clp->killersig, SIG_DFL);
- (void)kill(getpid(), clp->killersig);
+ if (cl_sigterm) {
+ (void)signal(cl_sigterm, SIG_DFL);
+ (void)kill(getpid(), cl_sigterm);
/* NOTREACHED */
}
@@ -215,17 +214,6 @@ term_init(char *ttype)
}
}
-#defineGLOBAL_CLP \
- CL_PRIVATE *clp = GCLP(__global_list);
-static void
-h_hup(int signo)
-{
- GLOBAL_CLP;
-
- cl_sighup = 1;
- clp->killersig = SIGHUP;
-}
-
static void
h_int(int signo)
{
@@ -235,10 +223,7 @@ h_int(int signo)
static void
h_term(int signo)
{
- GLOBAL_CLP;
-
- cl_sigterm = 1;
- clp->killersig = SIGTERM;
+ cl_sigterm = signo;
}
static void
@@ -246,7 +231,6 @@ h_winch(int signo)
{
cl_sigwinch = 1;
}
-#undef GLOBAL_CLP
/*
* sig_init --
@@ -261,20 +245,19 @@ sig_init(GS *gp, SCR *sp)
clp = GCLP(gp);
- cl_sighup = 0;
cl_sigint = 0;
cl_sigterm = 0;
cl_sigwinch = 0;
if (sp == NULL) {
- if (setsig(SIGHUP, >oact[INDX_HUP], h_hup) ||
+ if (setsig(SIGHUP, >oact[INDX_HUP], h_term) ||
setsig(SIGINT, >oact[INDX_INT], h_int) ||
setsig(SIGTERM, >oact[INDX_TERM], h_term) ||
setsig(SIGWINCH, >oact[INDX_WINCH], h_winch)
)
err(1, NULL);
} else
- if (setsig(SIGHUP, NULL, h_hup) ||
+ if (setsig(SIGHUP, NULL, h_term) ||
setsig(SIGINT, NULL, h_int) ||
setsig(SIGTERM, NULL, h_term) ||
setsig(SIGWINCH, NULL, h_winch)
Index: cl/cl_read.c
===
RCS file: /cvs/src/usr.bin/vi/cl/cl_read.c,v
retrieving revision 1.22
diff -u -p -r1.22 cl_read.c
--- cl/cl_read.c1 Sep 2021 14:28:15 - 1.22
+++ cl/cl_read.c1 Sep 2021 17:15:34 -
@@ -62,13 +62,15
Re: Atomic signal flags for vi(1)
Hi Tim,
trondd wrote on Tue, Aug 24, 2021 at 07:45:33PM -0400:
> "Theo de Raadt" wrote:
>> +h_alrm(int signo)
>> +{
>> + GLOBAL_CLP;
>> +
>> + F_SET(clp, CL_SIGALRM);
>>
>> F_SET is |=, which is not atomic.
>>
>> This is unsafe. Safe signal handlers need to make single stores to
>> atomic-sized variables, which tend to be int-sized, easier to declare
>> as sig_atomic_t. Most often these are global scope with the following
>> type:
>>
>> volatile sig_atomic_t variable
>>
>> I can see you copied an existing practice. Sadly all the other
>> signal handlers are also broken in the same way.
>>
>> The flag bits should be replaced with seperate sig_atomic_t variables.
> Ok. Took a swing at converting the signal handling to sig_atomic_t flags.
> No additional changes added other than removing a redundant check of the
> flags in cl_read.c. Seemed to be a pretty straight-forward conversion and
> I haven't found any change in behavior.
Committed, thank you.
Ingo
CVSROOT:/cvs
Module name:src
Changes by: schwa...@cvs.openbsd.org2021/09/01 08:28:15
Modified files:
usr.bin/vi/cl : cl.h cl_funcs.c cl_main.c cl_read.c
Log message:
As a first step towards safe signal handling, improve the h_int()
and h_winch() signal handlers to make one single store to a
sig_atomic_t variable. Note that the h_hup() and h_term() signal
handlers are still unsafe after this commit because they also set
the "killersig" (how fitting!) field in a global struct.
Despite storing information in static global variables rather than
in structs passed around as arguments, this patch does not cause a
change in behaviour because there is always exactly one GS object,
initialized using gs_init() called from the top of main(), and
screen_init() stores a pointer to this one and only GS object in
the .gp member of each and every SCR object. Talk about useless
abstraction...
Problem pointed out by deraadt@.
Patch from Tim on tech@.
OK deraadt@.
> Index: cl/cl.h
> ===
> RCS file: /cvs/src/usr.bin/vi/cl/cl.h,v
> retrieving revision 1.10
> diff -u -p -r1.10 cl.h
> --- cl/cl.h 27 May 2016 09:18:11 - 1.10
> +++ cl/cl.h 24 Aug 2021 23:34:27 -
> @@ -11,6 +11,11 @@
> * @(#)cl.h10.19 (Berkeley) 9/24/96
> */
>
> +extern volatile sig_atomic_t cl_sighup;
> +extern volatile sig_atomic_t cl_sigint;
> +extern volatile sig_atomic_t cl_sigterm;
> +extern volatile sig_atomic_t cl_sigwinch;
> +
> typedef struct _cl_private {
> CHAR_T ibuf[256]; /* Input keys. */
>
> @@ -45,11 +50,7 @@ typedef struct _cl_private {
> #define CL_RENAME_OK0x0004 /* User wants the windows renamed. */
> #define CL_SCR_EX_INIT 0x0008 /* Ex screen initialized. */
> #define CL_SCR_VI_INIT 0x0010 /* Vi screen initialized. */
> -#define CL_SIGHUP 0x0020 /* SIGHUP arrived. */
> -#define CL_SIGINT 0x0040 /* SIGINT arrived. */
> -#define CL_SIGTERM 0x0080 /* SIGTERM arrived. */
> -#define CL_SIGWINCH 0x0100 /* SIGWINCH arrived. */
> -#define CL_STDIN_TTY0x0200 /* Talking to a terminal. */
> +#define CL_STDIN_TTY0x0020 /* Talking to a terminal. */
> u_int32_t flags;
> } CL_PRIVATE;
>
> Index: cl/cl_funcs.c
> ===
> RCS file: /cvs/src/usr.bin/vi/cl/cl_funcs.c,v
> retrieving revision 1.20
> diff -u -p -r1.20 cl_funcs.c
> --- cl/cl_funcs.c 27 May 2016 09:18:11 - 1.20
> +++ cl/cl_funcs.c 24 Aug 2021 23:34:27 -
> @@ -601,7 +601,7 @@ cl_suspend(SCR *sp, int *allowedp)
> if (cl_ssize(sp, 1, NULL, NULL, ))
> return (1);
> if (changed)
> - F_SET(CLP(sp), CL_SIGWINCH);
> + cl_sigwinch = 1;
>
> return (0);
> }
> Index: cl/cl_main.c
> ===
> RCS file: /cvs/src/usr.bin/vi/cl/cl_main.c,v
> retrieving revision 1.33
> diff -u -p -r1.33 cl_main.c
> --- cl/cl_main.c 5 May 2016 20:36:41 - 1.33
> +++ cl/cl_main.c 24 Aug 2021 23:34:27 -
> @@ -33,6 +33,11 @@
>
> GS *__global_list; /* GLOBAL: List of screens. */
>
> +volatile sig_atomic_t cl_sighup;
> +volatile sig_atomic_t cl_sigint;
> +volatile sig_atomic_t cl_sigterm;
> +volatile sig_atomic_t cl_sigwinch;
> +
> static void cl_func_std(GS *);
> static CL_PRIVATE *cl_init(GS *);
> static GS *gs_init(void);
> @@ -217,16 +222,14 @@ h_hup(int signo)
> {
> GLOBAL_CLP;
>
> - F_SET(clp, CL_SIGHUP);
> + cl_sighup = 1;
> clp->killersig = SIGHUP;
> }
>
> static void
> h_int(int signo)
> {
> - GLOBAL_CLP;
> -
> - F_SET(clp, CL_SIGINT);
> + cl_sigint = 1;
> }
>
> static void
> @@ -234,16 +237,14 @@ h_term(int signo)
> {
> GLOBAL_CLP;
>
> - F_SET(clp,
Re: Atomic signal flags for vi(1)
Hi Theo, Theo de Raadt wrote on Sun, Aug 29, 2021 at 11:38:18AM -0600: > Ingo Schwarze wrote: >> *If* more than one GS object ever existed and/or the .gp pointers >> in different SCR objects could point to different GS objects, this >> patch might change behaviour. > If such multiple GS condition ever existed, since signals are (global), > the handler is only indicating a signal has happened. And the code > observing non-zero of these signal-indicating variables would be > responsible to determine which event it applies to. Absolutely, that is the sane way to code such stuff. However, we have all seen signal handlers making complicated decisions by navigating complex, non-trivial data structures, even in our own tree. Of course, those need to be fixed. But before changing the code, i still need to figure out whether the seemingly non-trivial data structure is indeed non-trivial, and which higher-level code must be made smarter to preserve the functionality, or whether the apparent complexity merely hides the fact that the data is actually trivial, as i think it is in this case. > It is too hard to do this kind of work safely/atomically in a > signal handler. 100% correct. And yet, people do try to, and then it even works. Well, kind of. Most of the time. Until it suddenly doesn't. And when it suddenly doesn't, people are used to blaming cosmic muons (and yes, there is indeed about one of those per square centimeter per minute, on average) rather than thinking something might be wrong with their code. Yours, Ingo
Re: Atomic signal flags for vi(1)
> *If* more than one GS object ever existed and/or the .gp pointers > in different SCR objects could point to different GS objects, this > patch might change behaviour. If such multiple GS condition ever existed, since signals are (global), the handler is only indicating a signal has happened. And the code observing non-zero of these signal-indicating variables would be responsible to determine which event it applies to. It is too hard to do this kind of work safely/atomically in a signal handler.
Re: Atomic signal flags for vi(1)
Hi,
Theo de Raadt wrote on Sun, Aug 29, 2021 at 02:54:57AM -0600:
> This does look better.
>
> I appreciate that you are fixing this underlying problem first, before
> overlaying your timer diff.
Indeed.
> Is this working for the vi crowd?
*If* more than one GS object ever existed and/or the .gp pointers
in different SCR objects could point to different GS objects, this
patch might change behaviour. So the hardest part about veryfying
it is to make sure that only one single GS object exists at any
given time and all .gp pointers in all SCR objects always point to
that one GS object. GS and [.>]gs are used at hundreds of places,
but i only managed to find a single place where GS is instantiated
(gs_init() in cl/cl_main.c, which is called inly once at the very
beginning of main()), and only a single place where the .gp member
of SCR is ever changed, in screen_init() in common/screen.c, which
is called from a handful of places, but always with a pointer to
that one particular GS object.
So yes, i do think changing members of GS to static globals is safe
and testing with a single screen is sufficient. After the patch,
* SIGHUP prints "Hangup" and exits as expected;
* SIGINT prints "Interrupted" in the status line and exits insert
mode, but stays in vi(1);
* SIGTERM prints "Terminated" and exits as expected;
* window size changes work as expected both in the forground and
in the background while suspended with Ctrl-Z, and sending
SIGWINCH manually has no effect, as expected.
Note that the patch is incomplete. The signal handler functions
h_hup() and h_term() still aren't safe because they still write to
GCLP(__global_list)->killersig. But this patch performs one
well-defined step and is hard enough to audit already, so let's get
it committed, then take care of killersig in a second step.
So if someone wants to commit, this patch is OK schwarze@.
Alternatively, provide an OK and tell me to commit.
Be careful that the original author of this patch signed as "Tim"
but is not tim@ for all i can tell.
Yours,
Ingo
> trondd wrote:
>
> > "Theo de Raadt" wrote:
> >
> > > +h_alrm(int signo)
> > > +{
> > > + GLOBAL_CLP;
> > > +
> > > + F_SET(clp, CL_SIGALRM);
> > >
> > > F_SET is |=, which is not atomic.
> > >
> > > This is unsafe. Safe signal handlers need to make single stores to
> > > atomic-sized variables, which tend to be int-sized, easier to declare
> > > as sig_atomic_t. Most often these are global scope with the following
> > > type:
> > >
> > > volatile sig_atomic_t variable
> > >
> > > I can see you copied an existing practice. Sadly all the other
> > > signal handlers are also broken in the same way.
> > >
> > > The flag bits should be replaced with seperate sig_atomic_t variables.
> >
> > Ok. Took a swing at converting the signal handling to sig_atomic_t flags.
> > No additional changes added other than removing a redundant check of the
> > flags in cl_read.c. Seemed to be a pretty straight-forward conversion and
> > I haven't found any change in behavior.
> >
> > Tim.
> >
> >
> > Index: cl/cl.h
> > ===
> > RCS file: /cvs/src/usr.bin/vi/cl/cl.h,v
> > retrieving revision 1.10
> > diff -u -p -r1.10 cl.h
> > --- cl/cl.h 27 May 2016 09:18:11 - 1.10
> > +++ cl/cl.h 24 Aug 2021 23:34:27 -
> > @@ -11,6 +11,11 @@
> > * @(#)cl.h10.19 (Berkeley) 9/24/96
> > */
> >
> > +extern volatile sig_atomic_t cl_sighup;
> > +extern volatile sig_atomic_t cl_sigint;
> > +extern volatile sig_atomic_t cl_sigterm;
> > +extern volatile sig_atomic_t cl_sigwinch;
> > +
> > typedef struct _cl_private {
> > CHAR_T ibuf[256]; /* Input keys. */
> >
> > @@ -45,11 +50,7 @@ typedef struct _cl_private {
> > #defineCL_RENAME_OK0x0004 /* User wants the windows renamed. */
> > #defineCL_SCR_EX_INIT 0x0008 /* Ex screen initialized. */
> > #defineCL_SCR_VI_INIT 0x0010 /* Vi screen initialized. */
> > -#defineCL_SIGHUP 0x0020 /* SIGHUP arrived. */
> > -#defineCL_SIGINT 0x0040 /* SIGINT arrived. */
> > -#defineCL_SIGTERM 0x0080 /* SIGTERM arrived. */
> > -#defineCL_SIGWINCH 0x0100 /* SIGWINCH arrived. */
> > -#defineCL_STDIN_TTY0x0200 /* Talking to a terminal. */
> > +#defineCL_STDIN_TTY0x0020 /* Talking to a terminal. */
> > u_int32_t flags;
> > } CL_PRIVATE;
> >
> > Index: cl/cl_funcs.c
> > ===
> > RCS file: /cvs/src/usr.bin/vi/cl/cl_funcs.c,v
> > retrieving revision 1.20
> > diff -u -p -r1.20 cl_funcs.c
> > --- cl/cl_funcs.c 27 May 2016 09:18:11 - 1.20
> > +++ cl/cl_funcs.c 24 Aug 2021 23:34:27 -
> > @@ -601,7 +601,7 @@ cl_suspend(SCR *sp, int *allowedp)
> > if (cl_ssize(sp, 1, NULL, NULL, ))
> > return (1);
> > if (changed)
> > - F_SET(CLP(sp), CL_SIGWINCH);
> > +
Re: Atomic signal flags for vi(1)
I'll see if I can fit this one in in the next few days.
Feel free to remind me :-)
martijn@
On Sun, 2021-08-29 at 02:54 -0600, Theo de Raadt wrote:
> This does look better.
>
> I appreciate that you are fixing this underlying problem first, before
> overlaying your timer diff.
>
> Is this working for the vi crowd?
>
>
> trondd wrote:
>
> > "Theo de Raadt" wrote:
> >
> > > +h_alrm(int signo)
> > > +{
> > > + GLOBAL_CLP;
> > > +
> > > + F_SET(clp, CL_SIGALRM);
> > >
> > > F_SET is |=, which is not atomic.
> > >
> > > This is unsafe. Safe signal handlers need to make single stores to
> > > atomic-sized variables, which tend to be int-sized, easier to declare
> > > as sig_atomic_t. Most often these are global scope with the following
> > > type:
> > >
> > > volatile sig_atomic_t variable
> > >
> > > I can see you copied an existing practice. Sadly all the other
> > > signal handlers are also broken in the same way.
> > >
> > > The flag bits should be replaced with seperate sig_atomic_t variables.
> >
> > Ok. Took a swing at converting the signal handling to sig_atomic_t flags.
> > No additional changes added other than removing a redundant check of the
> > flags in cl_read.c. Seemed to be a pretty straight-forward conversion and
> > I haven't found any change in behavior.
> >
> > Tim.
> >
> >
> > Index: cl/cl.h
> > ===
> > RCS file: /cvs/src/usr.bin/vi/cl/cl.h,v
> > retrieving revision 1.10
> > diff -u -p -r1.10 cl.h
> > --- cl/cl.h 27 May 2016 09:18:11 - 1.10
> > +++ cl/cl.h 24 Aug 2021 23:34:27 -
> > @@ -11,6 +11,11 @@
> > * @(#)cl.h10.19 (Berkeley) 9/24/96
> > */
> >
> > +extern volatile sig_atomic_t cl_sighup;
> > +extern volatile sig_atomic_t cl_sigint;
> > +extern volatile sig_atomic_t cl_sigterm;
> > +extern volatile sig_atomic_t cl_sigwinch;
> > +
> > typedef struct _cl_private {
> > CHAR_T ibuf[256]; /* Input keys. */
> >
> > @@ -45,11 +50,7 @@ typedef struct _cl_private {
> > #defineCL_RENAME_OK0x0004 /* User wants the windows renamed. */
> > #defineCL_SCR_EX_INIT 0x0008 /* Ex screen initialized. */
> > #defineCL_SCR_VI_INIT 0x0010 /* Vi screen initialized. */
> > -#defineCL_SIGHUP 0x0020 /* SIGHUP arrived. */
> > -#defineCL_SIGINT 0x0040 /* SIGINT arrived. */
> > -#defineCL_SIGTERM 0x0080 /* SIGTERM arrived. */
> > -#defineCL_SIGWINCH 0x0100 /* SIGWINCH arrived. */
> > -#defineCL_STDIN_TTY0x0200 /* Talking to a terminal. */
> > +#defineCL_STDIN_TTY0x0020 /* Talking to a terminal. */
> > u_int32_t flags;
> > } CL_PRIVATE;
> >
> > Index: cl/cl_funcs.c
> > ===
> > RCS file: /cvs/src/usr.bin/vi/cl/cl_funcs.c,v
> > retrieving revision 1.20
> > diff -u -p -r1.20 cl_funcs.c
> > --- cl/cl_funcs.c 27 May 2016 09:18:11 - 1.20
> > +++ cl/cl_funcs.c 24 Aug 2021 23:34:27 -
> > @@ -601,7 +601,7 @@ cl_suspend(SCR *sp, int *allowedp)
> > if (cl_ssize(sp, 1, NULL, NULL, ))
> > return (1);
> > if (changed)
> > - F_SET(CLP(sp), CL_SIGWINCH);
> > + cl_sigwinch = 1;
> >
> > return (0);
> > }
> > Index: cl/cl_main.c
> > ===
> > RCS file: /cvs/src/usr.bin/vi/cl/cl_main.c,v
> > retrieving revision 1.33
> > diff -u -p -r1.33 cl_main.c
> > --- cl/cl_main.c5 May 2016 20:36:41 - 1.33
> > +++ cl/cl_main.c24 Aug 2021 23:34:27 -
> > @@ -33,6 +33,11 @@
> >
> > GS *__global_list; /* GLOBAL: List of screens. */
> >
> > +volatile sig_atomic_t cl_sighup;
> > +volatile sig_atomic_t cl_sigint;
> > +volatile sig_atomic_t cl_sigterm;
> > +volatile sig_atomic_t cl_sigwinch;
> > +
> > static void cl_func_std(GS *);
> > static CL_PRIVATE *cl_init(GS *);
> > static GS*gs_init(void);
> > @@ -217,16 +222,14 @@ h_hup(int signo)
> > {
> > GLOBAL_CLP;
> >
> > - F_SET(clp, CL_SIGHUP);
> > + cl_sighup = 1;
> > clp->killersig = SIGHUP;
> > }
> >
> > static void
> > h_int(int signo)
> > {
> > - GLOBAL_CLP;
> > -
> > - F_SET(clp, CL_SIGINT);
> > + cl_sigint = 1;
> > }
> >
> > static void
> > @@ -234,16 +237,14 @@ h_term(int signo)
> > {
> > GLOBAL_CLP;
> >
> > - F_SET(clp, CL_SIGTERM);
> > + cl_sigterm = 1;
> > clp->killersig = SIGTERM;
> > }
> >
> > static void
> > h_winch(int signo)
> > {
> > - GLOBAL_CLP;
> > -
> > - F_SET(clp, CL_SIGWINCH);
> > + cl_sigwinch = 1;
> > }
> > #undef GLOBAL_CLP
> >
> > @@ -259,6 +260,11 @@ sig_init(GS *gp, SCR *sp)
> > CL_PRIVATE *clp;
> >
> > clp = GCLP(gp);
> > +
> > + cl_sighup = 0;
> > + cl_sigint = 0;
> > + cl_sigterm = 0;
> > + cl_sigwinch = 0;
> >
> > if (sp == NULL) {
> > if (setsig(SIGHUP, >oact[INDX_HUP],
Re: Atomic signal flags for vi(1)
This does look better.
I appreciate that you are fixing this underlying problem first, before
overlaying your timer diff.
Is this working for the vi crowd?
trondd wrote:
> "Theo de Raadt" wrote:
>
> > +h_alrm(int signo)
> > +{
> > + GLOBAL_CLP;
> > +
> > + F_SET(clp, CL_SIGALRM);
> >
> > F_SET is |=, which is not atomic.
> >
> > This is unsafe. Safe signal handlers need to make single stores to
> > atomic-sized variables, which tend to be int-sized, easier to declare
> > as sig_atomic_t. Most often these are global scope with the following
> > type:
> >
> > volatile sig_atomic_t variable
> >
> > I can see you copied an existing practice. Sadly all the other
> > signal handlers are also broken in the same way.
> >
> > The flag bits should be replaced with seperate sig_atomic_t variables.
>
> Ok. Took a swing at converting the signal handling to sig_atomic_t flags.
> No additional changes added other than removing a redundant check of the
> flags in cl_read.c. Seemed to be a pretty straight-forward conversion and
> I haven't found any change in behavior.
>
> Tim.
>
>
> Index: cl/cl.h
> ===
> RCS file: /cvs/src/usr.bin/vi/cl/cl.h,v
> retrieving revision 1.10
> diff -u -p -r1.10 cl.h
> --- cl/cl.h 27 May 2016 09:18:11 - 1.10
> +++ cl/cl.h 24 Aug 2021 23:34:27 -
> @@ -11,6 +11,11 @@
> * @(#)cl.h10.19 (Berkeley) 9/24/96
> */
>
> +extern volatile sig_atomic_t cl_sighup;
> +extern volatile sig_atomic_t cl_sigint;
> +extern volatile sig_atomic_t cl_sigterm;
> +extern volatile sig_atomic_t cl_sigwinch;
> +
> typedef struct _cl_private {
> CHAR_T ibuf[256]; /* Input keys. */
>
> @@ -45,11 +50,7 @@ typedef struct _cl_private {
> #define CL_RENAME_OK0x0004 /* User wants the windows renamed. */
> #define CL_SCR_EX_INIT 0x0008 /* Ex screen initialized. */
> #define CL_SCR_VI_INIT 0x0010 /* Vi screen initialized. */
> -#define CL_SIGHUP 0x0020 /* SIGHUP arrived. */
> -#define CL_SIGINT 0x0040 /* SIGINT arrived. */
> -#define CL_SIGTERM 0x0080 /* SIGTERM arrived. */
> -#define CL_SIGWINCH 0x0100 /* SIGWINCH arrived. */
> -#define CL_STDIN_TTY0x0200 /* Talking to a terminal. */
> +#define CL_STDIN_TTY0x0020 /* Talking to a terminal. */
> u_int32_t flags;
> } CL_PRIVATE;
>
> Index: cl/cl_funcs.c
> ===
> RCS file: /cvs/src/usr.bin/vi/cl/cl_funcs.c,v
> retrieving revision 1.20
> diff -u -p -r1.20 cl_funcs.c
> --- cl/cl_funcs.c 27 May 2016 09:18:11 - 1.20
> +++ cl/cl_funcs.c 24 Aug 2021 23:34:27 -
> @@ -601,7 +601,7 @@ cl_suspend(SCR *sp, int *allowedp)
> if (cl_ssize(sp, 1, NULL, NULL, ))
> return (1);
> if (changed)
> - F_SET(CLP(sp), CL_SIGWINCH);
> + cl_sigwinch = 1;
>
> return (0);
> }
> Index: cl/cl_main.c
> ===
> RCS file: /cvs/src/usr.bin/vi/cl/cl_main.c,v
> retrieving revision 1.33
> diff -u -p -r1.33 cl_main.c
> --- cl/cl_main.c 5 May 2016 20:36:41 - 1.33
> +++ cl/cl_main.c 24 Aug 2021 23:34:27 -
> @@ -33,6 +33,11 @@
>
> GS *__global_list; /* GLOBAL: List of screens. */
>
> +volatile sig_atomic_t cl_sighup;
> +volatile sig_atomic_t cl_sigint;
> +volatile sig_atomic_t cl_sigterm;
> +volatile sig_atomic_t cl_sigwinch;
> +
> static void cl_func_std(GS *);
> static CL_PRIVATE *cl_init(GS *);
> static GS *gs_init(void);
> @@ -217,16 +222,14 @@ h_hup(int signo)
> {
> GLOBAL_CLP;
>
> - F_SET(clp, CL_SIGHUP);
> + cl_sighup = 1;
> clp->killersig = SIGHUP;
> }
>
> static void
> h_int(int signo)
> {
> - GLOBAL_CLP;
> -
> - F_SET(clp, CL_SIGINT);
> + cl_sigint = 1;
> }
>
> static void
> @@ -234,16 +237,14 @@ h_term(int signo)
> {
> GLOBAL_CLP;
>
> - F_SET(clp, CL_SIGTERM);
> + cl_sigterm = 1;
> clp->killersig = SIGTERM;
> }
>
> static void
> h_winch(int signo)
> {
> - GLOBAL_CLP;
> -
> - F_SET(clp, CL_SIGWINCH);
> + cl_sigwinch = 1;
> }
> #undef GLOBAL_CLP
>
> @@ -259,6 +260,11 @@ sig_init(GS *gp, SCR *sp)
> CL_PRIVATE *clp;
>
> clp = GCLP(gp);
> +
> + cl_sighup = 0;
> + cl_sigint = 0;
> + cl_sigterm = 0;
> + cl_sigwinch = 0;
>
> if (sp == NULL) {
> if (setsig(SIGHUP, >oact[INDX_HUP], h_hup) ||
> Index: cl/cl_read.c
> ===
> RCS file: /cvs/src/usr.bin/vi/cl/cl_read.c,v
> retrieving revision 1.21
> diff -u -p -r1.21 cl_read.c
> --- cl/cl_read.c 27 May 2016 09:18:11 - 1.21
> +++ cl/cl_read.c 24 Aug 2021 23:34:27 -
> @@ -54,34 +54,32 @@ cl_event(SCR *sp, EVENT
