PlatformInit resets signal handlers to SIG_DFL causing crashes

[dvyukov]

Version

0c46051

Platform

Linux 5.19.11-amd64 #1 SMP x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

LD_PRELOAD or link in any library that sets a signal handler and schedules signal delivery (e.g. a posix timer).

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior?

The library handles own signals.

What do you see instead?

The program crashes.

Additional information

#615 added this code that resets all signal handlers to SIG_DFL:

node/src/node.cc

Lines 426 to 434 in 0c46051

	// The hard-coded upper limit is because NSIG is not very reliable; on Linux,
	// it evaluates to 32, 34 or 64, depending on whether RT signals are enabled.
	// Counting up to SIGRTMIN doesn't work for the same reason.
	for (unsigned nr = 1; nr < kMaxSignal; nr += 1) {
	if (nr == SIGKILL || nr == SIGSTOP)
	continue;
	act.sa_handler = (nr == SIGPIPE || nr == SIGXFSZ) ? SIG_IGN : SIG_DFL;
	CHECK_EQ(0, sigaction(nr, &act, nullptr));
	}

This causes crashes is there is a signal handler installed.

While SIG_IGN can indeed be inherited across execve, all actual handlers (not SIG_IGN/DFL) are reset to SIG_DFL.
So I think the startup code should reset to SIG_DFL iff the handler is set of SIG_IGN. Any real handlers should be left intact.

@bnoordhuis @sam-github @melver

[bnoordhuis]

I'm a bit torn on whether to add workarounds for dodgy LD_PRELOAD wrappers but philosophical objections aside, I don't think node can detect their presence in a race-free manner.

If third-party code can run really early, then it can call sigaction() from a newly started thread. Meaning this won't work:

sigaction(nr, nullptr, &old);
// <-- sigaction()-from-other-thread race window here
if (old.sa_handler == SIG_DFL) sigaction(nr, &act, nullptr);

And neither will this:

sigaction(nr, &act, &old);
// <-- signal delivery race window here
if (old.sa_handler != SIG_DFL) sigaction(nr, &old, nullptr);

(And I'm skipping over the practical concern that reading .sa_handler and .sa_sigaction is problematic on some platforms.)

[dvyukov]

If third-party code can run really early, then it can call sigaction() from a newly started thread. Meaning this won't work:

Yes, this won't work and looks wild. I don't think we should be bothered with such possibilities. Signal handlers are inherently global and shouldn't be changed asynchronously.

A normal sequential check for existing handlers sounds reasonable.

(And I'm skipping over the practical concern that reading .sa_handler and .sa_sigaction is problematic on some platforms.)

The code currently does:

act.sa_handler = (nr == SIGPIPE || nr == SIGXFSZ) ? SIG_IGN : SIG_DFL; 

so presumably at least this is working fine on all platforms.
Do you expect if (oldact.sa_handler == SIG_IGN) to cause issues that are not triggered by the existing line?

[bnoordhuis]

Yes, because .sa_handler and .sa_sigaction don't have to be union members, they can be separate fields. I'm reasonably sure this is the case on AIX or IBM i (or both) because I remember running afoul of that in the past.

The real logic should look something like this, and, caveat emptor, I'm not 100% sure this won't emit warnings or isn't UB according to POSIX:

if ((old.sa_flags & SA_SIGINFO) && old.sa_sigaction == SIG_DFL || old.sa_handler == SIG_DFL)

Well, pull request welcome, I suppose.

[bnoordhuis]

@dvyukov are you interested in pursuing this? If not, all good, but then I'll go ahead and close this out.

[dvyukov]

Yes, I am. I just somehow missed your previous reply.
I have not looked yet, but is there existing type of tests that could test this? Or just a code change is fine?

[bnoordhuis]

There's test/embedding/embedtest.cc and concomitant JS file but I wouldn't want to vouch it's the right place because it also doubles as an embedding example. I'd be okay with just an Obviously Correct Looking(TM) code change.