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@.

diff --git a/usr.bin/vi/cl/cl.h b/usr.bin/vi/cl/cl.h
index 5e5cdcb..bfef0dc 100644 (file)
--- a/usr.bin/vi/cl/cl.h
+++ b/usr.bin/vi/cl/cl.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: cl.h,v 1.10 2016/05/27 09:18:11 martijn Exp $ */
+/*     $OpenBSD: cl.h,v 1.11 2021/09/01 14:28:15 schwarze Exp $        */
 
 /*-
  * Copyright (c) 1993, 1994
@@ -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;
 

diff --git a/usr.bin/vi/cl/cl_funcs.c b/usr.bin/vi/cl/cl_funcs.c
index 4daa75e..eeb6046 100644 (file)
--- a/usr.bin/vi/cl/cl_funcs.c
+++ b/usr.bin/vi/cl/cl_funcs.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: cl_funcs.c,v 1.20 2016/05/27 09:18:11 martijn Exp $   */
+/*     $OpenBSD: cl_funcs.c,v 1.21 2021/09/01 14:28:15 schwarze Exp $  */
 
 /*-
  * Copyright (c) 1993, 1994
@@ -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);
 }

diff --git a/usr.bin/vi/cl/cl_main.c b/usr.bin/vi/cl/cl_main.c
index 38a1020..4cd624e 100644 (file)
--- a/usr.bin/vi/cl/cl_main.c
+++ b/usr.bin/vi/cl/cl_main.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: cl_main.c,v 1.33 2016/05/05 20:36:41 martijn Exp $    */
+/*     $OpenBSD: cl_main.c,v 1.34 2021/09/01 14:28:15 schwarze Exp $   */
 
 /*-
  * Copyright (c) 1993, 1994
@@ -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
 
@@ -260,6 +261,11 @@ 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, &clp->oact[INDX_HUP], h_hup) ||
                    setsig(SIGINT, &clp->oact[INDX_INT], h_int) ||

diff --git a/usr.bin/vi/cl/cl_read.c b/usr.bin/vi/cl/cl_read.c
index 3644684..aa3e1ba 100644 (file)
--- a/usr.bin/vi/cl/cl_read.c
+++ b/usr.bin/vi/cl/cl_read.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: cl_read.c,v 1.21 2016/05/27 09:18:11 martijn Exp $    */
+/*     $OpenBSD: cl_read.c,v 1.22 2021/09/01 14:28:15 schwarze Exp $   */
 
 /*-
  * Copyright (c) 1993, 1994
@@ -54,34 +54,32 @@ cl_event(SCR *sp, EVENT *evp, u_int32_t flags, int ms)
         * 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. */