[pbs-devel] [PATCH proxmox-backup v7] fix #4380: check if file is excluded before running `stat()`
Gabriel Goller
g.goller at proxmox.com
Tue Aug 22 16:05:17 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. As we updated the `matches()` function, we also updated all the
invocations of it.
Added `pathpatterns` crate to local overrides in cargo.toml.
Signed-off-by: Gabriel Goller <g.goller at proxmox.com>
---
changes v6:
- if the file has been there previously, but was then removed,
we ignore it.
changes v5:
- updated all invocations of `matches()`
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 | 5 ++--
pbs-client/src/catalog_shell.rs | 8 ++---
pbs-client/src/pxar/create.rs | 53 ++++++++++++++++++++++-----------
pbs-client/src/pxar/extract.rs | 14 +++++----
pbs-datastore/src/catalog.rs | 6 ++--
5 files changed, 54 insertions(+), 32 deletions(-)
diff --git a/Cargo.toml b/Cargo.toml
index 5cbae1b8..560794a4 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -264,8 +264,9 @@ proxmox-rrd.workspace = true
#proxmox-sortable-macro = { path = "../proxmox/proxmox-sortable-macro" }
#proxmox-human-byte = { path = "../proxmox/proxmox-human-byte" }
-#proxmox-apt = { path = "../proxmox/proxmox-apt" }
-#proxmox-openid = { path = "../proxmox/proxmox-openid" }
+#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 b8aaf8cb..f53b3cc5 100644
--- a/pbs-client/src/catalog_shell.rs
+++ b/pbs-client/src/catalog_shell.rs
@@ -1138,14 +1138,14 @@ 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) {
(_, DirEntryAttribute::Directory { .. }) => {
- self.handle_new_directory(entry, match_result).await?;
+ self.handle_new_directory(entry, match_result?).await?;
}
(true, DirEntryAttribute::File { .. }) => {
self.dir_stack.push(PathStackEntry::new(entry));
diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
index 2577cf98..2a539957 100644
--- a/pbs-client/src/pxar/create.rs
+++ b/pbs-client/src/pxar/create.rs
@@ -16,12 +16,12 @@ use nix::fcntl::OFlag;
use nix::sys::stat::{FileStat, Mode};
use pathpatterns::{MatchEntry, MatchFlag, MatchList, MatchType, PatternFlag};
+use proxmox_sys::error::SysError;
use pxar::encoder::{LinkOffset, SeqWrite};
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 +420,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 +434,42 @@ 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());
+
+ let mut stat_results: Option<FileStat> = None;
+
+ let get_file_mode = || {
+ nix::sys::stat::fstatat(
+ dir_fd,
+ file_name.to_owned().as_c_str(),
+ nix::fcntl::AtFlags::AT_SYMLINK_NOFOLLOW,
+ )
};
- let match_path = PathBuf::from("/").join(full_path.clone());
- if self
+ match self
.patterns
- .matches(match_path.as_os_str().as_bytes(), Some(stat.st_mode))
- == Some(MatchType::Exclude)
- {
- continue;
+ .matches(match_path.as_os_str().as_bytes(), || {
+ Ok::<_, Errno>(match &stat_results {
+ Some(result) => result.st_mode,
+ None => stat_results.insert(get_file_mode()?).st_mode,
+ })
+ }) {
+ Ok(Some(MatchType::Exclude)) => continue,
+ Ok(_) => (),
+ Err(err) => {
+ if err.not_found() {
+ continue;
+ } else {
+ return Err(err).with_context(|| format!("stat failed on {:?}", full_path));
+ }
+ }
}
+ let stat = stat_results
+ .map(Ok)
+ .unwrap_or_else(get_file_mode)
+ .with_context(|| format!("stat failed on {:?}", full_path))?;
+
self.entry_counter += 1;
if self.entry_counter > self.entry_limit {
bail!(
@@ -462,7 +479,7 @@ impl Archiver {
}
file_list.push(FileListEntry {
- name: file_name,
+ name: file_name.to_owned(),
path: full_path,
stat,
});
@@ -533,7 +550,7 @@ impl Archiver {
let match_path = PathBuf::from("/").join(self.path.clone());
if self
.patterns
- .matches(match_path.as_os_str().as_bytes(), Some(stat.st_mode))
+ .matches(match_path.as_os_str().as_bytes(), stat.st_mode)?
== Some(MatchType::Exclude)
{
return Ok(());
diff --git a/pbs-client/src/pxar/extract.rs b/pbs-client/src/pxar/extract.rs
index 4eb6fb90..1e2e901a 100644
--- a/pbs-client/src/pxar/extract.rs
+++ b/pbs-client/src/pxar/extract.rs
@@ -251,15 +251,19 @@ where
self.extractor.set_path(entry.path().as_os_str().to_owned());
- let match_result = self.match_list.matches(
- entry.path().as_os_str().as_bytes(),
- Some(metadata.file_type() as u32),
- );
+ // We can `unwrap()` safely here because we get a `Result<_, std::convert::Infallible>`
+ let match_result = self
+ .match_list
+ .matches(
+ entry.path().as_os_str().as_bytes(),
+ metadata.file_type() as u32,
+ )
+ .unwrap();
let did_match = match match_result {
Some(MatchType::Include) => true,
Some(MatchType::Exclude) => false,
- None => self.state.current_match,
+ _ => self.state.current_match,
};
let extract_res = match (did_match, entry.kind()) {
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