"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.
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 -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. */
