Re: [pbs-devel] [PATCH proxmox] sys: fs: set FD_CLOEXEC when creating temp files

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

> Dominik Csapak <d.csapak@proxmox.com> hat am 28.11.2024 15:54 CET geschrieben:
> 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 FD_CLOEXEC after getting the filehandle.
> 
> This fixes an issue with e.g. tape backups not working because of such
> lingering lock files after a reload.

and also one that "leaked" an additional FD for every proxmox-backup-proxy reload via the RRD journal files - so this fixes a bug where PBS will eventually run into the open file limits if you keep reloading that service without ever stopping or restarting it.

might be a good addition to the commit message :)

> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> there are other code parts where we open file without CLOEXEC, but
> wanted to send this for now.
> 
>  proxmox-sys/src/fs/file.rs | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/proxmox-sys/src/fs/file.rs b/proxmox-sys/src/fs/file.rs
> index fbfc0b58..05d0aff0 100644
> --- a/proxmox-sys/src/fs/file.rs
> +++ b/proxmox-sys/src/fs/file.rs
> @@ -7,7 +7,7 @@ use std::time::Duration;
>  
>  use anyhow::{bail, format_err, Context as _, Error};
>  use nix::errno::Errno;
> -use nix::fcntl::OFlag;
> +use nix::fcntl::{FcntlArg, FdFlag, OFlag};
>  use nix::sys::stat;
>  use nix::unistd;
>  use nix::NixPath;
> @@ -128,7 +128,10 @@ pub fn make_tmp_file<P: AsRef<Path>>(
>      let mut template = path.to_owned();
>      template.set_extension("tmp_XXXXXX");
>      let (mut file, tmp_path) = match unistd::mkstemp(&template) {
> -        Ok((fd, path)) => (unsafe { File::from_raw_fd(fd) }, path),
> +        Ok((fd, path)) => {
> +            nix::fcntl::fcntl(fd, FcntlArg::F_SETFD(FdFlag::FD_CLOEXEC))?;
> +            (unsafe { File::from_raw_fd(fd) }, path)
> +        }

unfortunately, this is still racy since the FD is open with O_CLOEXEC between the unistd::mkstemp and the fcntl - see the man page of fcntl which explicitly calls this out:

"In  multithreaded programs, using fcntl() F_SETFD to set the close-on-exec flag at the same time as another thread performs a fork(2) plus execve(2) is vulnerable to a race condition that may unintentionally leak the file descriptor to the program executed in the child process."

we could use libc::mkostemp (unsafe, path/template+flags -> raw fd or error as c_int) instead? and/or we could write a wrapper around it and propose it upstream for nix inclusion? ;) but since this seems to be the only place where we call mkstemp..

>          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