[systemd-devel] Odp: Re: BUG: several bugs in core/main.c (v218)

[Tomasz Pawlak]

Dnia Niedziela, 25 Stycznia 2015 07:45 Andrei Borzenkov <arvidjaar at gmail.com> napisał(a) 
> &#x412; Sun, 25 Jan 2015 03:37:09 +0100
> "Tomasz Pawlak" <tomazzi at wp.pl> &#x43F;&#x438;&#x448;&#x435;&#x442;:
> 
> > core/main.c:1519
> >       /* Make sure we leave a core dump without panicing the
> >        * kernel. */
> >       if (getpid() == 1) {
> >               install_crash_handler();
> > 
> >               r = mount_cgroup_controllers(arg_join_controllers);
> >               if (r < 0)
> >                       goto finish;
> >       }
> > 
> > core/main.c:226
> > static void install_crash_handler(void) {
> >       struct sigaction sa = {
> >               .sa_handler = crash,
> >               .sa_flags = SA_NODEFER,
> >       };
> > 
> >       sigaction_many(&sa, SIGNALS_CRASH_HANDLER, -1);
> > }
> > 
> > /shared/util.c:2207
> > int sigaction_many(const struct sigaction *sa, ...) {
> >       va_list ap;
> >       int r = 0, sig;
> > 
> >       va_start(ap, sa);
> >       while ((sig = va_arg(ap, int)) > 0)
> >               if (sigaction(sig, sa, NULL) < 0)
> >                       r = -errno;
> >       va_end(ap);
> > 
> >       return r;
> > }
> > 
> > shared/def.h:40
> > #define SIGNALS_CRASH_HANDLER SIGSEGV,SIGILL,SIGFPE,SIGBUS,SIGQUIT,SIGABRT
> > 
> > 
> > BUGS:
> > 1. Ignoring return value from sigaction_many(): all sig handlers can silently fail to install, thus leaving the whole process unprotected
> 
> What do you suggest to do in this case? We are running as PID1, we
> cannot just error exit. 
> 

You are right, but it's not as simple as it may look at first sight:

1. If we allow the process to continue without sig handlers installed, then results can be just catastrophic: kernel panic with all the services launched -> broken transanctions, half-written records/files, etc -> total mess, corrupted or lost data etc.
So, since successfull installation of the sig handlers is one of the most critical parts of initialisation, it is actualy safer to just quit. This is just a critical fault (and is currently completely ignored).

2. Another thing is, that those signals are not equivalently important, f.e. SIGABRT can be throwed by thousants lines of code in this project (by abort()), so it is much more likely that assertion checking will prevent segfaults, throwing SIGABRT instead. This means that SIGABRT is actually far more probable than SIGSEGV.
This in turn leads to simple solution: the process should unconditionally exit if hander for SIGABRT have failed to install, but with other sig handlers failed, we may take a risk and continue.
In any case, such situation should be logged as soon as possible.
Ignoring this is just asking for catastrophe.

3. SIGFPE: how often the code uses FPU? -> I mean, that handler for this sig can be dynamically installed/unistalled when needed, probably only on a thread level, not for the whole process. This will allow to completely safely report failed sigaction by assertion checking. 

4. So, sigaction_many() should be removed (also because it is a vararg function, what is rather bad idea), and a function for registering one sig handler at a time should be used. Then, we can tell (log) which signals were not registered by sigaction, and take conscious decision what to do  next.

Regards.

> > 
> > 2. SIGSEGV should be catched by a handler running on a separate stack (SA_ONSTACK) - otherwise it can cause segfault itself, when the first SIGSEGV which triggered the handler is caused by stack overflow.
> > 
> > 3. SA_NODEFER makes no sense, since the handler is expected to catch only first critical signal. With SA_NODEFER nesting of signals is possible, what can cause unpredictable results, including uncatched stack overflow caused by the handler itself.
> > 
> > Regards.
> > 
> > 
> > _______________________________________________
> > systemd-devel mailing list
> > systemd-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/systemd-devel