[pbs-devel] [PATCH proxmox-backup v5] fix #4380: check if file is excluded before running `stat()`
Gabriel Goller
g.goller at proxmox.com
Fri Aug 18 16:32:12 CEST 2023
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 at 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
More information about the pbs-devel
mailing list