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

Tempfile V4 #327

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from
Draft

Tempfile V4 #327

wants to merge 11 commits into from

Conversation

Stebalien
Copy link
Owner

@Stebalien Stebalien commented Feb 15, 2025

This is a stack of changes for tempfile v4. My plan is to wait for the next Debian stable release then set the MSRV to that, although I may merge and cut an RC before then. This version tries to be minimally breaking in "normal" code but it will still break some code (ideally code that was already broken/buggy).

Other than the MSRV bump (letting us drop once_cell), changes include:

  • Implement AsRef<Path> on &NamedTempFile (and friends) instead of directly on NamedTempFile. That way, functions accepting AsRef<Path> won't be able to take (then drop) NamedTempFile by value. See Got bitten by Drop impl running to early when passing TempDir to fn(p: impl AsRef<Path>) #115.
  • Temporary directories are created with 0700 permissions by default instead of 0755 to match temporary files.
  • Temporary filenames no longer start with a . by default. Temporary files shouldn't generally be hidden. Additionally, it's now possible to configure the default temporary-filename prefix via the env package.
  • The Builder can now has a single lifetime instead of separate ones for prefix and suffix. It can also be constructed with a const Builder::new if desired.
  • Temporary file names are validated to be valid file-names, not including path separators, etc. It's still not safe to allow an attacker to control the prefix/suffix, but this gives us a little extra safety.
  • The closure passed to Builder::make used to take a &Path but now takes an &MakeParams struct (that dereferences to a &Path). This lets us pass additional builder parameters like the desired permissions.

TBD:

  • Figure out how to deal with inaccessible parent directories #40. I'd love to fix this but... I'd need to keep open a parent-directory file descriptor and I'd rather not do that if I don't have to. I should probably poke around and see what other temporary file libraries do.
  • Prevent temporary file cleanup by taking a shared advisory locks on all named temporary files to prevent temporary file cleaners from deleting them. This could break some things so we have to be careful, but taking a shared lock is probably reasonable? Possibly only do this if the parent directory is +t (sticky)? We can also make it configurable in the builder, the question will be: what's the default behavior?

Fixes:

Stebalien added 11 commits March 6, 2025 11:19
Otherwise it's really easy to pass a temporary file handle by reference,
causing it to be dropped by the receiver deleting the underlying file.

fixes #115
1. Default to "tmp" as the file prefix instead of ".tmp".
2. Make it possible to construct the builder in a constfn.
3. Use a single lifetime in the builder.
That way, we can pass through additional builder options. This new
struct implements AsRef<Path> and Deref<Target=Path> so breakage should
be minimal.
To do this, I had to switch from `AsRef<OsStr>` to `&str` for prefixes
and suffixes, but a quick search indicates that this will break
approximately nothing. I'm sure it'll break _someone's_ code, but
there's really no good reason to support arbitrary os-strings here.

The benefit is that projects can now construct project-wide temporary
file builders, storing them in constants.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant