Hi Tim,
trondd wrote on Tue, Aug 24, 2021 at 07:45:33PM -0400:
> "Theo de Raadt" <dera...@openbsd.org> 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.org 2021/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 <trondd at kagu hyphen tsuchi dot com> 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 -0000 1.10
> +++ cl/cl.h 24 Aug 2021 23:34:27 -0000
> @@ -11,6 +11,11 @@
> * @(#)cl.h 10.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_OK 0x0004 /* 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_TTY 0x0200 /* Talking to a terminal. */
> +#define CL_STDIN_TTY 0x0020 /* 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 -0000 1.20
> +++ cl/cl_funcs.c 24 Aug 2021 23:34:27 -0000
> @@ -601,7 +601,7 @@ cl_suspend(SCR *sp, int *allowedp)
> if (cl_ssize(sp, 1, NULL, NULL, &changed))
> 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 -0000 1.33
> +++ cl/cl_main.c 24 Aug 2021 23:34:27 -0000
> @@ -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, &clp->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 -0000 1.21
> +++ cl/cl_read.c 24 Aug 2021 23:34:27 -0000
> @@ -54,34 +54,32 @@ cl_event(SCR *sp, EVENT *evp, u_int32_t
> * so that we just keep returning them until the editor dies.
> */
> clp = CLP(sp);
> -retest: if (LF_ISSET(EC_INTERRUPT) || F_ISSET(clp, CL_SIGINT)) {
> - if (F_ISSET(clp, CL_SIGINT)) {
> - F_CLR(clp, CL_SIGINT);
> +retest: if (LF_ISSET(EC_INTERRUPT) || cl_sigint) {
> + if (cl_sigint) {
> + cl_sigint = 0;
> evp->e_event = E_INTERRUPT;
> } else
> evp->e_event = E_TIMEOUT;
> return (0);
> }
> - if (F_ISSET(clp, CL_SIGHUP | CL_SIGTERM | CL_SIGWINCH)) {
> - if (F_ISSET(clp, CL_SIGHUP)) {
> - evp->e_event = E_SIGHUP;
> - return (0);
> - }
> - if (F_ISSET(clp, CL_SIGTERM)) {
> - evp->e_event = E_SIGTERM;
> + if (cl_sighup) {
> + evp->e_event = E_SIGHUP;
> + return (0);
> + }
> + if (cl_sigterm) {
> + evp->e_event = E_SIGTERM;
> + return (0);
> + }
> + if (cl_sigwinch) {
> + cl_sigwinch = 0;
> + if (cl_ssize(sp, 1, &lines, &columns, &changed))
> + return (1);
> + if (changed) {
> + (void)cl_resize(sp, lines, columns);
> + evp->e_event = E_WRESIZE;
> return (0);
> }
> - if (F_ISSET(clp, CL_SIGWINCH)) {
> - F_CLR(clp, CL_SIGWINCH);
> - if (cl_ssize(sp, 1, &lines, &columns, &changed))
> - return (1);
> - if (changed) {
> - (void)cl_resize(sp, lines, columns);
> - evp->e_event = E_WRESIZE;
> - return (0);
> - }
> - /* No real change, ignore the signal. */
> - }
> + /* No real change, ignore the signal. */
> }
>
> /* Set timer. */
