From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <pbs-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9])
	by lore.proxmox.com (Postfix) with ESMTPS id 566DF1FF16F
	for <inbox@lore.proxmox.com>; Fri, 29 Nov 2024 08:57:47 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
	by firstgate.proxmox.com (Proxmox) with ESMTP id 3534F12ED3;
	Fri, 29 Nov 2024 08:57:49 +0100 (CET)
Date: Fri, 29 Nov 2024 08:57:16 +0100 (CET)
From: =?UTF-8?Q?Fabian_Gr=C3=BCnbichler?= <f.gruenbichler@proxmox.com>
To: Proxmox Backup Server development discussion <pbs-devel@lists.proxmox.com>, 
 Dominik Csapak <d.csapak@proxmox.com>
Message-ID: <771443181.1837.1732867036375@webmail.proxmox.com>
In-Reply-To: <20241128145440.4119007-1-d.csapak@proxmox.com>
References: <20241128145440.4119007-1-d.csapak@proxmox.com>
MIME-Version: 1.0
X-Priority: 3
Importance: Normal
X-Mailer: Open-Xchange Mailer v7.10.6-Rev70
X-Originating-Client: open-xchange-appsuite
X-SPAM-LEVEL: Spam detection results:  0
 AWL 0.048 Adjusted score from AWL reputation of From: address
 BAYES_00                 -1.9 Bayes spam probability is 0 to 1%
 DMARC_MISSING             0.1 Missing DMARC policy
 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
 Validity was blocked. See
 https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
 information.
 RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
 Validity was blocked. See
 https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
 information.
 RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
 Validity was blocked. See
 https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
 information.
 SPF_HELO_NONE           0.001 SPF: HELO does not publish an SPF Record
 SPF_PASS               -0.001 SPF: sender matches SPF record
 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See
 http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more
 information. [proxmox.com, file.rs]
Subject: Re: [pbs-devel] [PATCH proxmox] sys: fs: set FD_CLOEXEC when
 creating temp files
X-BeenThere: pbs-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Backup Server development discussion
 <pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>, 
 <mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>, 
 <mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox Backup Server development discussion
 <pbs-devel@lists.proxmox.com>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pbs-devel-bounces@lists.proxmox.com
Sender: "pbs-devel" <pbs-devel-bounces@lists.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