Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

std: detect stack overflows in TLS destructors on UNIX #131282

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

joboet
Copy link
Member

@joboet joboet commented Oct 5, 2024

Fixes #111272.

With #127912 merged, we now have all the infrastructure in place to support stack overflow detection in TLS destructors. This was not possible before because the signal stack was freed in the thread main function, thus a SIGSEGV afterwards would immediately crash. And on platforms without native TLS, the guard page address was stored in an allocation freed in a TLS destructor, so would not be available. #127912 introduced the local_pointer macro which allows storing a pointer-sized TLS variable without allocation and the thread_cleanup runtime function which is called after all other code managed by the Rust runtime. This PR simply moves the signal stack cleanup to the end of thread_cleanup and uses local_pointer to store every necessary variable. And so, everything run under the Rust runtime is now properly protected against stack overflows.

@rustbot
Copy link
Collaborator

rustbot commented Oct 5, 2024

r? @Amanieu

rustbot has assigned @Amanieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added O-hermit Operating System: Hermit O-SGX Target: SGX O-solid Operating System: SOLID O-unix Operating system: Unix-like O-windows Operating system: Windows S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 5, 2024
@joboet joboet added A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows A-thread-locals Area: Thread local storage (TLS) labels Oct 5, 2024
@bors
Copy link
Contributor

bors commented Jan 15, 2025

☔ The latest upstream changes (presumably #135540) made this pull request unmergeable. Please resolve the merge conflicts.

@joboet joboet force-pushed the thread_local_stack_overflow branch from f46b255 to 83152c6 Compare January 19, 2025 15:55
@rust-log-analyzer

This comment has been minimized.

Fixes rust-lang#111272.

With rust-lang#127912 merged, we now have all the infrastructure in place to support stack overflow detection in TLS destructors. This was not possible before because the signal stack was freed in the thread main function, thus a SIGSEGV afterwards would immediately crash. And on platforms without native TLS, the guard page address was stored in an allocation freed in a TLS destructor, so would not be available. rust-lang#127912 introduced the `local_pointer` macro which allows storing a pointer-sized TLS variable without allocation and the `thread_cleanup` runtime function which is called after all other code managed by the Rust runtime. This PR simply moves the signal stack cleanup to the end of `thread_cleanup` and uses `local_pointer` to store every necessary variable. And so, everything run under the Rust runtime is now properly protected against stack overflows.
@joboet joboet force-pushed the thread_local_stack_overflow branch from 83152c6 to 6620e2b Compare January 19, 2025 19:44
@Amanieu
Copy link
Member

Amanieu commented Jan 22, 2025

The logic was a little hard to follow in the code, but after looking at it for a bit it makes sense.

@bors r+

@bors
Copy link
Contributor

bors commented Jan 22, 2025

📌 Commit 6620e2b has been approved by Amanieu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 22, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 22, 2025
…, r=Amanieu

std: detect stack overflows in TLS destructors on UNIX

Fixes rust-lang#111272.

With rust-lang#127912 merged, we now have all the infrastructure in place to support stack overflow detection in TLS destructors. This was not possible before because the signal stack was freed in the thread main function, thus a SIGSEGV afterwards would immediately crash. And on platforms without native TLS, the guard page address was stored in an allocation freed in a TLS destructor, so would not be available. rust-lang#127912 introduced the `local_pointer` macro which allows storing a pointer-sized TLS variable without allocation and the `thread_cleanup` runtime function which is called after all other code managed by the Rust runtime. This PR simply moves the signal stack cleanup to the end of `thread_cleanup` and uses `local_pointer` to store every necessary variable. And so, everything run under the Rust runtime is now properly protected against stack overflows.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 23, 2025
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#131282 (std: detect stack overflows in TLS destructors on UNIX)
 - rust-lang#134746 (Don't ICE in coerce when autoderef fails to structurally normalize non-WF type in new solver)
 - rust-lang#135790 (Update windows-gnu targets to set `DebuginfoKind::DWARF`)
 - rust-lang#135878 (ci: use 8 core arm runner for dist-aarch64-linux)
 - rust-lang#135879 (fix outdated file path ref in llvm)
 - rust-lang#135883 (Remove erroneous `unsafe` in `BTreeSet::upper_bound_mut`)
 - rust-lang#135884 (remove implied end of slice)
 - rust-lang#135898 (rustdoc-json-types: Finalize dyn compatibility renaming)

r? `@ghost`
`@rustbot` modify labels: rollup
@jieyouxu
Copy link
Member

jieyouxu commented Jan 23, 2025

I think this failed in rollup: #135906 (comment)

testing: silent-thread
testing: loud-thread
testing: silent-thread-tls
--- stderr -------------------------------

thread 'main' panicked at C:\a\rust\rust\tests\ui\runtime\out-of-stack.rs:71:5:
assertion failed: !status.success()
------------------------------------------

failures:
    [ui] tests\ui\runtime\out-of-stack.rs

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 23, 2025
@joboet
Copy link
Member Author

joboet commented Feb 2, 2025

I'm completely lost as to why this would fail. The stack overflow clearly gets detected properly (the correct message is printed), but the program still reports a success. Do stack overflows in TLS callback functions not lead to an abort?

@ChrisDenton you know a lot more about Windows than I do, have you got any ideas?

@ChrisDenton
Copy link
Member

Hmm.. the stack overflow is happening while the thread is already exiting? I'm pretty sure in that case the exiting thread just returns the original exit code rather than changing it. If that's indeed the case (I'll double check when I have a moment) then I don't think there's anything we can reasonably do about it.

@ChrisDenton
Copy link
Member

ChrisDenton commented Feb 2, 2025

Oh right, now that I'm more awake I think I can explain what's going on.

Thread locals drops are run via a TLS callback function. When running the callback, any exceptions thrown (such as the stack overflow exception) will be essential caught and discarded. The callback function will end and further TLS callback functions won't run but neither will the normal exception behaviour.

However, I'm finding it hard to find any official docs about exceptions in TLS callbacks.

We do add a Vectored Exception Handler to print the stack overflow message. This should still happen because the handler is called before unwinding, etc.

@joboet
Copy link
Member Author

joboet commented Feb 2, 2025

Mmmh. I guess this was always the case and should be fine... I'll just skip the test.

@joboet joboet added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows A-thread-locals Area: Thread local storage (TLS) O-hermit Operating System: Hermit O-SGX Target: SGX O-solid Operating System: SOLID O-unix Operating system: Unix-like O-windows Operating system: Windows S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stack overflow not caught in Drop for TLS data
7 participants