[pbs-devel] [PATH proxmox-backup] fix #4380: stat() is run when file is executed

[pbs-devel] [PATH proxmox-backup] fix #4380: stat() is run when file is executed

[Gabriel Goller]

When executing `proxmox-backup-client backup ...
--exclude "test/test.txt"` it still executed stat() on "test.txt",
which won't work when the current user doesn't have access to the
file or the parent folder. Now we check if the file is excluded,
and if it is not, then we execute stat().

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 pbs-client/src/pxar/create.rs | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
index 2577cf98..c573c2a3 100644
--- a/pbs-client/src/pxar/create.rs
+++ b/pbs-client/src/pxar/create.rs
@@ -434,6 +434,15 @@ 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());
+            if self
+                .patterns
+                .matches(match_path.as_os_str().as_bytes(), None)
+                == Some(MatchType::Exclude)
+            {
+                continue;
+            }
+
             let stat = match nix::sys::stat::fstatat(
                 dir_fd,
                 file_name.as_c_str(),
@@ -444,15 +453,6 @@ 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)
-            {
-                continue;
-            }
-
             self.entry_counter += 1;
             if self.entry_counter > self.entry_limit {
                 bail!(
-- 
2.39.2

Re: [pbs-devel] [PATH proxmox-backup] fix #4380: stat() is run when file is executed

[Fiona Ebner]

Am 03.08.23 um 17:22 schrieb Gabriel Goller:
> diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
> index 2577cf98..c573c2a3 100644
> --- a/pbs-client/src/pxar/create.rs
> +++ b/pbs-client/src/pxar/create.rs
> @@ -434,6 +434,15 @@ 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());
> +            if self
> +                .patterns
> +                .matches(match_path.as_os_str().as_bytes(), None)

Is it fine to call matches() without the file mode in all cases? Can't
it make a difference for directory matching? If it's okay, please
explain why in the commit message.

[pbs-devel] applied: [PATH proxmox-backup] fix #4380: stat() is run when file is executed

[Fabian Grünbichler]

although a somewhat rare edge case, this does improve things a bit!

there is still a stray

 failed to open file: ".pxarexclude": access denied

if the client encounters a dir with +r, but -x, even if all of the dirs
entries are excluded..

I guess we could actually skip iterating with a more meaningful error
message if we lack -x? or just check that all dir entries (which we get
thanks to +r) of that dir are excluded by the patterns we already have
at that point, instead of attempting to read the contained patterns and
then recursively iterate over the dir's entries?

On August 3, 2023 5:22 pm, Gabriel Goller wrote:
> When executing `proxmox-backup-client backup ...
> --exclude "test/test.txt"` it still executed stat() on "test.txt",
> which won't work when the current user doesn't have access to the
> file or the parent folder. Now we check if the file is excluded,
> and if it is not, then we execute stat().
> 
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
>  pbs-client/src/pxar/create.rs | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
> index 2577cf98..c573c2a3 100644
> --- a/pbs-client/src/pxar/create.rs
> +++ b/pbs-client/src/pxar/create.rs
> @@ -434,6 +434,15 @@ 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());
> +            if self
> +                .patterns
> +                .matches(match_path.as_os_str().as_bytes(), None)
> +                == Some(MatchType::Exclude)
> +            {
> +                continue;
> +            }
> +
>              let stat = match nix::sys::stat::fstatat(
>                  dir_fd,
>                  file_name.as_c_str(),
> @@ -444,15 +453,6 @@ 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)
> -            {
> -                continue;
> -            }
> -
>              self.entry_counter += 1;
>              if self.entry_counter > self.entry_limit {
>                  bail!(
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 

Re: [pbs-devel] [PATH proxmox-backup] fix #4380: stat() is run when file is executed

[Fiona Ebner]

Am 04.08.23 um 09:54 schrieb Gabriel Goller:
> On 8/4/23 09:42, Fiona Ebner wrote:
> 
>> Am 03.08.23 um 17:22 schrieb Gabriel Goller:
>>> diff --git a/pbs-client/src/pxar/create.rs
>>> b/pbs-client/src/pxar/create.rs
>>> index 2577cf98..c573c2a3 100644
>>> --- a/pbs-client/src/pxar/create.rs
>>> +++ b/pbs-client/src/pxar/create.rs
>>> @@ -434,6 +434,15 @@ 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());
>>> +            if self
>>> +                .patterns
>>> +                .matches(match_path.as_os_str().as_bytes(), None)
>> Is it fine to call matches() without the file mode in all cases? Can't
>> it make a difference for directory matching? If it's okay, please
>> explain why in the commit message.
> 
> I think so, because we get the `patterns` (at least those with
> MatchType::Exclude) only from the `.pxarexclude` file or from the
> command line argument `--exclude`. Both do not allow to specify the file
> mode.
> 

But the MatchFlag::MATCH_DIRECTORIES flag is set for the pattern with a
trailing slash in match_list.rs's parse_pattern_do() in the pathpatterns
create during construction. My concern is that now a file 'foo' will
match the pattern 'foo/' when it previously didn't (but I didn't test it).

Re: [pbs-devel] [PATH proxmox-backup] fix #4380: stat() is run when file is executed

[Fabian Grünbichler]

On August 4, 2023 9:42 am, Fiona Ebner wrote:
> Am 03.08.23 um 17:22 schrieb Gabriel Goller:
>> diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
>> index 2577cf98..c573c2a3 100644
>> --- a/pbs-client/src/pxar/create.rs
>> +++ b/pbs-client/src/pxar/create.rs
>> @@ -434,6 +434,15 @@ 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());
>> +            if self
>> +                .patterns
>> +                .matches(match_path.as_os_str().as_bytes(), None)
> 
> Is it fine to call matches() without the file mode in all cases? Can't
> it make a difference for directory matching? If it's okay, please
> explain why in the commit message.

good catch, thanks.

I guess we need something like this if we want to support it - the
second hunk is only needed in case we ever differentiate between the
different types other than directories ('/' at the end of the pattern)
and regular files.

in the end, it might make more sense to try the other approach I
indicated as follow-up in my first reply? we already have the stat info
of each dir we encounter, so we can decide if a dir is a "weird
unreadable one" and treat that specially, moving the pattern match here
back below the stat, and just never go down that code path for affected
dirs?

diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
index c573c2a3..eaa84c76 100644
--- a/pbs-client/src/pxar/create.rs
+++ b/pbs-client/src/pxar/create.rs
@@ -435,9 +435,15 @@ impl Archiver {
             let full_path = self.path.join(os_file_name);
 
             let match_path = PathBuf::from("/").join(full_path.clone());
+            let entry_type = if file.file_type() == Some(nix::dir::Type::Directory) {
+                Some(libc::S_IFDIR)
+            } else {
+                Some(libc::S_IFREG)
+            };
+
             if self
                 .patterns
-                .matches(match_path.as_os_str().as_bytes(), None)
+                .matches(match_path.as_os_str().as_bytes(), entry_type)
                 == Some(MatchType::Exclude)
             {
                 continue;
@@ -453,6 +459,14 @@ impl Archiver {
                 Err(err) => return Err(err).context(format!("stat failed on {:?}", full_path)),
             };
 
+            if self
+                .patterns
+                .matches(match_path.as_os_str().as_bytes(), Some(stat.st_mode))
+                == Some(MatchType::Exclude)
+            {
+                continue;
+            }
+
             self.entry_counter += 1;
             if self.entry_counter > self.entry_limit {
                 bail!(

Re: [pbs-devel] [PATH proxmox-backup] fix #4380: stat() is run when file is executed

[Fabian Grünbichler]

On August 4, 2023 10:20 am, Fiona Ebner wrote:
> Am 04.08.23 um 09:54 schrieb Gabriel Goller:
>> On 8/4/23 09:42, Fiona Ebner wrote:
>> 
>>> Am 03.08.23 um 17:22 schrieb Gabriel Goller:
>>>> diff --git a/pbs-client/src/pxar/create.rs
>>>> b/pbs-client/src/pxar/create.rs
>>>> index 2577cf98..c573c2a3 100644
>>>> --- a/pbs-client/src/pxar/create.rs
>>>> +++ b/pbs-client/src/pxar/create.rs
>>>> @@ -434,6 +434,15 @@ 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());
>>>> +            if self
>>>> +                .patterns
>>>> +                .matches(match_path.as_os_str().as_bytes(), None)
>>> Is it fine to call matches() without the file mode in all cases? Can't
>>> it make a difference for directory matching? If it's okay, please
>>> explain why in the commit message.
>> 
>> I think so, because we get the `patterns` (at least those with
>> MatchType::Exclude) only from the `.pxarexclude` file or from the
>> command line argument `--exclude`. Both do not allow to specify the file
>> mode.
>> 
> 
> But the MatchFlag::MATCH_DIRECTORIES flag is set for the pattern with a
> trailing slash in match_list.rs's parse_pattern_do() in the pathpatterns
> create during construction. My concern is that now a file 'foo' will
> match the pattern 'foo/' when it previously didn't (but I didn't test it).

I did test (see my other reply), and reverted this for now again.
Gabriel will try the "special case readable, but inaccessible
directories" approach.

if that doesn't work out, we can either mark this as WONTFIX, switch to
coarse matching via dir entry type, or coarse matching before, and full
matching after stat..

Re: [pbs-devel] [PATH proxmox-backup] fix #4380: stat() is run when file is executed

[Fiona Ebner]

Am 04.08.23 um 10:21 schrieb Fabian Grünbichler:
> On August 4, 2023 9:42 am, Fiona Ebner wrote:
>> Am 03.08.23 um 17:22 schrieb Gabriel Goller:
>>> diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
>>> index 2577cf98..c573c2a3 100644
>>> --- a/pbs-client/src/pxar/create.rs
>>> +++ b/pbs-client/src/pxar/create.rs
>>> @@ -434,6 +434,15 @@ 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());
>>> +            if self
>>> +                .patterns
>>> +                .matches(match_path.as_os_str().as_bytes(), None)
>>
>> Is it fine to call matches() without the file mode in all cases? Can't
>> it make a difference for directory matching? If it's okay, please
>> explain why in the commit message.
> 
> good catch, thanks.
> 
> I guess we need something like this if we want to support it - the
> second hunk is only needed in case we ever differentiate between the
> different types other than directories ('/' at the end of the pattern)
> and regular files.

More is required if we ever need that, because (continued below)

> 
> in the end, it might make more sense to try the other approach I
> indicated as follow-up in my first reply? we already have the stat info
> of each dir we encounter, so we can decide if a dir is a "weird
> unreadable one" and treat that specially, moving the pattern match here
> back below the stat, and just never go down that code path for affected
> dirs?

Can't there be cases where stat() for some non-directory could also fail?

If we ever require to differentiate between different non-directory
types, it becomes a real chicken-and-egg problem I think. Seems like if
we can't stat(), we can choose between:
1. exclude anyways, even if we can't be sure whether it's special or regular
2. failing (thus WONTFIXing the bug for this edge case)

But such patterns are currently not used, so.. :P

> 
> diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
> index c573c2a3..eaa84c76 100644
> --- a/pbs-client/src/pxar/create.rs
> +++ b/pbs-client/src/pxar/create.rs
> @@ -435,9 +435,15 @@ impl Archiver {
>              let full_path = self.path.join(os_file_name);
>  
>              let match_path = PathBuf::from("/").join(full_path.clone());
> +            let entry_type = if file.file_type() == Some(nix::dir::Type::Directory) {
> +                Some(libc::S_IFDIR)
> +            } else {
> +                Some(libc::S_IFREG)

(continued) this here wouldn't work for patterns that want to skip only
regular files, but not other kinds of files. By claiming that it's a
regular file here we'd skip regardless of what it actually is.

> +            };
> +
>              if self
>                  .patterns
> -                .matches(match_path.as_os_str().as_bytes(), None)
> +                .matches(match_path.as_os_str().as_bytes(), entry_type)
>                  == Some(MatchType::Exclude)
>              {
>                  continue;

Re: [pbs-devel] [PATH proxmox-backup] fix #4380: stat() is run when file is executed

[Wolfgang Bumiller]

On Fri, Aug 04, 2023 at 10:52:13AM +0200, Fiona Ebner wrote:
> Am 04.08.23 um 10:21 schrieb Fabian Grünbichler:
> > On August 4, 2023 9:42 am, Fiona Ebner wrote:
> >> Am 03.08.23 um 17:22 schrieb Gabriel Goller:
> >>> diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
> >>> index 2577cf98..c573c2a3 100644
> >>> --- a/pbs-client/src/pxar/create.rs
> >>> +++ b/pbs-client/src/pxar/create.rs
> >>> @@ -434,6 +434,15 @@ 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());
> >>> +            if self
> >>> +                .patterns
> >>> +                .matches(match_path.as_os_str().as_bytes(), None)
> >>
> >> Is it fine to call matches() without the file mode in all cases? Can't
> >> it make a difference for directory matching? If it's okay, please
> >> explain why in the commit message.
> > 
> > good catch, thanks.
> > 
> > I guess we need something like this if we want to support it - the
> > second hunk is only needed in case we ever differentiate between the
> > different types other than directories ('/' at the end of the pattern)
> > and regular files.
> 
> More is required if we ever need that, because (continued below)
> 
> > 
> > in the end, it might make more sense to try the other approach I
> > indicated as follow-up in my first reply? we already have the stat info
> > of each dir we encounter, so we can decide if a dir is a "weird
> > unreadable one" and treat that specially, moving the pattern match here
> > back below the stat, and just never go down that code path for affected
> > dirs?
> 
> Can't there be cases where stat() for some non-directory could also fail?
> 
> If we ever require to differentiate between different non-directory
> types, it becomes a real chicken-and-egg problem I think. Seems like if
> we can't stat(), we can choose between:
> 1. exclude anyways, even if we can't be sure whether it's special or regular
> 2. failing (thus WONTFIXing the bug for this edge case)
> 
> But such patterns are currently not used, so.. :P
> 
> > 
> > diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
> > index c573c2a3..eaa84c76 100644
> > --- a/pbs-client/src/pxar/create.rs
> > +++ b/pbs-client/src/pxar/create.rs
> > @@ -435,9 +435,15 @@ impl Archiver {
> >              let full_path = self.path.join(os_file_name);
> >  
> >              let match_path = PathBuf::from("/").join(full_path.clone());
> > +            let entry_type = if file.file_type() == Some(nix::dir::Type::Directory) {
> > +                Some(libc::S_IFDIR)
> > +            } else {
> > +                Some(libc::S_IFREG)
> 
> (continued) this here wouldn't work for patterns that want to skip only
> regular files, but not other kinds of files. By claiming that it's a
> regular file here we'd skip regardless of what it actually is.

It should be possible to do a complete translation here.

I think what we should do for a best-effort solution is add a matching
variant to `pathpatterns`' `MatchList` for "untyped" files such that we
*error* when hitting a pattern which requires the type to be known.

This way, the user can exclude the file, they just need to make sure
those particular files are matched first (as in, their `--exclude`
parameter must not be followed by any *typed* `--exclude/--include`
parameters)

So basically:
- If readdir() gives us a type -> use it.
- Otherwise: stat() -> use that info.
- If stat fails -> use the "untyped match" described above.
- If the untyped match fails, bail.