Skip to content

Conversation

joboet
Copy link
Member

@joboet joboet commented May 23, 2025

The panic machinery uses TLS, so panicking if no TLS keys are left can lead to infinite recursion (see #140798 (comment)). Rather than having separate logic for the panic count and the thread name, just always abort the process if a TLS key allocation fails. This also has the benefit of aligning the key-based TLS implementation with the documentation, which does not mention that a panic could also occur because of resource exhaustion.

The panic machinery uses TLS, so panicking if no TLS keys are left can lead to infinite recursion (see rust-lang#140798 (comment)). Rather than having separate logic for the panic count and the thread name, just always abort the process if a TLS key allocation fails. This also has the benefit of aligning the key-based TLS implementation with the documentation, which does not mention that a panic could also occur because of resource exhaustion.
@rustbot
Copy link
Collaborator

rustbot commented May 23, 2025

r? @tgross35

rustbot has assigned @tgross35.
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 S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 23, 2025
@tgross35
Copy link
Contributor

tgross35 commented May 23, 2025

The reasoning at #140798 (comment) makes sense to me and this diff is straightforward enough, r=me with green CI.

It would be nice if somebody (@Berrysoft?) could confirm that we get an abort rather than the overflow with this patch.

(if this comment posts about 4 times, blame it on the apparent github outage)

@tgross35
Copy link
Contributor

tgross35 commented Jun 3, 2025

May as well get this merged. @Berrysoft if you get the chance, could you see if the behavior you observed changes with this?

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 3, 2025

📌 Commit 8bf5153 has been approved by tgross35

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 Jun 3, 2025
bors added a commit that referenced this pull request Jun 3, 2025
Rollup of 8 pull requests

Successful merges:

 - #137725 (Add `iter` macro)
 - #141455 (std: abort the process on failure to allocate a TLS key)
 - #141569 (Replace ad-hoc ABI "adjustments" with an `AbiMap` to `CanonAbi`)
 - #141698 (Use the informative error as the main const eval error message)
 - #141925 (Remove bootstrap cfgs from library/)
 - #141943 (Remove pre-expansion AST stats.)
 - #141945 (Remove `Path::is_ident`.)
 - #141957 (Add missing `dyn` keywords to tests that do not test for them Part 2)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d096ebf into rust-lang:master Jun 4, 2025
6 checks passed
@rustbot rustbot added this to the 1.89.0 milestone Jun 4, 2025
rust-timer added a commit that referenced this pull request Jun 4, 2025
Rollup merge of #141455 - joboet:tls_exhaustion_abort, r=tgross35

std: abort the process on failure to allocate a TLS key

The panic machinery uses TLS, so panicking if no TLS keys are left can lead to infinite recursion (see #140798 (comment)). Rather than having separate logic for the panic count and the thread name, just always abort the process if a TLS key allocation fails. This also has the benefit of aligning the key-based TLS implementation with the documentation, which does not mention that a panic could also occur because of resource exhaustion.
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Jun 4, 2025
Rollup of 8 pull requests

Successful merges:

 - rust-lang/rust#137725 (Add `iter` macro)
 - rust-lang/rust#141455 (std: abort the process on failure to allocate a TLS key)
 - rust-lang/rust#141569 (Replace ad-hoc ABI "adjustments" with an `AbiMap` to `CanonAbi`)
 - rust-lang/rust#141698 (Use the informative error as the main const eval error message)
 - rust-lang/rust#141925 (Remove bootstrap cfgs from library/)
 - rust-lang/rust#141943 (Remove pre-expansion AST stats.)
 - rust-lang/rust#141945 (Remove `Path::is_ident`.)
 - rust-lang/rust#141957 (Add missing `dyn` keywords to tests that do not test for them Part 2)

r? `@ghost`
`@rustbot` modify labels: rollup
bjorn3 pushed a commit to rust-lang/rustc_codegen_cranelift that referenced this pull request Jun 5, 2025
Rollup of 8 pull requests

Successful merges:

 - rust-lang/rust#137725 (Add `iter` macro)
 - rust-lang/rust#141455 (std: abort the process on failure to allocate a TLS key)
 - rust-lang/rust#141569 (Replace ad-hoc ABI "adjustments" with an `AbiMap` to `CanonAbi`)
 - rust-lang/rust#141698 (Use the informative error as the main const eval error message)
 - rust-lang/rust#141925 (Remove bootstrap cfgs from library/)
 - rust-lang/rust#141943 (Remove pre-expansion AST stats.)
 - rust-lang/rust#141945 (Remove `Path::is_ident`.)
 - rust-lang/rust#141957 (Add missing `dyn` keywords to tests that do not test for them Part 2)

r? `@ghost`
`@rustbot` modify labels: rollup
tautschnig pushed a commit to model-checking/verify-rust-std that referenced this pull request Jun 17, 2025
…oss35

std: abort the process on failure to allocate a TLS key

The panic machinery uses TLS, so panicking if no TLS keys are left can lead to infinite recursion (see rust-lang#140798 (comment)). Rather than having separate logic for the panic count and the thread name, just always abort the process if a TLS key allocation fails. This also has the benefit of aligning the key-based TLS implementation with the documentation, which does not mention that a panic could also occur because of resource exhaustion.
tautschnig pushed a commit to model-checking/verify-rust-std that referenced this pull request Jun 17, 2025
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#137725 (Add `iter` macro)
 - rust-lang#141455 (std: abort the process on failure to allocate a TLS key)
 - rust-lang#141569 (Replace ad-hoc ABI "adjustments" with an `AbiMap` to `CanonAbi`)
 - rust-lang#141698 (Use the informative error as the main const eval error message)
 - rust-lang#141925 (Remove bootstrap cfgs from library/)
 - rust-lang#141943 (Remove pre-expansion AST stats.)
 - rust-lang#141945 (Remove `Path::is_ident`.)
 - rust-lang#141957 (Add missing `dyn` keywords to tests that do not test for them Part 2)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

4 participants