svn commit: r345696 - head/lib/libvgl

[Konstantin Belousov]

On Sat, Mar 30, 2019 at 03:24:40PM +1100, Bruce Evans wrote:
> On Fri, 29 Mar 2019, Konstantin Belousov wrote:
> 
> > On Fri, Mar 29, 2019 at 03:57:09PM +0000, Bruce Evans wrote:
> >> Author: bde
> >> Date: Fri Mar 29 15:57:08 2019
> >> New Revision: 345696
> >> URL: https://svnweb.freebsd.org/changeset/base/345696
> >>
> >> Log:
> >>   Fix endless loops for handling SIGBUS and SIGSEGV.
> >>
> >>   r80270 has the usual wrong fix for unsafe signal handling -- just set
> >>   a flag and return to let an event loop check the flag and do safe
> >>   handling.  This never works for signals like SIGBUS and SIGSEGV that
> >>   repeat and works poorly for others unless the application has an event
> >>   loop designed to support this.
> >>
> >>   For these signals, clean up unsafely as before, except for arranging that
> >>   nested signals are fatal and forcing a nested signal if the cleanup doesn't
> >>   cause one.
> >>
> >> Modified:
> >>   head/lib/libvgl/main.c
> >>
> >> Modified: head/lib/libvgl/main.c
> >> ==============================================================================
> >> --- head/lib/libvgl/main.c	Fri Mar 29 15:20:48 2019	(r345695)
> >> +++ head/lib/libvgl/main.c	Fri Mar 29 15:57:08 2019	(r345696)
> >> ...
> >> @@ -107,14 +107,22 @@ struct vt_mode smode;
> >>  }
> >>
> >>  static void
> >> -VGLAbort(int arg __unused)
> >> +VGLAbort(int arg)
> >>  {
> >> +  sigset_t mask;
> >> +
> >>    VGLAbortPending = 1;
> >>    signal(SIGINT, SIG_IGN);
> >>    signal(SIGTERM, SIG_IGN);
> >> -  signal(SIGSEGV, SIG_IGN);
> >> -  signal(SIGBUS, SIG_IGN);
> >>    signal(SIGUSR2, SIG_IGN);
> >> +  if (arg == SIGBUS || arg == SIGSEGV) {
> >> +    signal(arg, SIG_DFL);
> >> +    sigemptyset(&mask);
> >> +    sigaddset(&mask, arg);
> >> +    sigprocmask(SIG_UNBLOCK, &mask, NULL);
> >> +    VGLEnd();
> >> +    kill(getpid(), arg);
> > This of course misses the siginfo information from the real fault.
> 
> It is in the nested signal frame.
> 
> > Why SIGBUS/SIGSEGV are caught at all ?
> 
> Otherwise, the screen is left in an application mode that the kernel
> doesn't support (except to not write to the screen, so that the
> application has full control).
> 
> Of course, it is bad for libraries to catch signals.  The man page warns
> about installing signal handlers and trying to call VGLEnd() and exit(3)
> [from the signal handler] to end the program, although it is important
> to use VGLEnd() to end the program.  I haven't tried installing signal
> handlers in programs, but my main application is another graphics library
> that wraps libvgl, and it installs an atexit() handler which calls its
> cleanup function which calls VGLEnd().  libvgl should do the same.  The
> cleanup is safe in exit() provided calling exit() is safe.
> 
> The man page has general nonsense recommending setting a flag and not
> terminating the program in signal handlers.  This was added in the same
> commit that completely broke the SIGBUS and SIGSEGV handling using this
> method.  A much longer lesson is needed to describe how to use this
> method correctly.  SA_RESTART must be turned off for all signals, and
> this gives the complexity of handling EINTR returns from all syscalls
> affected by SA_RESTART ...
> 
> The man page has almost enough details about which signals the library
> catches.  It doesn't mention SIGUSR1 (used for screen switches) or
> SIGUSR2 (used for mouse signals).  I plan to add use of SIGIO to fix
> busy-waiting for keyboard events.  select() might work for the keyboard,
> but I doubt that it works for the mouse.
> 
> What should happen is if the application installs signal handlers and
> does unsafe things not including calling VGLEnd(), then nested SIGBUS's
> and SIGSEGV's should reach the above cleanup.  Applications should not
> catch SIGBUS or SIGSEGV unless they can clean up better than the above.
> It is possible to clean up better than the above, but not in applications.
> The above can check a soft signal mask flag like the one set by INTOFF()
> and not do so much when it is set.  When it is not set, I think restoring
> the screen mode is safe in signal handlers, and only things like free()
> and printf() are not safe in signal handlers.

I am more about not doing kill(2) from the handler, instead do plain
return to the context which caused the original signal.