search

Stebalien / tempfile

Temporary file library for rust
http://stebalien.com/projects/tempfile-rs
Apache License 2.0
1.2k stars 120 forks source link

Make tempfile robust against TLS deallocation #281

Open stoeckmann opened 8 months ago

stoeckmann commented 8 months ago

This PR is a result of discussion at https://github.com/smol-rs/fastrand/pull/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_drop starts returning true for Rng, 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.

Stebalien commented 8 months ago

Honestly, I'd rather fix https://github.com/Stebalien/tempfile/issues/178 if we're going to do anything here (although I'm still not sold on that either).

stoeckmann commented 8 months ago
  • Is this only an issue if someone tries to create a temporary file from a drop implementation in a thread local?

Correct

  • Just to confirm, this will only ever be an issue if Rng ever has drop glue (which likely won't happen, as far as I can tell)?

Either that or mem::needs_drop returns true for another reason. It's not stated that it cannot happen, but I haven't seen such a situation yet.

Honestly, I'd rather fix #178 if we're going to do anything here (although I'm still not sold on that either).

Since this is a hypothetical issue (I guess, because nobody complained), we can keep it as it is.