pxar: better error handling on extract

Errors while applying metadata will not be considered fatal
by default using `pxar extract` unless `--strict` was passed
in which case it'll bail out immediately.

It'll still return an error exit status if something had
failed along the way.

Note that most other errors will still cause it to bail out
(eg. errors creating files, or I/O errors while writing
the contents).

Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>

src/backup/catalog_shell.rs

@@ -3,7 +3,7 @@ use std::ffi::{CStr, CString, OsStr, OsString};
 use std::future::Future;
 use std::io::Write;
 use std::mem;
-use std::os::unix::ffi::OsStrExt;
+use std::os::unix::ffi::{OsStrExt, OsStringExt};
 use std::path::{Path, PathBuf};
 use std::pin::Pin;
 
@@ -1073,6 +1073,7 @@ impl<'a> ExtractorState<'a> {
             }
             self.path.extend(&entry.name);
 
+            self.extractor.set_path(OsString::from_vec(self.path.clone()));
             self.handle_entry(entry).await?;
         }
 

src/bin/proxmox-backup-client.rs

@@ -1383,6 +1383,7 @@ async fn restore(param: Value) -> Result<Value, Error> {
                         println!("{:?}", path);
                     }
                 },
+                None,
             )
             .map_err(|err| format_err!("error extracting archive - {}", err))?;
         } else {

src/bin/pxar.rs

@@ -3,8 +3,10 @@ use std::ffi::OsStr;
 use std::fs::OpenOptions;
 use std::os::unix::fs::OpenOptionsExt;
 use std::path::{Path, PathBuf};
+use std::sync::Arc;
+use std::sync::atomic::{AtomicBool, Ordering};
 
-use anyhow::{format_err, Error};
+use anyhow::{bail, format_err, Error};
 use futures::future::FutureExt;
 use futures::select;
 use tokio::signal::unix::{signal, SignalKind};
@@ -25,6 +27,7 @@ fn extract_archive_from_reader<R: std::io::Read>(
     verbose: bool,
     match_list: &[MatchEntry],
     extract_match_default: bool,
+    on_error: Option<Box<dyn FnMut(Error) -> Result<(), Error> + Send>>,
 ) -> Result<(), Error> {
     proxmox_backup::pxar::extract_archive(
         pxar::decoder::Decoder::from_std(reader)?,
@@ -38,6 +41,7 @@ fn extract_archive_from_reader<R: std::io::Read>(
                 println!("{:?}", path);
             }
         },
+        on_error,
     )
 }
 
@@ -104,6 +108,11 @@ fn extract_archive_from_reader<R: std::io::Read>(
                 optional: true,
                 default: false,
             },
+            strict: {
+                description: "Stop on errors. Otherwise most errors will simply warn.",
+                optional: true,
+                default: false,
+            },
         },
     },
 )]
@@ -121,6 +130,7 @@ fn extract_archive(
     no_device_nodes: bool,
     no_fifos: bool,
     no_sockets: bool,
+    strict: bool,
 ) -> Result<(), Error> {
     let mut feature_flags = Flags::DEFAULT;
     if no_xattrs {
@@ -166,6 +176,20 @@ fn extract_archive(
 
     let extract_match_default = match_list.is_empty();
 
+    let was_ok = Arc::new(AtomicBool::new(true));
+    let on_error = if strict {
+        // by default errors are propagated up
+        None
+    } else {
+        let was_ok = Arc::clone(&was_ok);
+        // otherwise we want to log them but not act on them
+        Some(Box::new(move |err| {
+            was_ok.store(false, Ordering::Release);
+            eprintln!("error: {}", err);
+            Ok(())
+        }) as Box<dyn FnMut(Error) -> Result<(), Error> + Send>)
+    };
+
     if archive == "-" {
         let stdin = std::io::stdin();
         let mut reader = stdin.lock();
@@ -177,6 +201,7 @@ fn extract_archive(
             verbose,
             &match_list,
             extract_match_default,
+            on_error,
         )?;
     } else {
         if verbose {
@@ -192,9 +217,14 @@ fn extract_archive(
             verbose,
             &match_list,
             extract_match_default,
+            on_error,
         )?;
     }
 
+    if !was_ok.load(Ordering::Acquire) {
+        bail!("there were errors");
+    }
+
     Ok(())
 }
 

src/pxar/extract.rs

@@ -6,6 +6,7 @@ use std::io;
 use std::os::unix::ffi::OsStrExt;
 use std::os::unix::io::{AsRawFd, FromRawFd, RawFd};
 use std::path::Path;
+use std::sync::{Arc, Mutex};
 
 use anyhow::{bail, format_err, Error};
 use nix::dir::Dir;
@@ -20,8 +21,8 @@ use proxmox::c_result;
 use proxmox::tools::fs::{create_path, CreateOptions};
 
 use crate::pxar::dir_stack::PxarDirStack;
-use crate::pxar::Flags;
 use crate::pxar::metadata;
+use crate::pxar::Flags;
 
 pub fn extract_archive<T, F>(
     mut decoder: pxar::decoder::Decoder<T>,
@@ -31,6 +32,7 @@ pub fn extract_archive<T, F>(
     feature_flags: Flags,
     allow_existing_dirs: bool,
     mut callback: F,
+    on_error: Option<Box<dyn FnMut(Error) -> Result<(), Error> + Send>>,
 ) -> Result<(), Error>
 where
     T: pxar::decoder::SeqRead,
@@ -69,7 +71,12 @@ where
         feature_flags,
     );
 
+    if let Some(on_error) = on_error {
+        extractor.on_error(on_error);
+    }
+
     let mut match_stack = Vec::new();
+    let mut err_path_stack = Vec::new();
     let mut current_match = extract_match_default;
     while let Some(entry) = decoder.next() {
         use pxar::EntryKind;
@@ -88,6 +95,8 @@ where
 
         let metadata = entry.metadata();
 
+        extractor.set_path(entry.path().as_os_str().to_owned());
+
         let match_result = match_list.matches(
             entry.path().as_os_str().as_bytes(),
             Some(metadata.file_type() as u32),
@@ -103,17 +112,32 @@ where
                 callback(entry.path());
 
                 let create = current_match && match_result != Some(MatchType::Exclude);
-                extractor.enter_directory(file_name_os.to_owned(), metadata.clone(), create)?;
+                extractor
+                    .enter_directory(file_name_os.to_owned(), metadata.clone(), create)
+                    .map_err(|err| format_err!("error at entry {:?}: {}", file_name_os, err))?;
 
                 // We're starting a new directory, push our old matching state and replace it with
                 // our new one:
                 match_stack.push(current_match);
                 current_match = did_match;
 
+                // When we hit the goodbye table we'll try to apply metadata to the directory, but
+                // the Goodbye entry will not contain the path, so push it to our path stack for
+                // error messages:
+                err_path_stack.push(extractor.clone_path());
+
                 Ok(())
             }
             (_, EntryKind::GoodbyeTable) => {
                 // go up a directory
+
+                extractor.set_path(err_path_stack.pop().ok_or_else(|| {
+                    format_err!(
+                        "error at entry {:?}: unexpected end of directory",
+                        file_name_os
+                    )
+                })?);
+
                 extractor
                     .leave_directory()
                     .map_err(|err| format_err!("error at entry {:?}: {}", file_name_os, err))?;
@@ -182,6 +206,13 @@ pub(crate) struct Extractor {
     feature_flags: Flags,
     allow_existing_dirs: bool,
     dir_stack: PxarDirStack,
+
+    /// For better error output we need to track the current path in the Extractor state.
+    current_path: Arc<Mutex<OsString>>,
+
+    /// Error callback. Includes `current_path` in the reformatted error, should return `Ok` to
+    /// continue extracting or the passed error as `Err` to bail out.
+    on_error: Box<dyn FnMut(Error) -> Result<(), Error> + Send>,
 }
 
 impl Extractor {
@@ -196,9 +227,30 @@ impl Extractor {
             dir_stack: PxarDirStack::new(root_dir, metadata),
             allow_existing_dirs,
             feature_flags,
+            current_path: Arc::new(Mutex::new(OsString::new())),
+            on_error: Box::new(|err| Err(err)),
         }
     }
 
+    /// We call this on errors. The error will be reformatted to include `current_path`. The
+    /// callback should decide whether this error was fatal (simply return it) to bail out early,
+    /// or log/remember/accumulate errors somewhere and return `Ok(())` in its place to continue
+    /// extracting.
+    pub fn on_error(&mut self, mut on_error: Box<dyn FnMut(Error) -> Result<(), Error> + Send>) {
+        let path = Arc::clone(&self.current_path);
+        self.on_error = Box::new(move |err: Error| -> Result<(), Error> {
+            on_error(format_err!("error at {:?}: {}", path.lock().unwrap(), err))
+        });
+    }
+
+    pub fn set_path(&mut self, path: OsString) {
+        *self.current_path.lock().unwrap() = path;
+    }
+
+    pub fn clone_path(&self) -> OsString {
+        self.current_path.lock().unwrap().clone()
+    }
+
     /// When encountering a directory during extraction, this is used to keep track of it. If
     /// `create` is true it is immediately created and its metadata will be updated once we leave
     /// it. If `create` is false it will only be created if it is going to have any actual content.
@@ -217,7 +269,7 @@ impl Extractor {
         Ok(())
     }
 
-    /// When done with a directory we need to make sure we're
+    /// When done with a directory we can apply its metadata if it has been created.
     pub fn leave_directory(&mut self) -> Result<(), Error> {
         let dir = self
             .dir_stack
@@ -231,6 +283,7 @@ impl Extractor {
                 dir.metadata(),
                 fd,
                 &CString::new(dir.file_name().as_bytes())?,
+                &mut self.on_error,
             )
             .map_err(|err| format_err!("failed to apply directory metadata: {}", err))?;
         }
@@ -256,14 +309,16 @@ impl Extractor {
     ) -> Result<(), Error> {
         let parent = self.parent_fd()?;
         nix::unistd::symlinkat(link, Some(parent), file_name)?;
-        metadata::apply_at(self.feature_flags, metadata, parent, file_name)
+        metadata::apply_at(
+            self.feature_flags,
+            metadata,
+            parent,
+            file_name,
+            &mut self.on_error,
+        )
     }
 
-    pub fn extract_hardlink(
+    pub fn extract_hardlink(&mut self, file_name: &CStr, link: &OsStr) -> Result<(), Error> {
-        &mut self,
-        file_name: &CStr,
-        link: &OsStr,
-    ) -> Result<(), Error> {
         crate::pxar::tools::assert_relative_path(link)?;
 
         let parent = self.parent_fd()?;
@@ -307,7 +362,13 @@ impl Extractor {
         unsafe { c_result!(libc::mknodat(parent, file_name.as_ptr(), mode, device)) }
             .map_err(|err| format_err!("failed to create device node: {}", err))?;
 
-        metadata::apply_at(self.feature_flags, metadata, parent, file_name)
+        metadata::apply_at(
+            self.feature_flags,
+            metadata,
+            parent,
+            file_name,
+            &mut self.on_error,
+        )
     }
 
     pub fn extract_file(
@@ -319,16 +380,23 @@ impl Extractor {
     ) -> Result<(), Error> {
         let parent = self.parent_fd()?;
         let mut file = unsafe {
-            std::fs::File::from_raw_fd(nix::fcntl::openat(
+            std::fs::File::from_raw_fd(
+                nix::fcntl::openat(
                     parent,
                     file_name,
                     OFlag::O_CREAT | OFlag::O_EXCL | OFlag::O_WRONLY | OFlag::O_CLOEXEC,
                     Mode::from_bits(0o600).unwrap(),
                 )
-            .map_err(|err| format_err!("failed to create file {:?}: {}", file_name, err))?)
+                .map_err(|err| format_err!("failed to create file {:?}: {}", file_name, err))?,
+            )
         };
 
-        metadata::apply_initial_flags(self.feature_flags, metadata, file.as_raw_fd())?;
+        metadata::apply_initial_flags(
+            self.feature_flags,
+            metadata,
+            file.as_raw_fd(),
+            &mut self.on_error,
+        )?;
 
         let extracted = io::copy(&mut *contents, &mut file)
             .map_err(|err| format_err!("failed to copy file contents: {}", err))?;
@@ -336,7 +404,13 @@ impl Extractor {
             bail!("extracted {} bytes of a file of {} bytes", extracted, size);
         }
 
-        metadata::apply(self.feature_flags, metadata, file.as_raw_fd(), file_name)
+        metadata::apply(
+            self.feature_flags,
+            metadata,
+            file.as_raw_fd(),
+            file_name,
+            &mut self.on_error,
+        )
     }
 
     pub async fn async_extract_file<T: tokio::io::AsyncRead + Unpin>(
@@ -348,16 +422,23 @@ impl Extractor {
     ) -> Result<(), Error> {
         let parent = self.parent_fd()?;
         let mut file = tokio::fs::File::from_std(unsafe {
-            std::fs::File::from_raw_fd(nix::fcntl::openat(
+            std::fs::File::from_raw_fd(
+                nix::fcntl::openat(
                     parent,
                     file_name,
                     OFlag::O_CREAT | OFlag::O_EXCL | OFlag::O_WRONLY | OFlag::O_CLOEXEC,
                     Mode::from_bits(0o600).unwrap(),
                 )
-            .map_err(|err| format_err!("failed to create file {:?}: {}", file_name, err))?)
+                .map_err(|err| format_err!("failed to create file {:?}: {}", file_name, err))?,
+            )
         });
 
-        metadata::apply_initial_flags(self.feature_flags, metadata, file.as_raw_fd())?;
+        metadata::apply_initial_flags(
+            self.feature_flags,
+            metadata,
+            file.as_raw_fd(),
+            &mut self.on_error,
+        )?;
 
         let extracted = tokio::io::copy(&mut *contents, &mut file)
             .await
@@ -366,6 +447,12 @@ impl Extractor {
             bail!("extracted {} bytes of a file of {} bytes", extracted, size);
         }
 
-        metadata::apply(self.feature_flags, metadata, file.as_raw_fd(), file_name)
+        metadata::apply(
+            self.feature_flags,
+            metadata,
+            file.as_raw_fd(),
+            file_name,
+            &mut self.on_error,
+        )
     }
 }

src/pxar/metadata.rs

@@ -62,6 +62,7 @@ pub fn apply_at(
     metadata: &Metadata,
     parent: RawFd,
     file_name: &CStr,
+    on_error: &mut (dyn FnMut(Error) -> Result<(), Error> + Send),
 ) -> Result<(), Error> {
     let fd = proxmox::tools::fd::Fd::openat(
         &unsafe { RawFdNum::from_raw_fd(parent) },
@@ -70,20 +71,32 @@ pub fn apply_at(
         Mode::empty(),
     )?;
 
-    apply(flags, metadata, fd.as_raw_fd(), file_name)
+    apply(flags, metadata, fd.as_raw_fd(), file_name, on_error)
 }
 
 pub fn apply_initial_flags(
     flags: Flags,
     metadata: &Metadata,
     fd: RawFd,
+    on_error: &mut (dyn FnMut(Error) -> Result<(), Error> + Send),
 ) -> Result<(), Error> {
     let entry_flags = Flags::from_bits_truncate(metadata.stat.flags);
-    apply_chattr(fd, entry_flags.to_initial_chattr(), flags.to_initial_chattr())?;
+    apply_chattr(
+        fd,
+        entry_flags.to_initial_chattr(),
+        flags.to_initial_chattr(),
+    )
+    .or_else(on_error)?;
     Ok(())
 }
 
-pub fn apply(flags: Flags, metadata: &Metadata, fd: RawFd, file_name: &CStr) -> Result<(), Error> {
+pub fn apply(
+    flags: Flags,
+    metadata: &Metadata,
+    fd: RawFd,
+    file_name: &CStr,
+    on_error: &mut (dyn FnMut(Error) -> Result<(), Error> + Send),
+) -> Result<(), Error> {
     let c_proc_path = CString::new(format!("/proc/self/fd/{}", fd)).unwrap();
 
     unsafe {
@@ -95,15 +108,18 @@ pub fn apply(flags: Flags, metadata: &Metadata, fd: RawFd, file_name: &CStr) ->
         ))
         .map(drop)
         .or_else(allow_notsupp)
-        .map_err(|err| format_err!("failed to set ownership: {}", err))?;
+        .map_err(|err| format_err!("failed to set ownership: {}", err))
+        .or_else(&mut *on_error)?;
     }
 
     let mut skip_xattrs = false;
-    apply_xattrs(flags, c_proc_path.as_ptr(), metadata, &mut skip_xattrs)?;
+    apply_xattrs(flags, c_proc_path.as_ptr(), metadata, &mut skip_xattrs)
-    add_fcaps(flags, c_proc_path.as_ptr(), metadata, &mut skip_xattrs)?;
+        .or_else(&mut *on_error)?;
+    add_fcaps(flags, c_proc_path.as_ptr(), metadata, &mut skip_xattrs).or_else(&mut *on_error)?;
     apply_acls(flags, &c_proc_path, metadata)
-        .map_err(|err| format_err!("failed to apply acls: {}", err))?;
+        .map_err(|err| format_err!("failed to apply acls: {}", err))
-    apply_quota_project_id(flags, fd, metadata)?;
+        .or_else(&mut *on_error)?;
+    apply_quota_project_id(flags, fd, metadata).or_else(&mut *on_error)?;
 
     // Finally mode and time. We may lose access with mode, but the changing the mode also
     // affects times.
@@ -113,11 +129,12 @@ pub fn apply(flags: Flags, metadata: &Metadata, fd: RawFd, file_name: &CStr) ->
         })
         .map(drop)
         .or_else(allow_notsupp)
-        .map_err(|err| format_err!("failed to change file mode: {}", err))?;
+        .map_err(|err| format_err!("failed to change file mode: {}", err))
+        .or_else(&mut *on_error)?;
     }
 
     if metadata.stat.flags != 0 {
-        apply_flags(flags, fd, metadata.stat.flags)?;
+        apply_flags(flags, fd, metadata.stat.flags).or_else(&mut *on_error)?;
     }
 
     let res = c_result!(unsafe {
@@ -131,13 +148,13 @@ pub fn apply(flags: Flags, metadata: &Metadata, fd: RawFd, file_name: &CStr) ->
     match res {
         Ok(_) => (),
         Err(ref err) if err.is_errno(Errno::EOPNOTSUPP) => (),
-        Err(ref err) if err.is_errno(Errno::EPERM) => {
+        Err(err) => {
-            println!(
+            on_error(format_err!(
                 "failed to restore mtime attribute on {:?}: {}",
-                file_name, err
+                file_name,
-            );
+                err
+            ))?;
         }
-        Err(err) => return Err(err.into()),
     }
 
     Ok(())
@@ -189,7 +206,7 @@ fn apply_xattrs(
         }
 
         if !xattr::is_valid_xattr_name(xattr.name()) {
-            println!("skipping invalid xattr named {:?}", xattr.name());
+            eprintln!("skipping invalid xattr named {:?}", xattr.name());
             continue;
         }