Make tempfile robust against TLS deallocation #281
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
No need to match result of f if it is called only once.
The fastrand Rng should be created manually to avoid TLS deallocation issues. The fastrand::alphanumeric function uses a TLS storage internally. With current Rust and fastrand implementation, this is no problem. Yet, we can speed up the process a bit by not accessing the storage for every single random char.
Honestly, I'd rather fix #178 if we're going to do anything here (although I'm still not sold on that either). |
Correct
Either that or
Since this is a hypothetical issue (I guess, because nobody complained), we can keep it as it is. |
Stebalien
added a commit
that referenced
this pull request
Feb 15, 2025
Previously, creating a temporary file from a TLS destructor could panic in fastrand (because the thread-local RNG may have been deallocated). Now, we fork the RNG before we create each file, falling back on an RNG with a static seed if the thread-local RNG has been deallocated. Two downsides to this patch: 1. Temporary files created during TLS deallocation will have extremely predictable names until the `getrandom` re-seed kicks in (assuming that feature is enabled). IMO, that's fine; this would panic previously. 2. `getrandom` re-seeding used to re-randomize to the entire per-thread RNG, now it only applies to the per-filename RNG. However, the will still serve its purpose as a mitigation against potential DoS attacks. I also considered managing the thread-local RNG myself instead of relying on fastrand, but that just isn't worth the added code, IMO. Thanks to @stoeckmann for reporting this and explaining the issue to me. I went with this version instead of their version because I needed to keep `tmpname` as a separate function for some tempfile v4 changes. fixes #281
Stebalien
added a commit
that referenced
this pull request
Feb 15, 2025
Previously, creating a temporary file from a TLS destructor could panic in fastrand (because the thread-local RNG may have been deallocated). Now, we fork the RNG before we create each file, falling back on an RNG with a static seed if the thread-local RNG has been deallocated. Two downsides to this patch: 1. Temporary files created during TLS deallocation will have extremely predictable names until the `getrandom` re-seed kicks in (assuming that feature is enabled). IMO, that's fine; this would panic previously. 2. `getrandom` re-seeding used to re-randomize to the entire per-thread RNG, now it only applies to the per-filename RNG. However, the will still serve its purpose as a mitigation against potential DoS attacks. I also considered managing the thread-local RNG myself instead of relying on fastrand, but that just isn't worth the added code, IMO. Thanks to @stoeckmann for reporting this and explaining the issue to me. I went with this version instead of their version because I needed to keep `tmpname` as a separate function for some tempfile v4 changes. fixes #281
Stebalien
added a commit
that referenced
this pull request
Feb 15, 2025
Previously, creating a temporary file from a TLS destructor could panic in fastrand (because the thread-local RNG may have been deallocated). Now, we fork the RNG before we create each file, falling back on an RNG with a static seed if the thread-local RNG has been deallocated. Two downsides to this patch: 1. Temporary files created during TLS deallocation will have extremely predictable names until the `getrandom` re-seed kicks in (assuming that feature is enabled). IMO, that's fine; this would panic previously. 2. `getrandom` re-seeding used to re-randomize to the entire per-thread RNG, now it only applies to the per-filename RNG. However, the will still serve its purpose as a mitigation against potential DoS attacks. I also considered managing the thread-local RNG myself instead of relying on fastrand, but that just isn't worth the added code, IMO. Thanks to @stoeckmann for reporting this and explaining the issue to me. I went with this version instead of their version because I needed to keep `tmpname` as a separate function for some tempfile v4 changes. fixes #281
Stebalien
added a commit
that referenced
this pull request
Feb 15, 2025
Previously, creating a temporary file from a TLS destructor could panic in fastrand (because the thread-local RNG may have been deallocated). Now, we fork the RNG before we create each file, falling back on an RNG with a static seed if the thread-local RNG has been deallocated. Two downsides to this patch: 1. Temporary files created during TLS deallocation will have extremely predictable names until the `getrandom` re-seed kicks in (assuming that feature is enabled). IMO, that's fine; this would panic previously. 2. `getrandom` re-seeding used to re-randomize to the entire per-thread RNG, now it only applies to the per-filename RNG. However, the will still serve its purpose as a mitigation against potential DoS attacks. I also considered managing the thread-local RNG myself instead of relying on fastrand, but that just isn't worth the added code, IMO. Thanks to @stoeckmann for reporting this and explaining the issue to me. I went with this version instead of their version because I needed to keep `tmpname` as a separate function for some tempfile v4 changes. fixes #281
|
I've decided to go ahead and fix this in #324 just to be safe (even though I'm unable to reproduce it in a test no matter what I try). Thanks for reporting it and helping me understand the issue. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is a result of discussion at smol-rs/fastrand#77.
Right now the fastrand implementation is robust with current Rust implementation against TLS deallocation issues, because the Rng struct does not require any form of drop, so the TLS memory is kept in tact. At least I was unable to provoke an issue in any way.
Even if Rust changes at some time and
mem::needs_dropstarts returningtrueforRng, the instantiation of a new Rng will succeed. Unfortunately, it won't be random at that point, because the seed is a constant value in this case. See https://github.com/smol-rs/fastrand/blob/dda0fe824b078bc844300db86021c2552465c468/src/global_rng.rs#L26 for implementation.It really depends if tempfile is actually supposed to be functional during TLS deallocation (i.e. while thread local storages are dropped). If this is no goal, this PR can simply be closed.