Re: [pve-devel] [RFC proxmox 2/7] sys: add make_tmp_dir

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Mon, Aug 21, 2023 at 03:44:39PM +0200, Lukas Wagner wrote:
> Under the hood, this function calls `mkdtemp` from libc. Unfortunatly
> the nix crate did not provide bindings for this function, so we have
> to call into libc directly.
> 
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
>  proxmox-sys/src/fs/dir.rs | 73 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 71 insertions(+), 2 deletions(-)
> 
> diff --git a/proxmox-sys/src/fs/dir.rs b/proxmox-sys/src/fs/dir.rs
> index 6aee316..72bf1ad 100644
> --- a/proxmox-sys/src/fs/dir.rs
> +++ b/proxmox-sys/src/fs/dir.rs
> @@ -1,6 +1,8 @@
> -use std::ffi::CStr;
> +use std::ffi::{CStr, CString, OsStr};
> +use std::fs::File;
> +use std::os::unix::ffi::OsStrExt;
>  use std::os::unix::io::{AsRawFd, OwnedFd};
> -use std::path::Path;
> +use std::path::{Path, PathBuf};
>  
>  use anyhow::{bail, Error};
>  use nix::errno::Errno;
> @@ -8,6 +10,8 @@ use nix::fcntl::OFlag;
>  use nix::sys::stat;
>  use nix::unistd;
>  
> +use proxmox_lang::try_block;
> +
>  use crate::fs::{fchown, CreateOptions};
>  
>  /// Creates directory at the provided path with specified ownership.
> @@ -152,6 +156,54 @@ fn create_path_at_do(
>      }
>  }
>  
> +///  Create a temporary directory.
> +///
> +/// `prefix` determines where the temporary directory will be created. For instance, if
> +/// `prefix` is `/tmp`, on success the function will return a path in the style of
> +/// `/tmp/tmp_XXXXXX`, where X stands for a random string, ensuring that the path is unique.
> +///
> +/// By default, the created directory has `0o700` permissions. If this is not desired, custom
> +/// [`CreateOptions`] can be passed via the `option` parameter.
> +pub fn make_tmp_dir<P: AsRef<Path>>(
> +    prefix: P,
> +    options: Option<CreateOptions>,
> +) -> Result<PathBuf, Error> {
> +    let mut template = prefix.as_ref().to_owned();
> +    template = template.join("tmp_XXXXXX");
> +    let template = CString::new(template.into_os_string().as_bytes())?;
> +
> +    let raw_template_buffer = template.into_raw();

^ This might be shorter without going over the `CString` type with just
a `Vec<u8>` with an explicit `.push(0)` without temporarily giving up
ownership for the `mkdtemp` call.

> +
> +    let path = unsafe {
> +        let raw_returned_buffer = libc::mkdtemp(raw_template_buffer);
> +        if raw_returned_buffer.is_null() {

Need to add

    let err = io::Error::last_os_error();

as the very first thing you do in this branch.

Never give any external libraries a chance to mess with your
`errno` values before you use them, even `std` ;-)

> +            // The returned pointer points to the same buffer, so in case
> +            // of an error we need to make sure to claim it back to that
> +            // it is freed properly.
> +            drop(CString::from_raw(raw_template_buffer));

^ but I think we could avoid this - but as long as you fix up the
`errno` usage the CString code can also just stay this way, no strong
feelings there.

> +            return Err(std::io::Error::last_os_error().into());
> +        }
> +        CString::from_raw(raw_returned_buffer)
> +    };
> +
> +    let path = OsStr::from_bytes(path.as_bytes());
> +    let path = PathBuf::from(path);

^ This seems like there should be a cheap non-copying version:
    PathBuf::from(OsString::from_vec(path.into_bytes())) ?

> +
> +    if let Some(options) = options {
> +        if let Err(err) = try_block!({
> +            let fd = crate::fd::open(&path, OFlag::O_DIRECTORY, stat::Mode::empty())?;
> +            let mut file = File::from(fd);
> +            options.apply_to(&mut file, &path)?;

^ Huh, just noticing this weirdness, can we fix up the apply_to API to
take a `RawFd` or `&BorrowedFd` instead of a `File`? This is... not a
file... :-) And also `CreateOptions` doesn't really need it to be
mutable ;-)

> +            Ok::<(), Error>(())
> +        }) {
> +            let _ = unistd::unlink(&path);

^ This calls `unlink(2)` which does not remove directories. You need to
use either `std::fs::remove_dir()` or `unlinkat` with
`UnlinkatFlags::RemoveDir`.

Also, please also log the error if this fails.

> +            bail!("could not apply create options to new temporary directory: {err}");
> +        }
> +    }
> +
> +    Ok(path)
> +}
> +
>  #[cfg(test)]
>  mod tests {
>      use super::*;
> @@ -169,4 +221,21 @@ mod tests {
>          )
>          .expect("expected create_path to work");
>      }
> +
> +    #[test]
> +    fn test_make_tmp_dir() -> Result<(), Error> {
> +        let options = CreateOptions::new()
> +            .owner(unistd::Uid::effective())
> +            .group(unistd::Gid::effective())
> +            .perm(stat::Mode::from_bits_truncate(0o755));
> +
> +        let path = make_tmp_dir("/tmp", Some(options))?;
> +
> +        assert!(path.exists());
> +        assert!(path.is_dir());
> +
> +        std::fs::remove_dir_all(&path)?;
> +
> +        Ok(())
> +    }
>  }
> -- 
> 2.39.2