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

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

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.