Re: [pbs-devel] [PATCH proxmox v2 1/2] sys: fs: set CLOEXEC when creating temp files

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

Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>

On November 29, 2024 3:28 pm, Dominik Csapak wrote:
> In general we want all open files to have set CLOEXEC since our
> reloading mechanism can basically fork at any moment and we don't want
> newer daemons to carry around old file descriptors, especially lock
> files.
> 
> Since `make_tmp_file` is called by many things (e.g. open_file_locked,
> logrotate, rrd), set O_CLOEXEC with mkostemp.
> 
> This fixes issues with leftover file descriptors e.g. tape backups not
> working because of lingering locks after a reload, or having deleted
> rrd files open.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> changes from v1:
> * introduce mkostemp helper which is similar to nix's mkstemp helper
>   (the code is a copy of mkstemp aside from the call to libcmkostemp +
>   the oflag handling)
> 
>   I did it this way, since we may be able to upstream this, have
>   to look more closer at this though.
> 
>  proxmox-sys/src/fs/file.rs | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/proxmox-sys/src/fs/file.rs b/proxmox-sys/src/fs/file.rs
> index fbfc0b58..74b9e74e 100644
> --- a/proxmox-sys/src/fs/file.rs
> +++ b/proxmox-sys/src/fs/file.rs
> @@ -116,6 +116,29 @@ pub fn file_read_firstline<P: AsRef<Path>>(path: P) -> Result<String, Error> {
>      read_firstline(path).map_err(|err| format_err!("unable to read {path:?} - {err}"))
>  }
>  
> +#[inline]
> +/// Creates a tmpfile like [`nix::unistd::mkstemp`], but with [`nix::fctnl::Oflag`] set.
> +///
> +/// Note that some flags are masked out since they can produce an error, see mkostemp(2) for details.
> +// code is mostly copied from nix mkstemp
> +fn mkostemp<P: ?Sized + NixPath>(
> +    template: &P,
> +    oflag: OFlag,
> +) -> nix::Result<(std::os::fd::RawFd, PathBuf)> {
> +    use std::os::unix::ffi::OsStringExt;
> +    let mut path = template.with_nix_path(|path| path.to_bytes_with_nul().to_owned())?;
> +    let p = path.as_mut_ptr().cast();
> +
> +    let flags = OFlag::intersection(OFlag::O_APPEND | OFlag::O_CLOEXEC | OFlag::O_SYNC, oflag);
> +
> +    let fd = unsafe { libc::mkostemp(p, flags.bits()) };
> +    let last = path.pop(); // drop the trailing nul
> +    debug_assert!(last == Some(b'\0'));
> +    let pathname = std::ffi::OsString::from_vec(path);
> +    Errno::result(fd)?;
> +    Ok((fd, PathBuf::from(pathname)))
> +}
> +
>  /// Takes a Path and CreateOptions, creates a tmpfile from it and returns
>  /// a RawFd and PathBuf for it
>  pub fn make_tmp_file<P: AsRef<Path>>(
> @@ -127,7 +150,7 @@ pub fn make_tmp_file<P: AsRef<Path>>(
>      // use mkstemp here, because it works with different processes, threads, even tokio tasks
>      let mut template = path.to_owned();
>      template.set_extension("tmp_XXXXXX");
> -    let (mut file, tmp_path) = match unistd::mkstemp(&template) {
> +    let (mut file, tmp_path) = match mkostemp(&template, OFlag::O_CLOEXEC) {
>          Ok((fd, path)) => (unsafe { File::from_raw_fd(fd) }, path),
>          Err(err) => bail!("mkstemp {:?} failed: {}", template, err),
>      };
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel