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 <tro...@kagu-tsuchi.com> wrote:
>
> > "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. */
