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

[Zbigniew Jędrzejewski-Szmek]

On Sun, Jan 25, 2015 at 03:37:09AM +0100, Tomasz Pawlak wrote:
> 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

Actually it *is* protected, see kill(2). Signals are ignored for PID 1
unless it installed handlers for them. Nevertheless, we probably want to
abort on SIGSEGV and similar and not continue, so we shouldn't ever run
without the handlers installed.

We shouldn't really ever fail to install the handlers, so this is a
rather academic exercise. I guess we can add an assert_se() around it.

> 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.

That's a good point.

> 
> 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.

Your analysis sounds correct here too.

A patch would be welcome.

Zbyszek