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

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

On 8/11/23 10:51, Wolfgang Bumiller wrote:
> 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.
I used this approach so that we don't `stat` every excluded files as 
well (I think `matches_path` is
probably faster than `stat`). Also we will very rarely make the matching 
twice, because most of the
excluded paths come from the `--exclude` parameter or the `.pxarexclude` 
file, which both don't
allow to set the `file_mode`.
>> 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 <g.goller@proxmox.com>
>> ---
>>   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.

It is not strictly necessary, but the problem is that the function tries 
to open `.pxarexclude` in every directory.
Also directories were we have no access to, which will obviously fail. 
So if we have a directory with limited permissions,
(regardless if a `.pxarexclude` lives in it or not) we will get an error 
`failed to open .pxarexclude file` (even though it
doesn't even exist).

>> +            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.

I'll fix the implementation of `matches_path()` so that it actually 
fails (returns `Err`) when a pattern
with a `file_mode` is encountered (as it was intended). Otherwise this 
should work, as I didn't change
anything (except the preceding `matches_path()`).

>> +                && 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