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

[Tomasz Pawlak]

On Sun, 25.01.15 03:37, Tomasz Pawlak (tomazzi at wp.pl) wrote:

--->
Unfortunatelly, due to my mistake this thread was splitted,
The other part is here:
http://lists.freedesktop.org/archives/systemd-devel/2015-January/027648.html

---<

> 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

Well, if the kernel, policy, valgrind, some arch or whatever else
disallows us to install a crash handler for one of the signals, than
we won't have one on it, but I don't think that's a problem. We do the
best we can regarding setting up crash handling and if we can't we
just proceed. I mean, if your system crashes then you are fucked
anyway - the crash handler is something to makes it slighlty less
annyoing, but essentially you are still fucked, there's no way around
it.

Hence, I think ignoring the result of sigaction_many() is really the
best thing we can do: we make the best of the situation, for a
peripheral feature.

--->
I've replied in the 2nd part of this td - btw, can someone merge those parts?
---<

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

I'd take a patch for this, but in general: the crash handler is
nothing that is supposed to recover your system. All it does its close
all fds, fork, and coredump in the child while freezing in the
parent. That makes sure that all communication with PID 1 is severed
(so that clients talking to it don't hang), we get a clean coredump,
but the kernel doesn't oops immediately. That's really all there is to
it.

So far we never ran into stack overflow issues, hence nobody was ever
tempted to cover that case too, and set up a separate stack. In
particular since doing this without testing it is pointless...

--->
Actually, when the program is a critical part of the system, than ALL sig handlers should use separate stack (what in fact makes the life easier) - that's because it's completely unpredictable, whether there's enough stack space to execute the handler - i.e. each sig handler can cause segfault when it's executed at the bottom of the stack (where top/bootom is only a matter of naming convention)
---<

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

We set SA_NODEFER so that the signal handler can raise the signal
again in the child process so that we get clean coredump for the logs.

Anyway, given that this all is not obvious to see from the sources I
have now added a couple of comments there.

--->
The only effect of SA_NODEFER is that sig mask is not automatically set for a signal which triggered the handler.
This means nesting of signals, as the handler can be interrupted and another instance is started for a new signal context.  (and sig handlers are not signal-safe with SA_NODEFER, what means that they have to be fully re-entrant)
This also means, that there's completely no warranty that the handler won't be interrupted with external signal -> mess.
---<

Lennart