Signal handling: what you can't do.

[Geoff Keating]

I just found this on gnupg-devel, to which I'm not subscribed.  I
don't remember replying to it, so...

Werner Koch wrote:
> Hi Geoff,
> 
> First, many thanks for your review.  I still have some questions:
> 
> Geoff Keating <geoffk at ozemail.com.au> writes:
> 
> > static RETSIGTYPE
> > got_fatal_signal( int sig )
> > {
> >     if( caught_fatal_sig )
> >         raise( sig );
> >     caught_fatal_sig = 1;
> > 
> >     fprintf( stderr, "\n%s: %s caught ... exiting\n",
> >               log_get_name(), signal_name(sig) );
> >     secmem_term();
> >     exit( 8 );
> > }
> > 
> > now, this is not good.  There are three issues here, but most flow
> > from the same important rule:
> > 
> > In a signal handler, under Linux (and most other unixes), you cannot:
> > 
> > 1. Assume that the values in non-volatile variables are stored in the same
> >    order as implied by the code (or in fact, that they are ever stored
> >    at all);
>
> But caught_fatal_sig is declared volatile; so where is the problem
> here?

The variables that secmem_term uses are not declared volatile.

> > 2. Call non-reentrant procedures.
> 
> I know that using printf is not good style but the only problem I can
> see, is that the output is garbled or you get an SEGV.

You may write to some important memory, which can be used like a
buffer overflow to gain root access (eg. a write to the PLT entry for
exit() ).

> I tried to use write(2) directly here but for some reasons
> (portability?) I gave up on this.  I assume that the correct solution
> would be to register an atexit function which displays such a message
> if a fatal signal occured.

You can't call 'exit', either.  You must call _exit.

Probably the right thing to do here is to just call raise(sig) again,
after clearing secure memory; eg.

static RETSIGTYPE
got_fatal_signal( int sig )
{
    if( ! caught_fatal_sig )
    {
      caught_fatal_sig = 1;
      secmem_term();
    }
    raise( sig );
}

> > Also for instance, in secmem_init, there is this code:
> > 
> >     else {
> >         pool_is_mmapped = 1;
> >         pool_okay = 1;
> >     }
> > 
> > there is absolutely no guarantee that `pool_is_mmapped' is physically
> > set to 1 before `pool_ok', or that `pool_ok' is set to 1 after the
> 
> Okay. I make pool_okay volatile.

You need to make both volatile.  If you have pool_okay volatile, the
will still move the pool_is_mmapped write after the change to
pool_okay.  You also need to make any other variable that
secmem_term may access volatile.

For instance, in the loop

extern volatile int foo;
extern int i;

void set_foo(void)
{
  for (i = 0; i < 10; i++)
    foo = 1;
}

you will get (assuming the optimization is still working) only one
write to 'i', at the end of the routine, but 10 writes to 'foo'.

> > Finally, a minor nit:  in signal_name(), there is this:
> > 
> >   #if SYS_SIGLIST_DECLARED
> >     return sys_siglist[signum];
> >   #else
> >     static char buf[20];
> >     sprintf(buf, "signal %d", signum );
> >     return buf;
> >   #endif
> > 
> > it should check that `signum' is less than NSIG, or better yet it
> > should use strsignal() which does the check for you and will still
> 
> Which is not available on BSD systems

This is why the GNU project has autoconf :-).

Better yet, remove signal_name altogether as I suggest above.

-- 
Geoffrey Keating <geoffk at cygnus.com>