Stebalien / tempfile

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

[rfc] setting permissions for unnamed tempfiles #292

Closed cyphar closed 3 months ago

cyphar commented 3 months ago

While this might seem like a strange request, given TempFile::persist if you want to have a particular permission set that obeys umask, being able to set the mode during the initial creation is ideal.

Windows doesn't support setting permissions, so there's no changes needed there.


However, this feature request is actually motivated by more generic umask issues on Linux.

The only guaranteed way to get the umask of a process is using umask(2). However, umask(2) requires you to set the umask in order to get the current one. The umask is shared by all CLONE_FS processes, which means that a multi-threaded process (or a child process forked with CLONE_FS) cannot use this interface because changing the umask could lead to files having the wrong permissions during the race window (or a forking process could inherit the wrong umask in the child).

For "newer" kernels (v4.11, released in 2017) you can just parse /proc/thread-self/status to get the Umask: field. However, for older kernels there are only two solutions:

  1. Fork a subprocess (without CLONE_FS), get the umask in the guaranteed-to-be-single-threaded process, and return. This option is quite cumbersome and on Linux it is very difficult to safely create a private subprocess from a library without the main application noticing it (such as through SIGCHLD or wait). There are also problems with a library inside a privileged application doing this -- what binary should we execute? Ideally you would only do this after fork but Rust doesn't provide a fork primitive so you would need to use Process::pre_exec and then kill the process, or try to use clone() directly.

    In short, this method is awful.

  2. Try to create a file with permissions 0o777 and check what permissions it was actually created with.

Unfortunately, this crate only supports setting permissions with named files, which means that programs that want to use O_TMPFILE if available (Linux 3.11) need to write their own code to use O_TMPFILE and then fallback to tempfile::Builder if that fails. It would be nice if tempfile::Builder had a method that let you create unnamed files.

(Obviously, this whole thing is racy, and that's fine for my particular purposes. But I'm not asking for a get_umask API from this crate so that shouldn't matter for this RFC anyway.)

Stebalien commented 3 months ago

I'm really confused, what are you trying to do here? Unnamed temporary files are not supposed to be readable by other processes/users/etc. in general (unless you, e.g., send a file descriptor). Why do you need to set a umask for unnamed temporary files?

NOTE: if O_TMPFILE doesn't work, the default permissions are 0o600.

cyphar commented 3 months ago

You can do persist on unnamed temporary files to make them accessible by other users though. However, if you want to set a particular mode (that obeys umask) there is no way to do that without manually calculating the right mode and doing chmod (and as mentioned above, getting the current umask is difficult because the interfaces to get the value of umask are either unusable in practice (umask(2)) or require newer kernels (the Umask field in /proc/self/status)).

what are you trying to do here?

I am writing some code to figure out the umask of the running process in a way that won't cause issues in multi-threaded programs (this is needed by a library I'm working on). The least painful way of doing this is to create a temporary file with mode 0o777 and then doing fstat on it to calculate the umask (method 2 in the issue description). Ideally we would use O_TMPFILE if it's available, and then fallback to a regular named temporary file.

This crate already implements almost all of this, the only feature needed is the ability to set permissions. Otherwise, I have to do O_TMPFILE myself and then use tempfile as a fallback. That works, of course, but it would be nice if it could be done with a single tempfile call...

Stebalien commented 3 months ago

Ok, got it. Unfortunately, I can't think of a single other use-case for setting the permissions of unnamed temporary file and I don't want to extend the API just for this one niche case.

However, the following code should get you there:

use std::fs::Permissions;
use std::os::unix::fs::PermissionsExt;

use rustix::fs::{OFlags, OpenOptionsExt};
use rustix::io::Errno;
use tempfile::{env, Builder};

let tmpfile = OpenOptions::new()
    .read(true)
    .write(true)
    .custom_flags(OFlags::TMPFILE.bits() as i32) // do not mix with `create_new(true)`
    .open(env::temp_dir())
    .or_else(|e| {
        match Errno::from_io_error(&e) {
            // These are the three "not supported" error codes for O_TMPFILE.
            Some(Errno::OPNOTSUPP) | Some(Errno::ISDIR) | Some(Errno::NOENT) => {
                Ok(Builder::new()
                    .permissions(Permissions::from_mode(0o777))
                    .tempfile()?
                    .into_file())
            }
            _ => Err(e),
        }
    });

It's copying a bit of code, but not a significant amount.

cyphar commented 3 months ago

Thanks, I already had something similar but thanks for writing it up!

I mainly opened this to ask if you would be interested in adding such a feature, I agree it's fairly niche. I'm happy to close this as "not planned" if you feel it doesn't make sense. umask is something most people prefer not to deal with, so I suspect most applications will do a standard chown anyway. Obeying umask in this way is something only some command-line tools and libraries care about. (For the library I'm working on, it isn't really safe for us to do a chown.)

Stebalien commented 3 months ago

I'll close this as not planned for now and re-open it if it comes up again (e.g., if we get a reliable way to "persist" unnamed temporary files).