Re: [pbs-devel] [PATCH v2 proxmox-backup 1/2] fix: #4761: unlink existing entries for hard/symlinks when overwrite

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

On Tue, Aug 01, 2023 at 12:34:11PM +0200, Christian Ebner wrote:
> Creating symlinks or hardlinks might fail if a directory entry with the
> same name is already present on the filesystem during restore.
> 
> When the overwrite flag is given, on failure unlink the existing entry
> (except directories) and retry hard/symlink creation.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> changes since v1:
> * rebased to current master
> 
>  pbs-client/src/pxar/extract.rs | 39 +++++++++++++++++++++++++++++++---
>  1 file changed, 36 insertions(+), 3 deletions(-)
> 
> diff --git a/pbs-client/src/pxar/extract.rs b/pbs-client/src/pxar/extract.rs
> index 4dbaf52d..c1e7b417 100644
> --- a/pbs-client/src/pxar/extract.rs
> +++ b/pbs-client/src/pxar/extract.rs
> @@ -547,7 +547,21 @@ impl Extractor {
>          link: &OsStr,
>      ) -> Result<(), Error> {
>          let parent = self.parent_fd()?;
> -        nix::unistd::symlinkat(link, Some(parent), file_name)?;
> +
> +        match nix::unistd::symlinkat(link, Some(parent), file_name) {
> +            Ok(()) => {}
> +            Err(nix::errno::Errno::EEXIST) => {
> +                if !self.overwrite {
> +                    return Err(nix::errno::Errno::EEXIST.into());
> +                }

The above could be shortened to a pattern gaurd:

               Err(nix::errno::Errno::EEXIST) if self.overwrite => {

then we don't need the extra `return Err()` since we just run into the
final `Err(err)` case.

> +                // Never unlink directories
> +                let flag = nix::unistd::UnlinkatFlags::NoRemoveDir;
> +                nix::unistd::unlinkat(Some(parent), file_name, flag)?;
> +                nix::unistd::symlinkat(link, Some(parent), file_name)?;
> +            }
> +            Err(err) => return Err(err.into())
> +        }
> +
>          metadata::apply_at(
>              self.feature_flags,
>              metadata,
> @@ -564,13 +578,32 @@ impl Extractor {
>          let parent = self.parent_fd()?;
>          let root = self.dir_stack.root_dir_fd()?;
>          let target = CString::new(link.as_bytes())?;
> -        nix::unistd::linkat(
> +
> +        match nix::unistd::linkat(
>              Some(root.as_raw_fd()),
>              target.as_c_str(),
>              Some(parent),
>              file_name,
>              nix::unistd::LinkatFlags::NoSymlinkFollow,
> -        )?;

We can avoid some duplication by moving the `linkat` call into a
closure:
        let dolink = || {
            nix::unistd::linkat(
                Some(root.as_raw_fd()),
                target.as_c_str(),
                Some(parent),
                file_name,
                nix::unistd::LinkatFlags::NoSymlinkFollow,
            )
        };
        match dolink() {
            ...
            nix::unistd::unlinkat(Some(parent), file_name, flag)?;
            dolink()?;
        }
        ...

> +        ) {
> +            Ok(()) => {}
> +            Err(nix::errno::Errno::EEXIST) => {
> +                if !self.overwrite {
> +                    return Err(nix::errno::Errno::EEXIST.into());
> +                }
> +                // Never unlink directories
> +                let flag = nix::unistd::UnlinkatFlags::NoRemoveDir;
> +                nix::unistd::unlinkat(Some(parent), file_name, flag)?;
> +                nix::unistd::linkat(
> +                    Some(root.as_raw_fd()),
> +                    target.as_c_str(),
> +                    Some(parent),
> +                    file_name,
> +                    nix::unistd::LinkatFlags::NoSymlinkFollow,
> +                )?;
> +            }
> +            Err(err) => return Err(err.into())
> +        }
>  
>          Ok(())
>      }
> -- 
> 2.39.2