all lists on lists.proxmox.com
 help / color / mirror / Atom feed
From: Wolfgang Bumiller <w.bumiller@proxmox.com>
To: Mira Limbeck <m.limbeck@proxmox.com>
Cc: pbs-devel@lists.proxmox.com
Subject: Re: [pbs-devel] [PATCH v4 proxmox] Add tempfile() helper function
Date: Mon, 17 Aug 2020 09:41:04 +0200	[thread overview]
Message-ID: <20200817074104.codqb6fm2yozhs2k@olga.proxmox.com> (raw)
In-Reply-To: <20200814150107.7425-1-m.limbeck@proxmox.com>

On Fri, Aug 14, 2020 at 05:01:06PM +0200, Mira Limbeck wrote:
> The tempfile() helper function tries to create a temporary file in /tmp
> with the O_TMPFILE option. If that fails it falls back to using
> mkstemp(). This happens in /tmp/proxmox-<UID> which is either created,
> or if it already exists, checked for the right owner and permissions.
> 
> As O_TMPFILE was introduced in kernel 3.11 this fallback can help with
> CentOS 7 and its 3.10 kernel as well as with WSL (Windows Subsystem for
> Linux).
> 
> Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
> ---
> v4:
>  - changed directory from proxmox-backup-<UID> to proxmox-<UID>
>  - added check for owner and permissions
> v3:
>  - O_TMPFILE support is tested on first run of tempfile()
>  - EISDIR is handled specifically to test for O_TMPFILE support
>  - AtomicBool is used as it provides a safe interface, but 'static mut'
>      could also be used
>  - mkstemp() now creates the tempfile in a subdirectory called
>    proxmox-backup-<UID>
> 
>  proxmox/src/tools/fs.rs | 77 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 75 insertions(+), 2 deletions(-)
> 
> diff --git a/proxmox/src/tools/fs.rs b/proxmox/src/tools/fs.rs
> index b1a95b5..7e13ede 100644
> --- a/proxmox/src/tools/fs.rs
> +++ b/proxmox/src/tools/fs.rs
> @@ -1,17 +1,20 @@
>  //! File related utilities such as `replace_file`.
>  
>  use std::ffi::CStr;
> -use std::fs::{File, OpenOptions};
> +use std::fs::{DirBuilder, File, OpenOptions};
>  use std::io::{self, BufRead, BufReader, Write};
> +use std::os::unix::fs::{DirBuilderExt, MetadataExt, OpenOptionsExt, PermissionsExt};
>  use std::os::unix::io::{AsRawFd, FromRawFd, RawFd};
>  use std::path::Path;
> +use std::sync::atomic::{AtomicBool, Ordering};
>  use std::time::Duration;
>  
>  use anyhow::{bail, format_err, Error};
> +use lazy_static::lazy_static;
>  use nix::errno::Errno;
>  use nix::fcntl::OFlag;
>  use nix::sys::stat;
> -use nix::unistd::{self, Gid, Uid};
> +use nix::unistd::{self, geteuid, mkstemp, unlink, Gid, Uid};
>  use serde_json::Value;
>  
>  use crate::sys::error::SysResult;
> @@ -518,3 +521,73 @@ pub fn open_file_locked<P: AsRef<Path>>(path: P, timeout: Duration) -> Result<Fi
>          Err(err) => bail!("Unable to acquire lock {:?} - {}", path, err),
>      }
>  }
> +
> +static O_TMPFILE_SUPPORT: AtomicBool = AtomicBool::new(true);

Which value we read here has no real influence on functionality, and we
only ever write false to it, so IMO this doesn't even need to be atomic,
a `static mut ...: bool` should be sufficient.

> +lazy_static! {
> +    static ref MKSTEMP_PATH: String = {
> +        let uid = geteuid();
> +        format!("/tmp/proxmox-{}", uid)
> +    };
> +    static ref MKSTEMP_FILE: String = { format!("{}/tmpfile_XXXXXX", MKSTEMP_PATH.as_str()) };
> +}
> +
> +/// Create a new tempfile by using O_TMPFILE with a fallback to mkstemp() if it fails (e.g. not supported).

^ needs a line break

> +pub fn tempfile() -> Result<File, Error> {
> +    if O_TMPFILE_SUPPORT.load(Ordering::Relaxed) {

Seeing the indentation I somehow feel like the `if` here should just
forward to two separate private `fn()`s, also because it's easier to
fill the error context if you do:

    if <can_use_X> {
        x()
    } else {
        y()
    }
    .map_err(|e| format_err!("error creating tempfile: {}", e))?

That way you also cover the two cases you "silently" omitted via '?':
the `mkstemp()` call and the `unlink()` call (the latter really should
have additional context though, because if that one fails the error
messages will be confusing).

> +        match std::fs::OpenOptions::new()
> +            .write(true)
> +            .read(true)
> +            .custom_flags(libc::O_TMPFILE)
> +            .open("/tmp")
> +        {
> +            Ok(file) => return Ok(file),

The `Err` case could be a little more readable:

                Err(ref err) if err.raw_os_error() == Some(libc::EISDIR) => {
                    O_TMPFILE_SUPPORT.store(false, Ordering::Relaxed);
                }
                Err(err) => bail!("creating tempfile failed: {}", err),

> +            Err(err) => {
> +                let raw_os_error = match err.raw_os_error() {
> +                    Some(v) => v,
> +                    None => -1,
> +                };
> +                if raw_os_error == 21 {

^ hardcoding 21 here is a no-go, use libc::EISDIR

> +                    O_TMPFILE_SUPPORT.store(false, Ordering::Relaxed);
> +                    eprintln!(
> +                        "Error creating tempfile: 'EISDIR', falling back to mkstemp() instead",
> +                    );

Do we really need to be verbose about which kind of temp file support
we're using? And if so, does it have to be `eprintln`? I think maybe we
should add a way to the `proxmox` crate to register a logging mechanism
and use an 'info' or 'debug' level for this kind of information?

> +                } else {
> +                    bail!("creating tempfile failed: '{}'", err);
> +                }
> +            }
> +        }
> +    }
> +
> +    match DirBuilder::new().mode(0o700).create(MKSTEMP_PATH.as_str()) {
> +        Err(err) => {

Similarly this could start with

           Err(ref err) if !err.already_exists() => {
               bail!("creating directory failed: '{}': {}", MKSTEMP_PATH.as_str(), e);
           }
           Err(err) => {
               // drop one level of indentation in the entire 'else' case
           }

> +            if err.kind() != std::io::ErrorKind::AlreadyExists {
> +                bail!("creating directory failed: '{}'", MKSTEMP_PATH.as_str());

^ The message should also contain the actual error for clarity.

> +            } else {
> +                // check owner
> +                let metadata = std::fs::metadata(MKSTEMP_PATH.as_str())?;
> +                if metadata.uid() != geteuid().as_raw() {
> +                    bail!(
> +                        "directory '{}' has wrong owner: {}",
> +                        MKSTEMP_PATH.as_str(),
> +                        metadata.uid()
> +                    );
> +                }
> +
> +                // check permissions
> +                let perm = metadata.permissions();
> +                if (perm.mode() & 0o077) != 0 {
> +                    bail!(
> +                        "directory '{}' already exists with wrong permissions: {:o}",

I think it's sufficient to just say "directory {} has invalid
permissions" - the "already exists" is implied by the fact that it has
permissions :P, and the *current* permissions aren't relevant, only the
ones we *want* it to have (otherwise how is the user supposed to fix it
if they see this error? ;-) )

> +                        MKSTEMP_PATH.as_str(),
> +                        perm.mode() & 0o777
> +                    );
> +                }
> +            }
> +        }
> +        _ => {}
> +    }
> +    let (fd, path) = mkstemp(MKSTEMP_FILE.as_str())?;
> +    unlink(path.as_path())?;

^ .map_err() here, and more importantly, the '?' is leaking an `fd`, so:

> +    let file = unsafe { File::from_raw_fd(fd) };

^ must be moved above the unlink call

> +    Ok(file)
> +}
> -- 
> 2.20.1




      parent reply	other threads:[~2020-08-17  7:41 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-14 15:01 Mira Limbeck
2020-08-14 15:01 ` [pbs-devel] [PATCH v4 proxmox-backup] Replace all occurences of open() with O_TMPFILE Mira Limbeck
2020-08-17  7:41 ` Wolfgang Bumiller [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200817074104.codqb6fm2yozhs2k@olga.proxmox.com \
    --to=w.bumiller@proxmox.com \
    --cc=m.limbeck@proxmox.com \
    --cc=pbs-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal