Re: [pbs-devel] [PATCH backup v2] fix-3699: pbs-client/tools: use xdg basedirectories for tmp files

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On January 31, 2024 11:53 am, Maximiliano Sandoval wrote:
> Adds a helper to create temporal files in XDG_CACHE_HOME. If the we
> cannot use that path, we fallback to /tmp as before.
> 
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
> Differences from v1:
>  - temporal file -> temporary file
>  - add a test
> 
>  pbs-client/src/backup_reader.rs      | 31 ++++++-------------
>  pbs-client/src/backup_writer.rs      | 13 ++------
>  pbs-client/src/tools/mod.rs          | 45 ++++++++++++++++++++++++++++
>  proxmox-backup-client/src/catalog.rs | 19 ++----------
>  4 files changed, 59 insertions(+), 49 deletions(-)
> 
> diff --git a/pbs-client/src/tools/mod.rs b/pbs-client/src/tools/mod.rs
> index 1b0123a3..9a7327ed 100644
> --- a/pbs-client/src/tools/mod.rs
> +++ b/pbs-client/src/tools/mod.rs
> @@ -3,8 +3,10 @@ use std::collections::HashMap;
>  use std::env::VarError::{NotPresent, NotUnicode};
>  use std::fs::File;
>  use std::io::{BufRead, BufReader};
> +use std::os::unix::fs::OpenOptionsExt;
>  use std::os::unix::io::FromRawFd;
>  use std::process::Command;
> +use std::sync::OnceLock;
>  
>  use anyhow::{bail, format_err, Context, Error};
>  use serde_json::{json, Value};
> @@ -526,3 +528,46 @@ pub fn place_xdg_file(
>          .and_then(|base| base.place_config_file(file_name).map_err(Error::from))
>          .with_context(|| format!("failed to place {} in xdg home", description))
>  }
> +
> +/// Creates a temporary file (created with O_TMPFILE) in either
> +/// XDG_CACHE_HOME/proxmox-backup if the directories can be created, or in /tmp.
> +pub fn create_tmp_file() -> std::io::Result<std::fs::File> {
> +    static TMP_PATH: OnceLock<std::path::PathBuf> = OnceLock::new();
> +
> +    let tmp_path = TMP_PATH.get_or_init(|| {
> +        let base = match base_directories() {

these base_directories use the prefix 'proxmox-backup', which is not
really needed here since we don't want to actually store the file..

> +            Ok(v) => v,
> +            Err(_) => return std::path::PathBuf::from("/tmp"),
> +        };
> +        let cache_home = base.get_cache_home();

I do wonder whether XDG_RUNTIME_DIR would be a better fit, but tbh, our
use case doesn't really fit either ;) runtime would have the advantage
that the specs basically says it must support O_TMPFILE (not explicitly,
but "The directory MUST by fully-featured by the standards of the
operating system."). the main downside is it might be backed by memory
(but so is the current path "/tmp" on most systems) and on some systems
it might be misconfigured w.r.t. session handling (although such systems
will have plenty of other problems already).

> +
> +        match std::fs::create_dir_all(&cache_home) {

without the prefix, we could just test for existence here, instead of
(potentially) creating a dir structure where we don't even persist anything?

> +            Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => cache_home,
> +            Ok(()) => cache_home,
> +            Err(_) => std::path::PathBuf::from("/tmp"),
> +        }
> +    });
> +
> +    std::fs::OpenOptions::new()
> +        .write(true)
> +        .read(true)
> +        .custom_flags(libc::O_TMPFILE)
> +        .open(tmp_path)

since you don't have a fallback to tmp here, this actually has a chance
of breaking systems with XDG_CACHE_HOME or HOME set, but with the
full resolved path not supporting O_TMPFILE..

> +}
> +
> +#[cfg(test)]
> +mod test {
> +    use std::io::{Read, Seek, Write};
> +
> +    #[test]
> +    fn test_create_tmp_file() -> anyhow::Result<()> {
> +        let mut file = crate::tools::create_tmp_file()?;
> +        const BYTES: &[u8] = b"Hello, World";
> +        file.write(BYTES)?;
> +        let mut buf = [0; BYTES.len()];
> +        file.rewind()?;
> +        file.read(&mut buf)?;
> +        assert_eq!(&buf, BYTES);
> +        Ok(())
> +    }
> +}

this test doesn't really test much other than that the environment in
which it runs has a configured XDG dir or /tmp with write access and
O_TMPFILE support ;) it has the side-effect of potentially creating
directories (outside of the build dir) if they don't exist already,
which might be undesired.