From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 4E8EAC611 for ; Mon, 14 Aug 2023 09:42:21 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 2863B1F56E for ; Mon, 14 Aug 2023 09:41:51 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Mon, 14 Aug 2023 09:41:50 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 377364730A for ; Mon, 14 Aug 2023 09:41:50 +0200 (CEST) Message-ID: <56ca2e7e-00ca-d565-dcad-add9a06788c8@proxmox.com> Date: Mon, 14 Aug 2023 09:41:49 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.1 Content-Language: en-US To: Wolfgang Bumiller Cc: pbs-devel@lists.proxmox.com References: <20230809101913.81818-1-g.goller@proxmox.com> <20230809101913.81818-2-g.goller@proxmox.com> From: Gabriel Goller In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.697 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 NICE_REPLY_A -2.265 Looks like a legit reply (A) 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. [create.rs] Subject: Re: [pbs-devel] [PATCH proxmox-backup v3 1/1] fix #4380: check if file is excluded before running `stat()` 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: , X-List-Received-Date: Mon, 14 Aug 2023 07:42:21 -0000 On 8/11/23 10:51, Wolfgang Bumiller wrote: > On Wed, Aug 09, 2023 at 12:19:13PM +0200, Gabriel Goller wrote: >> Before running `stat()` we now make a `matches_path()` call, which checks > I was more thinking to use `matches_path()` only if `stat()` actually > fails and then propagate `stat()`'s error (instead of the error that a > file mode is required... ;-) ) > That way we wouldn't match twice in the other case. I used this approach so that we don't `stat` every excluded files as well (I think `matches_path` is probably faster than `stat`). Also we will very rarely make the matching twice, because most of the excluded paths come from the `--exclude` parameter or the `.pxarexclude` file, which both don't allow to set the `file_mode`. >> if the file/directory is excluded (`matches_path()` tries to match using >> only the path, without the file mode). If the file is not excluded, >> we `stat()` and then check again if the path matches using the file_mode and >> the `matches()` function. >> Added `pathpatterns` crate to local overrides in cargo.toml. >> >> changes v2: >> - checking for `read` and `execute` permissions before entering directory, >> doesn't work because there are a lot of side-effects (executed by >> different user, AppArmor, SELinux, ...). >> changes v1: >> - checking for excluded files with `matches()` before executing `stat()`, >> this doesn't work because we get the file_mode from `stat()` and don't >> want to ignore it when matching. >> >> Signed-off-by: Gabriel Goller >> --- >> Cargo.toml | 1 + >> pbs-client/src/pxar/create.rs | 45 ++++++++++++++++++++++++++--------- >> 2 files changed, 35 insertions(+), 11 deletions(-) >> >> diff --git a/Cargo.toml b/Cargo.toml >> index 2a05c471..c54bc28b 100644 >> --- a/Cargo.toml >> +++ b/Cargo.toml >> @@ -262,6 +262,7 @@ proxmox-rrd.workspace = true >> >> #proxmox-apt = { path = "../proxmox-apt" } >> #proxmox-openid = { path = "../proxmox-openid-rs" } >> +#pathpatterns = {path = "../pathpatterns" } >> >> #pxar = { path = "../pxar" } >> >> diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs >> index 2577cf98..a6d608e7 100644 >> --- a/pbs-client/src/pxar/create.rs >> +++ b/pbs-client/src/pxar/create.rs >> @@ -232,7 +232,7 @@ impl Archiver { >> let entry_counter = self.entry_counter; >> >> let old_patterns_count = self.patterns.len(); >> - self.read_pxar_excludes(dir.as_raw_fd())?; >> + self.read_pxar_excludes(&mut dir)?; >> >> let mut file_list = self.generate_directory_file_list(&mut dir, is_root)?; >> >> @@ -315,8 +315,23 @@ impl Archiver { >> } >> } >> >> - fn read_pxar_excludes(&mut self, parent: RawFd) -> Result<(), Error> { >> - let fd = match self.open_file(parent, c_str!(".pxarexclude"), OFlag::O_RDONLY, false)? { >> + fn read_pxar_excludes(&mut self, parent: &mut Dir) -> Result<(), Error> { >> + let mut exclude_file_found = false; >> + for file in parent.iter() { >> + if file?.file_name().to_str()? == ".pxarexclude" { >> + exclude_file_found = true; >> + break; >> + } >> + } >> + if !exclude_file_found { > Why are we adding this check? > It's not necessary. It is not strictly necessary, but the problem is that the function tries to open `.pxarexclude` in every directory. Also directories were we have no access to, which will obviously fail. So if we have a directory with limited permissions, (regardless if a `.pxarexclude` lives in it or not) we will get an error `failed to open .pxarexclude file` (even though it doesn't even exist). >> + return Ok(()); >> + } >> + let fd = match self.open_file( >> + parent.as_raw_fd(), >> + c_str!(".pxarexclude"), >> + OFlag::O_RDONLY, >> + false, >> + )? { >> Some(fd) => fd, >> None => return Ok(()), > ^ This already means the file does not exist. > >> }; >> @@ -420,7 +435,7 @@ impl Archiver { >> for file in dir.iter() { >> let file = file?; >> >> - let file_name = file.file_name().to_owned(); >> + let file_name = file.file_name(); >> let file_name_bytes = file_name.to_bytes(); >> if file_name_bytes == b"." || file_name_bytes == b".." { >> continue; >> @@ -434,9 +449,17 @@ impl Archiver { >> assert_single_path_component(os_file_name)?; >> let full_path = self.path.join(os_file_name); >> >> + let match_path = PathBuf::from("/").join(full_path.clone()); >> + let pre_stat_match = self >> + .patterns >> + .matches_path(match_path.as_os_str().as_bytes()); > As said above, I'd like this to instead happen in stat's `Err() => {}` > case. > >> + if pre_stat_match == Ok(Some(MatchType::Exclude)) { >> + continue; >> + } >> + >> let stat = match nix::sys::stat::fstatat( >> dir_fd, >> - file_name.as_c_str(), >> + file_name.to_owned().as_c_str(), >> nix::fcntl::AtFlags::AT_SYMLINK_NOFOLLOW, >> ) { >> Ok(stat) => stat, >> @@ -444,11 +467,11 @@ impl Archiver { >> Err(err) => return Err(err).context(format!("stat failed on {:?}", full_path)), >> }; >> >> - let match_path = PathBuf::from("/").join(full_path.clone()); >> - if self >> - .patterns >> - .matches(match_path.as_os_str().as_bytes(), Some(stat.st_mode)) >> - == Some(MatchType::Exclude) >> + if pre_stat_match.is_err() > And then this won't be needed and AFAICT isn't correct with the current > implementation of `matches_path()` of this series since it might have > found a match *after* one of a higher precedence would have already > matched potentially differently. I'll fix the implementation of `matches_path()` so that it actually fails (returns `Err`) when a pattern with a `file_mode` is encountered (as it was intended). Otherwise this should work, as I didn't change anything (except the preceding `matches_path()`). >> + && self >> + .patterns >> + .matches(match_path.as_os_str().as_bytes(), Some(stat.st_mode)) >> + == Some(MatchType::Exclude) >> { >> continue; >> } >> @@ -462,7 +485,7 @@ impl Archiver { >> } >> >> file_list.push(FileListEntry { >> - name: file_name, >> + name: file_name.to_owned(), >> path: full_path, >> stat, >> }); >> -- >> 2.39.2