Poorly-documented safety assumptions in unix::stack_overflow::init

[workingjubilee]

Relevant history:

• -Zsanitizer=address failing to catch SIGSEGV #69524
• unix: Don't override existing SIGSEGV/BUS handlers #69685

It's not clear, to me, why the update of the NEEDS_ALTSTACK variable is occurring. I don't know everything there is to know about signal handlers, but I do know

• they are terribly underspecified
• they are surprisingly thread-specific in multithreaded programs
• yet some aspects of them remain process-wide in relevance

We have two fairly distinct codepaths for make_handler. In code we control, we only call one or the other (i.e. a constant input). They're really quite different functions. So it's not clear where the logical dependencies emerge between the two.

[workingjubilee]

uh what?

I have no idea what happened here.
I'm going to reuse this for something I found, one sec.

[workingjubilee]

cc @cuviper I believe you were the one to add the NEED_ALTSTACK var. it was quite a while ago, but do you happen to remember anything useful to know about the reasons underlying that PR and why the global variable approach? I ask mostly because Handler::new() is only called from the thread impl, and it could be an entirely different function, for instance.

Perhaps it couldn't be at the time, I think new_main is relatively recent, but

[cuviper]

I guess you must have found commit 676b9bc to "blame" me, but I don't remember any more about that offhand. I think it's global because the decision is made once when registering the signals, but needs to affect all threads.

(I won't be able to dig any more until next week at the earliest.)

[workingjubilee]

dw, I figured 4 years ago might've been too long ago to remember!

[workingjubilee]

@cuviper for reference this has mostly led to this question, because I started thinking about the ordering of when we should set NEED_ALTSTACK: https://github.com/rust-lang/rust/pull/127843/files#r1680398732

[joboet]

There are two parts to our SIGSEGV handler:

• The handler action registered with sigaction. This is shared per-process.
• The alternate stack the handler function is run on, registered with sigaltstack. This is per-thread.

In the Rust runtime initialization code, we attempt to install our signal handler. If there is no handler previously installed, we need to create a signal stack for every thread we spawn, so that the handler has a stack to run on (the main thread obviously can't be used, it already overflowed). If there is already a handler installed, we do not overwrite it. Thus, when we create another thread, we do and should not allocate a signal stack, as that might interfere with the code that registered the action. NEED_ALTSTACK is used to propagate this check from the main initialization code to the initialization code for threads. It also ensures that the PAGE_SIZE variable used to cache the platform page size has the current value (we could use a OnceLock for this, too, but this is more efficient).

[workingjubilee]

Part of the reason this code is hard to follow is that there are several unsafe fn with no obvious safety conditions on them. Thus it feels like everything is overly exciting, when in reality the unsafe operations are usually completely harmless inspections.

I've already started cleaning this up as part of #127747