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 3FF68BED3 for ; Fri, 11 Aug 2023 10:52:04 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 19939ADEB for ; Fri, 11 Aug 2023 10:51:34 +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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Fri, 11 Aug 2023 10:51:33 +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 E3F9645F17 for ; Fri, 11 Aug 2023 10:51:32 +0200 (CEST) Date: Fri, 11 Aug 2023 10:51:31 +0200 From: Wolfgang Bumiller To: Gabriel Goller Cc: pbs-devel@lists.proxmox.com Message-ID: References: <20230809101913.81818-1-g.goller@proxmox.com> <20230809101913.81818-2-g.goller@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230809101913.81818-2-g.goller@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.106 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 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: Fri, 11 Aug 2023 08:52:04 -0000 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. > 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. > + 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. > + && 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