From: schwarze <schwarze@openbsd.org>
Date: Wed, 1 Sep 2021 14:28:15 +0000 (+0000)
Subject: As a first step towards safe signal handling, improve the h_int()
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=3e13b57bacccf623c8b2fab1e5dd87af6026466b;p=openbsd

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 5e5cdcb614c..bfef0dc225a 100644
--- 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 4daa75e4360..eeb6046358f 100644
--- 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 38a10209f9e..4cd624e72bf 100644
--- 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 3644684b65f..aa3e1ba0dd4 100644
--- 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. */