-
Notifications
You must be signed in to change notification settings - Fork 53
Conversation
f1e7b23 to
0c6abfb
Compare
champtar
commented
May 26, 2025
Still pretty new to rust, not sure if this is ok to use #[cfg(unix)] for the mode argument, but 'it works (TM)'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now using rustic::fs::Mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Back to u32 for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit unusual, how about 0o200 instead so we can at least write to the file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 will allow to keep the happy path (O_TMPFILE) simpler/faster in coreos/cap-std-ext#73
champtar
commented
Jun 2, 2025
This one is ready for re-review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"anonymous" and "unnamed" mean the same thing, so how about anonymous_mode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
sunfishcode
commented
Dec 11, 2025
I apologize for the very late review here. I think we can fix the permissions of new_anonymous to not be readable or writable, so I've now submitted #403 to do that.
I'm hesitant to add a new_with_mode function to cap-tempfile itself as it doesn't correspond to anything in the regular tempfile crate. If tempfile doesn't have an interface for this, is it generally needed?
champtar
commented
Dec 15, 2025
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 O_TMPFILE we are already safe, so you can create the file directly with the wanted permissions and it'll apply the umask during creation without extra steps.
When you can't use O_TMPFILE, you set the permissions to 0o000, and then you need to somehow retrieve the umask (by reading /proc or creating a temp file just for that) to compute the permission.
The interest of new_with_modes + is_named is to avoid having to retrieve umask + call chmod in the O_TMPFILE case, that's it.
sunfishcode
commented
Feb 15, 2026
@champtar It sounds like the same concerns would apply to the regular tempfile crate as well, right?
cap-tempfile here is aiming to be to be a cap- adaptation of the tempfile crate. If tempfile adds a new_with_mode option then it will make sense for cap-tempfile to do so.
champtar
commented
Mar 11, 2026
@sunfishcode I completely forgot to answer you, and then today in something completely unrelated I had to fix a race condition, fixed by using atomic writes.
For devs to use atomic writes by default it needs to be easy to do and cheap.
cap-tempfile support replace with O_TMPFILE, tempfile doesn't and will not (Stebalien/tempfile#13), so new_with_mode will never be added to tempfile.
IMO being an adaptation means covering all the use cases of tempfile, but it doesn't prevent you from having extra features.
Uh oh!
There was an error while loading. Please reload this page.
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