tempfile: add new_with_modes and is_named methods#390
tempfile: add new_with_modes and is_named methods#390champtar wants to merge 1 commit intobytecodealliance:mainfrom
Conversation
f1e7b23 to
0c6abfb
Compare
|
Still pretty new to rust, not sure if this is ok to use |
cap-tempfile/src/tempfile.rs
Outdated
| let mode = match mode { | ||
| Some(mode) => Mode::from(mode), | ||
| None => Mode::from(0o666), | ||
| }; |
There was a problem hiding this comment.
| let mode = match mode { | |
| Some(mode) => Mode::from(mode), | |
| None => Mode::from(0o666), | |
| }; | |
| let mode = mode.unwrap_or(0o666).map(Mode::from); |
would be slightly shorter.
Actually for internal APIs I'd say we should actually use rustic::fs::Mode as much as possible; in other words make this function take a Mode instead. (We can't for public APIs yet as we aren't depending on rustix there yet AFAIK)
There was a problem hiding this comment.
This fails for Darwin
error[E0308]: mismatched types
--> cap-tempfile/src/tempfile.rs:130:19
|
130 | opts.mode(mode.as_raw_mode());
| ---- ^^^^^^^^^^^^^^^^^^ expected `u32`, found `u16`
| |
| arguments to this method are incorrect
|
note: method defined here
--> /home/runner/work/cap-std/cap-std/cap-primitives/src/fs/open_options.rs:257:8
|
257 | fn mode(&mut self, mode: u32) -> &mut Self;
| ^^^^
help: you can convert a `u16` to a `u32`
|
130 | opts.mode(mode.as_raw_mode().into());
| +++++++
error[E0277]: the trait bound `Mode: From<u32>` is not satisfied
--> cap-tempfile/src/tempfile.rs:159:56
|
159 | let (fd, name) = new_tempfile(dir, false, Some(rustix::fs::Mode::from(mode)))?;
| ^^^^^^^^^^^^^^^^ the trait `From<u32>` is not implemented for `Mode`
|
= help: the trait `From<u32>` is not implemented for `Mode`
but trait `From<u16>` is implemented for it
= help: for that trait implementation, expected `u16`, found `u32`
| eprintln!("notice: Detected older Windows"); | ||
| } | ||
|
|
||
| // Test that we can create with 0o000 mode |
There was a problem hiding this comment.
A bit unusual, how about 0o200 instead so we can at least write to the file?
There was a problem hiding this comment.
Definitely unusual, but we can write to the fd as it was open with O_RDWR
242819e to
62a9a6f
Compare
On filesystems that do not support `O_TMPFILE` (e.g. `vfat`), TempFile::new() automatically falls back to a potentially world readable named temp file. Add TempFile::new_with_modes() to create temp files with a chosen mode directly. We pass 2 modes, one for unnamed tempfile (happy path, created with `O_TMPFILE`), and one for named tempfile. The final mode is still restricted by the process umask.
62a9a6f to
72ee5b6
Compare
new_with_mode method|
I switched to passing 2 modes (for unnamed and named case) + adding is_named() method. |
|
This one is ready for re-review |
| #[cfg(any(target_os = "android", target_os = "linux"))] unnamed_mode: Option<u32>, | ||
| #[cfg(unix)] named_mode: Option<u32>, |
There was a problem hiding this comment.
The first condition implies the second, which makes things feel backwards to me.
This all might be cleaner if we used a struct NewTempfileOpts instead - then there'd be conditionals in way fewer places (just the definition and uses, not needing to thread them through all the internal APIs)
There was a problem hiding this comment.
The first condition implies the second, which makes things feel backwards to me.
I can reorder, i think I put unnamed_mode first because I use it first
This all might be cleaner if we used a
struct NewTempfileOptsinstead - then there'd be conditionals in way fewer places (just the definition and uses, not needing to thread them through all the internal APIs)
That would just change new_tempfile and new_tempfile_linux definition, and new_tempfile_linux doesn't need named_mode, so not much difference. I can do it in a separate commit if you want to see.
| // On Linux, try O_TMPFILE | ||
| #[cfg(any(target_os = "android", target_os = "linux"))] | ||
| if let Some(f) = new_tempfile_linux(d, anonymous)? { | ||
| if let Some(f) = new_tempfile_linux(d, anonymous, unnamed_mode)? { |
There was a problem hiding this comment.
"anonymous" and "unnamed" mean the same thing, so how about anonymous_mode?
There was a problem hiding this comment.
Then we would have named_mode and anonymous_mode
Looking at the man page (https://man7.org/linux/man-pages/man2/openat.2.html):
O_TMPFILE (since Linux 3.11)
Create an unnamed temporary regular file.
I would replace anonymous by unnamed instead
When creating anonymous temporary files, use mode 0o000 instead of 0o666 to further discourage anything else from opening them. This fixes a bug pointed out in #390.
When creating anonymous temporary files, use mode 0o000 instead of 0o666 to further discourage anything else from opening them. This fixes a bug pointed out in #390.
* cap-tempfile: Don't create anonymous files with 0o666 When creating anonymous temporary files, use mode 0o000 instead of 0o666 to further discourage anything else from opening them. This fixes a bug pointed out in #390. * Fix anonymous files on Windows. * Fix umask tests on non-Linux Unix platforms. * Fix tempfile path. * Fix compilation on MSRV. * Fix compilation on Darwin.
|
I apologize for the very late review here. I think we can fix the permissions of I'm hesitant to add a |
|
My goal is to do atomic writes, but have the temporary file not readable AND respect umask (optimize functions like https://docs.rs/cap-std-ext/latest/cap_std_ext/dirext/trait.CapStdExtDirExt.html#tymethod.atomic_replace_with) When using The interest of |
On filesystems that do not support
O_TMPFILE(e.g.vfat), TempFile::new()automatically falls back to a potentially world readable named temp file.
Add TempFile::new_with_modes() to create temp files with a chosen mode directly.
We pass 2 modes, one for unnamed tempfile (happy path, created with
O_TMPFILE),and one for named tempfile.
The final mode is still restricted by the process umask.
@cgwalters this will help improve cap-std-ext atomic* permissions handling