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

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

On 8/21/23 10:41, Wolfgang Bumiller wrote:
> needs a rebase, and comments inline:
>
> On Fri, Aug 18, 2023 at 04:32:12PM +0200, Gabriel Goller wrote:
>> 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>,
> ^ This is not needed, let's not take errors with us throughout the
> process.
>
> This is our catalog shell. The catalog file always contains file types
> for its contents except for hardlinks, since we cannot resolve them.
> However, hardlinks also cannot recurse down any further, since they're
> also never directories, so `handle_new_directory` will never actually
> get `Err()` passed here.
>
>>       ) -> 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));
> ^ so no need for this change
>
>>           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());
> I'd like to point out that if we don't know what to do, we should fail,
> so, just use `?` here.
>
> Since this is an interactive tool, we *may* get away with just logging
> this and continuing with the matches you wrote out below here.
>
> But we should not just be silent about it :-)
>
> Personally, I'm fine with erroring and telling the user to investigate.
> In practice I doubt anyone will really run into this. The catalog shell
> is too awkward to have many users (I think???)
How about we introduce this log message, then use the question mark 
operator?

     match (did_match, &entry.attr) {
         (_, DirEntryAttribute::Directory { .. }) => {
             if match_result.is_err() {
                 log::error!("error while matching file paths");
             }
             self.handle_new_directory(entry, match_result?).await?;
         }
         ...
     }
>>           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;
> Please factor the duplicate `fstatat` call out up here into a closure
>      let get_file_mode = || fstatat(...);
>
>> +
>> +            let stat = self
> ^ stat is not a good name for this.
> This can stay an `if` as before since the condition doesn't need to be
> this huge.
>
>>                   .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),
>> +                    },
>> +                )
> ^ this block would just be
>                  .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,
>                      })
>                  })
>
>
>> +                .map_err(|err| {
>> +                    anyhow::Error::new(err).context(format!(
>> +                        "stat failed on {:?} with {:?}",
>> +                        full_path,
>> +                        err.desc()
> When switching to 'context', there's no need to include the error in the
> message, it just causes duplicate text in the output.
>
>> +                    ))
>> +                })?;
> ^ We have both `Error` and `Context` in imported. The entire `.map_err`
> can be turned into just:
>
>          .with_context(|| format!("stat failed on {full_path:?}"))?
>
>> +
>> +            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()
>> +                        ));
>> +                    }
>> +                }
>> +            }
> ^ Instead of this large block we then only need:
>
>      let stat = stat_results.map(Ok).unwrap_or_else(get_file_mode)?;
>
>> +
>>               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(),
> ^ and this change can just be dropped :-)
>
>>               });
>>           }
>>   
>> @@ -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))
> ^ Please don't just ignore errors, just put a `?` onto the call.
>
> On the one hand we know that this cannot error since we pass `Some`, so
> we could even unwrap, but that's worse, since there's no guarantee that
> no other errors can happen.
> We *may* be able to guarantee this by adding a `GetFileMode` impl
> in pathpatterns directly to `u32` with `type Error =
> std::convert::Infallible`, and then drop the `Some()` wrapping around
> this piece?

Yes, that's nice. Adding this implementation to pathpatterns:

     impl GetFileMode for u32 {
         type Error = std::convert::Infallible;
         fn get(self) -> Result<u32, Self::Error> {
             Ok(self)
         }
     }

then it should look like this:

     if self
         .patterns
         .matches(match_path.as_os_str().as_bytes(), stat.st_mode)?
             == Some(MatchType::Exclude)
     {
         return Ok(());
     }

>>           {
>>               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,
> Same here, 3 lines above we pass `Some(file_type)` to `matches()`.
> We should not just use `_` here, but rather, the above should fail.

Here we can write this:

     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,
         _ => self.state.current_match,

     };

Unwrapping should (?) be safe because the `Err` is `Infallible`.

>>           };
>>   
>>           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