[pbs-devel] [PATCH proxmox-backup v5] fix #4380: check if file is excluded before running `stat()`

[Gabriel Goller <g.goller@proxmox.com>]

Passed a closure with the `stat()` function call to `matches()`. This
will traverse through all patterns and try to match using the path only, if a
`file_mode` is needed, it will run the closure. This means that if we exclude
a file with the `MatchType::ANY_FILE_TYPE`, we will skip it without running
`stat()` on it.
Added `pathpatterns` crate to local overrides in cargo.toml.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---

changes v4:
 - match only by path and exclude the matched files, the run `stat()` and
   match again, this time using the `file_mode`. This will match everything 
   twice in the worst case, which is not optimal.
changes v3:
 - 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 v2:
 - 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.

 Cargo.toml                      |  1 +
 pbs-client/src/catalog_shell.rs | 12 +++---
 pbs-client/src/pxar/create.rs   | 69 ++++++++++++++++++++++++---------
 pbs-client/src/pxar/extract.rs  |  9 +++--
 pbs-datastore/src/catalog.rs    |  6 +--
 5 files changed, 65 insertions(+), 32 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/catalog_shell.rs b/pbs-client/src/catalog_shell.rs
index 98af5699..89bcc280 100644
--- a/pbs-client/src/catalog_shell.rs
+++ b/pbs-client/src/catalog_shell.rs
@@ -13,7 +13,7 @@ use nix::dir::Dir;
 use nix::fcntl::OFlag;
 use nix::sys::stat::Mode;
 
-use pathpatterns::{MatchEntry, MatchList, MatchPattern, MatchType, PatternFlag};
+use pathpatterns::{FileModeRequired, MatchEntry, MatchList, MatchPattern, MatchType, PatternFlag};
 use proxmox_router::cli::{self, CliCommand, CliCommandMap, CliHelper, CommandLineInterface};
 use proxmox_schema::api;
 use proxmox_sys::fs::{create_path, CreateOptions};
@@ -1108,7 +1108,7 @@ impl<'a> ExtractorState<'a> {
     async fn handle_new_directory(
         &mut self,
         entry: catalog::DirEntry,
-        match_result: Option<MatchType>,
+        match_result: Result<Option<MatchType>, FileModeRequired>,
     ) -> Result<(), Error> {
         // enter a new directory:
         self.read_dir_stack.push(mem::replace(
@@ -1123,7 +1123,7 @@ impl<'a> ExtractorState<'a> {
         Shell::walk_pxar_archive(self.accessor, &mut self.dir_stack).await?;
         let dir_pxar = self.dir_stack.last().unwrap().pxar.as_ref().unwrap();
         let dir_meta = dir_pxar.entry().metadata().clone();
-        let create = self.matches && match_result != Some(MatchType::Exclude);
+        let create = self.matches && match_result != Ok(Some(MatchType::Exclude));
         self.extractor
             .enter_directory(dir_pxar.file_name().to_os_string(), dir_meta, create)?;
 
@@ -1133,9 +1133,9 @@ impl<'a> ExtractorState<'a> {
     pub async fn handle_entry(&mut self, entry: catalog::DirEntry) -> Result<(), Error> {
         let match_result = self.match_list.matches(&self.path, entry.get_file_mode());
         let did_match = match match_result {
-            Some(MatchType::Include) => true,
-            Some(MatchType::Exclude) => false,
-            None => self.matches,
+            Ok(Some(MatchType::Include)) => true,
+            Ok(Some(MatchType::Exclude)) => false,
+            _ => self.matches,
         };
 
         match (did_match, &entry.attr) {
diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
index 2577cf98..4a844599 100644
--- a/pbs-client/src/pxar/create.rs
+++ b/pbs-client/src/pxar/create.rs
@@ -21,7 +21,6 @@ use pxar::Metadata;
 
 use proxmox_io::vec;
 use proxmox_lang::c_str;
-use proxmox_sys::error::SysError;
 use proxmox_sys::fs::{self, acl, xattr};
 
 use pbs_datastore::catalog::BackupCatalogWriter;
@@ -420,7 +419,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,25 +433,57 @@ impl Archiver {
             assert_single_path_component(os_file_name)?;
             let full_path = self.path.join(os_file_name);
 
-            let stat = match nix::sys::stat::fstatat(
-                dir_fd,
-                file_name.as_c_str(),
-                nix::fcntl::AtFlags::AT_SYMLINK_NOFOLLOW,
-            ) {
-                Ok(stat) => stat,
-                Err(ref err) if err.not_found() => continue,
-                Err(err) => return Err(err).context(format!("stat failed on {:?}", full_path)),
-            };
-
             let match_path = PathBuf::from("/").join(full_path.clone());
-            if self
+
+            let mut stat_results: Option<FileStat> = None;
+
+            let stat = self
                 .patterns
-                .matches(match_path.as_os_str().as_bytes(), Some(stat.st_mode))
-                == Some(MatchType::Exclude)
-            {
+                .matches(
+                    match_path.as_os_str().as_bytes(),
+                    || match nix::sys::stat::fstatat(
+                        dir_fd,
+                        file_name.to_owned().as_c_str(),
+                        nix::fcntl::AtFlags::AT_SYMLINK_NOFOLLOW,
+                    ) {
+                        Ok(stat) => {
+                            stat_results = Some(stat);
+                            Ok(stat.st_mode)
+                        }
+                        Err(e) => Err(e),
+                    },
+                )
+                .map_err(|err| {
+                    anyhow::Error::new(err).context(format!(
+                        "stat failed on {:?} with {:?}",
+                        full_path,
+                        err.desc()
+                    ))
+                })?;
+
+            if stat == Some(MatchType::Exclude) {
                 continue;
             }
 
+            if stat_results.is_none() {
+                match nix::sys::stat::fstatat(
+                    dir_fd,
+                    file_name.to_owned().as_c_str(),
+                    nix::fcntl::AtFlags::AT_SYMLINK_NOFOLLOW,
+                ) {
+                    Ok(stat) => {
+                        stat_results = Some(stat);
+                    }
+                    Err(err) => {
+                        return Err(err).context(format!(
+                            "stat failed on {:?} with {:?}",
+                            full_path,
+                            err.desc()
+                        ));
+                    }
+                }
+            }
+
             self.entry_counter += 1;
             if self.entry_counter > self.entry_limit {
                 bail!(
@@ -462,9 +493,9 @@ impl Archiver {
             }
 
             file_list.push(FileListEntry {
-                name: file_name,
+                name: file_name.to_owned(),
                 path: full_path,
-                stat,
+                stat: stat_results.unwrap(),
             });
         }
 
@@ -534,7 +565,7 @@ impl Archiver {
         if self
             .patterns
             .matches(match_path.as_os_str().as_bytes(), Some(stat.st_mode))
-            == Some(MatchType::Exclude)
+            == Ok(Some(MatchType::Exclude))
         {
             return Ok(());
         }
diff --git a/pbs-client/src/pxar/extract.rs b/pbs-client/src/pxar/extract.rs
index 4dbaf52d..e08af3c0 100644
--- a/pbs-client/src/pxar/extract.rs
+++ b/pbs-client/src/pxar/extract.rs
@@ -244,16 +244,17 @@ where
         );
 
         let did_match = match match_result {
-            Some(MatchType::Include) => true,
-            Some(MatchType::Exclude) => false,
-            None => self.state.current_match,
+            Ok(Some(MatchType::Include)) => true,
+            Ok(Some(MatchType::Exclude)) => false,
+            _ => self.state.current_match,
         };
 
         let extract_res = match (did_match, entry.kind()) {
             (_, EntryKind::Directory) => {
                 self.callback(entry.path());
 
-                let create = self.state.current_match && match_result != Some(MatchType::Exclude);
+                let create =
+                    self.state.current_match && match_result != Ok(Some(MatchType::Exclude));
                 let res = self
                     .extractor
                     .enter_directory(file_name_os.to_owned(), metadata.clone(), create)
diff --git a/pbs-datastore/src/catalog.rs b/pbs-datastore/src/catalog.rs
index 11c14b64..86e20c92 100644
--- a/pbs-datastore/src/catalog.rs
+++ b/pbs-datastore/src/catalog.rs
@@ -678,9 +678,9 @@ impl<R: Read + Seek> CatalogReader<R> {
             }
             file_path.extend(&e.name);
             match match_list.matches(&file_path, e.get_file_mode()) {
-                Some(MatchType::Exclude) => continue,
-                Some(MatchType::Include) => callback(file_path)?,
-                None => (),
+                Ok(Some(MatchType::Exclude)) => continue,
+                Ok(Some(MatchType::Include)) => callback(file_path)?,
+                _ => (),
             }
             if is_dir {
                 self.find(&e, file_path, match_list, callback)?;
-- 
2.39.2