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