From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 566DF1FF16F for ; 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?= To: Proxmox Backup Server development discussion , Dominik Csapak 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" > Dominik Csapak 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 > --- > 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>( > 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