[pbs-devel] [PATCH v2 many] fix #4995: Include symlinks in zip file restore

[pbs-devel] [PATCH v2 many] fix #4995: Include symlinks in zip file restore

[Filip Schauer]

Include symlinks when restoring files from a backup as a zip file.

Changes since v1:
* Use P instead of &Path
* Fix compile error due to misplaced comma
* Check content before symlink_target, since regular files are more
  common than symlinks

proxmox:

Filip Schauer (1):
  fix #4995: compression: Include symlinks in zip file restore

 proxmox-compression/src/zip.rs | 46 ++++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 11 deletions(-)

proxmox-backup:

Filip Schauer (1):
  fix #4995: pxar: Include symlinks in zip file restore

 pbs-client/src/pxar/extract.rs | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

-- 
2.39.2

[pbs-devel] [PATCH v2 proxmox 1/1] fix #4995: compression: Include symlinks in zip file restore

[Filip Schauer]

Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
 proxmox-compression/src/zip.rs | 46 ++++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 11 deletions(-)

diff --git a/proxmox-compression/src/zip.rs b/proxmox-compression/src/zip.rs
index d2d3fd8..e30f50a 100644
--- a/proxmox-compression/src/zip.rs
+++ b/proxmox-compression/src/zip.rs
@@ -204,6 +204,7 @@ pub struct ZipEntry {
     offset: u64,
     is_file: bool,
     is_utf8_filename: bool,
+    symlink_target: Option<OsString>,
 }
 
 impl ZipEntry {
@@ -211,7 +212,13 @@ impl ZipEntry {
     ///
     /// if is_file is false the path will contain an trailing separator,
     /// so that the zip file understands that it is a directory
-    pub fn new<P: AsRef<Path>>(path: P, mtime: i64, mode: u16, is_file: bool) -> Self {
+    pub fn new<P: AsRef<Path>>(
+        path: P,
+        mtime: i64,
+        mode: u16,
+        is_file: bool,
+        symlink_target: Option<P>,
+    ) -> Self {
         let mut relpath = PathBuf::new();
 
         for comp in path.as_ref().components() {
@@ -226,6 +233,7 @@ impl ZipEntry {
 
         let filename: OsString = relpath.into();
         let is_utf8_filename = filename.to_str().is_some();
+        let symlink_target_osstr =  symlink_target.map(|x| x.as_ref().into());
 
         Self {
             filename,
@@ -237,6 +245,7 @@ impl ZipEntry {
             offset: 0,
             is_file,
             is_utf8_filename,
+            symlink_target: symlink_target_osstr,
         }
     }
 
@@ -360,7 +369,9 @@ impl ZipEntry {
                 comment_len: 0,
                 start_disk: 0,
                 internal_flags: 0,
-                external_flags: (self.mode as u32) << 16 | (!self.is_file as u32) << 4,
+                external_flags: (self.mode as u32) << 16
+                | (self.symlink_target.is_some() as u32) << 5
+                | (!self.is_file as u32) << 4,
                 offset,
             },
         )
@@ -486,23 +497,30 @@ impl<W: AsyncWrite + Unpin> ZipEncoder<W> {
             .ok_or_else(|| format_err!("had no target during add entry"))?;
         entry.offset = self.byte_count.try_into()?;
         self.byte_count += entry.write_local_header(&mut target).await?;
-        if let Some(content) = content {
-            let mut reader = HashWrapper::new(content);
+
+        if content.is_some() || entry.symlink_target.is_some() {
             let mut enc = DeflateEncoder::with_quality(target, Level::Fastest);
 
-            enc.compress(&mut reader).await?;
+            if let Some(content) = content {
+                let mut reader = HashWrapper::new(content);
+                enc.compress(&mut reader).await?;
+                entry.crc32 = reader.finish().0;
+            } else if let Some(symlink_target) = entry.symlink_target.as_ref() {
+                let cursor = std::io::Cursor::new(symlink_target.as_bytes());
+                let mut reader = HashWrapper::new(cursor);
+                enc.compress(&mut reader).await?;
+                entry.crc32 = reader.finish().0;
+            }
+
             let total_in = enc.total_in();
             let total_out = enc.total_out();
             target = enc.into_inner();
 
-            let (crc32, _reader) = reader.finish();
-
             self.byte_count += total_out as usize;
             entry.compressed_size = total_out;
             entry.uncompressed_size = total_in;
-
-            entry.crc32 = crc32;
         }
+
         self.byte_count += entry.write_data_descriptor(&mut target).await?;
         self.target = Some(target);
 
@@ -658,10 +676,16 @@ where
 
             if entry.file_type().is_file() {
                 let file = tokio::fs::File::open(entry.path()).await?;
-                let ze = ZipEntry::new(entry_path_no_base, mtime, mode, true);
+                let ze = ZipEntry::new(entry_path_no_base, mtime, mode, true, None);
                 Ok(Some((ze, Some(file))))
             } else if entry.file_type().is_dir() {
-                let ze = ZipEntry::new(entry_path_no_base, mtime, mode, false);
+                let ze = ZipEntry::new(entry_path_no_base, mtime, mode, false, None);
+                let content: Option<tokio::fs::File> = None;
+                Ok(Some((ze, content)))
+            } else if entry.file_type().is_symlink() {
+                let target = std::fs::read_link(entry.path())?;
+                let ze =
+                    ZipEntry::new(entry_path_no_base, mtime, mode, true, Some(target.as_ref()));
                 let content: Option<tokio::fs::File> = None;
                 Ok(Some((ze, content)))
             } else {
-- 
2.39.2

[pbs-devel] [PATCH v2 backup 1/1] fix #4995: pxar: Include symlinks in zip file restore

[Filip Schauer]

Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
 pbs-client/src/pxar/extract.rs | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/pbs-client/src/pxar/extract.rs b/pbs-client/src/pxar/extract.rs
index f78e06c2..6f23c144 100644
--- a/pbs-client/src/pxar/extract.rs
+++ b/pbs-client/src/pxar/extract.rs
@@ -998,6 +998,7 @@ where
                 metadata.stat.mtime.secs,
                 metadata.stat.mode as u16,
                 false,
+                None,
             );
             zip.add_entry::<FileContents<T>>(entry, None).await?;
         }
@@ -1017,6 +1018,7 @@ where
                         metadata.stat.mtime.secs,
                         metadata.stat.mode as u16,
                         true,
+                        None,
                     );
                     zip.add_entry(entry, decoder.contents())
                         .await
@@ -1035,6 +1037,7 @@ where
                         metadata.stat.mtime.secs,
                         metadata.stat.mode as u16,
                         true,
+                        None,
                     );
                     zip.add_entry(entry, decoder.contents())
                         .await
@@ -1047,9 +1050,24 @@ where
                         metadata.stat.mtime.secs,
                         metadata.stat.mode as u16,
                         false,
+                        None,
                     );
                     zip.add_entry::<FileContents<T>>(entry, None).await?;
                 }
+                EntryKind::Symlink(link) => {
+                    log::debug!("adding '{}' to zip", path.display());
+                    let realpath = Path::new(link);
+
+                    let entry = ZipEntry::new(
+                        path,
+                        metadata.stat.mtime.secs,
+                        metadata.stat.mode as u16,
+                        true,
+                        Some(realpath),
+                    );
+
+                    zip.add_entry::<FileContents<T>>(entry, None).await?;
+                }
                 _ => {} // ignore all else
             };
         }
-- 
2.39.2

Re: [pbs-devel] [PATCH v2 proxmox 1/1] fix #4995: compression: Include symlinks in zip file restore

[Dominik Csapak]

a few high level comments (did not look too closely on the code):

* if we change the function/struct parameters anyway wouldn't it make more sense
   to add a 'filetype' enum instead of having a 'is_file' bool and a symlink option?
   i used the bool because we only had files + dirs, but now we add a third type,
   but imho representing the types properly would be better
   we even could put the content into the various enum parts
   (or even make the ZipEntry an enum altogether?)
   i know this refactoring is much more work than slapping just a new parameter on,
   but it makes it easier to understand the code and expand it if we need it
   (honestly i probably should have done so initially when adding the code)


* what i'm missing here a bit is the source on how to encode symlinks in zip.
   the "official" zip spec[0] only talks about (symbolic) links in the description
   of a "-UNIX Extra Field" but you simply encode it here into the content
   how did you arrive at that solution?
   (also generally a commit message is a good idea ;) )

* for these things i'd also like a short comment (does not have to be in the
   commit message) on which systems you did test this, e.g. zipinfo/zip/unar on linux
   explorer on windows, mac (?), etc.

* if you want to go the extra mile, i guess this would be a good time to add tests
   that create a new zip from test data, to see if they don't break with your changes

0: https://pkware.cachefly.net/webdocs/APPNOTE/APPNOTE-6.3.9.TXT

On 11/23/23 14:06, Filip Schauer wrote:
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---
>   proxmox-compression/src/zip.rs | 46 ++++++++++++++++++++++++++--------
>   1 file changed, 35 insertions(+), 11 deletions(-)
> 
> diff --git a/proxmox-compression/src/zip.rs b/proxmox-compression/src/zip.rs
> index d2d3fd8..e30f50a 100644
> --- a/proxmox-compression/src/zip.rs
> +++ b/proxmox-compression/src/zip.rs
> @@ -204,6 +204,7 @@ pub struct ZipEntry {
>       offset: u64,
>       is_file: bool,
>       is_utf8_filename: bool,
> +    symlink_target: Option<OsString>,
>   }
>   
>   impl ZipEntry {
> @@ -211,7 +212,13 @@ impl ZipEntry {
>       ///
>       /// if is_file is false the path will contain an trailing separator,
>       /// so that the zip file understands that it is a directory
> -    pub fn new<P: AsRef<Path>>(path: P, mtime: i64, mode: u16, is_file: bool) -> Self {
> +    pub fn new<P: AsRef<Path>>(
> +        path: P,
> +        mtime: i64,
> +        mode: u16,
> +        is_file: bool,
> +        symlink_target: Option<P>,
> +    ) -> Self {
>           let mut relpath = PathBuf::new();
>   
>           for comp in path.as_ref().components() {
> @@ -226,6 +233,7 @@ impl ZipEntry {
>   
>           let filename: OsString = relpath.into();
>           let is_utf8_filename = filename.to_str().is_some();
> +        let symlink_target_osstr =  symlink_target.map(|x| x.as_ref().into());
>   
>           Self {
>               filename,
> @@ -237,6 +245,7 @@ impl ZipEntry {
>               offset: 0,
>               is_file,
>               is_utf8_filename,
> +            symlink_target: symlink_target_osstr,
>           }
>       }
>   
> @@ -360,7 +369,9 @@ impl ZipEntry {
>                   comment_len: 0,
>                   start_disk: 0,
>                   internal_flags: 0,
> -                external_flags: (self.mode as u32) << 16 | (!self.is_file as u32) << 4,
> +                external_flags: (self.mode as u32) << 16
> +                | (self.symlink_target.is_some() as u32) << 5
> +                | (!self.is_file as u32) << 4,
>                   offset,
>               },
>           )
> @@ -486,23 +497,30 @@ impl<W: AsyncWrite + Unpin> ZipEncoder<W> {
>               .ok_or_else(|| format_err!("had no target during add entry"))?;
>           entry.offset = self.byte_count.try_into()?;
>           self.byte_count += entry.write_local_header(&mut target).await?;
> -        if let Some(content) = content {
> -            let mut reader = HashWrapper::new(content);
> +
> +        if content.is_some() || entry.symlink_target.is_some() {
>               let mut enc = DeflateEncoder::with_quality(target, Level::Fastest);
>   
> -            enc.compress(&mut reader).await?;
> +            if let Some(content) = content {
> +                let mut reader = HashWrapper::new(content);
> +                enc.compress(&mut reader).await?;
> +                entry.crc32 = reader.finish().0;
> +            } else if let Some(symlink_target) = entry.symlink_target.as_ref() {
> +                let cursor = std::io::Cursor::new(symlink_target.as_bytes());
> +                let mut reader = HashWrapper::new(cursor);
> +                enc.compress(&mut reader).await?;
> +                entry.crc32 = reader.finish().0;
> +            }
> +
>               let total_in = enc.total_in();
>               let total_out = enc.total_out();
>               target = enc.into_inner();
>   
> -            let (crc32, _reader) = reader.finish();
> -
>               self.byte_count += total_out as usize;
>               entry.compressed_size = total_out;
>               entry.uncompressed_size = total_in;
> -
> -            entry.crc32 = crc32;
>           }
> +
>           self.byte_count += entry.write_data_descriptor(&mut target).await?;
>           self.target = Some(target);
>   
> @@ -658,10 +676,16 @@ where
>   
>               if entry.file_type().is_file() {
>                   let file = tokio::fs::File::open(entry.path()).await?;
> -                let ze = ZipEntry::new(entry_path_no_base, mtime, mode, true);
> +                let ze = ZipEntry::new(entry_path_no_base, mtime, mode, true, None);
>                   Ok(Some((ze, Some(file))))
>               } else if entry.file_type().is_dir() {
> -                let ze = ZipEntry::new(entry_path_no_base, mtime, mode, false);
> +                let ze = ZipEntry::new(entry_path_no_base, mtime, mode, false, None);
> +                let content: Option<tokio::fs::File> = None;
> +                Ok(Some((ze, content)))
> +            } else if entry.file_type().is_symlink() {
> +                let target = std::fs::read_link(entry.path())?;
> +                let ze =
> +                    ZipEntry::new(entry_path_no_base, mtime, mode, true, Some(target.as_ref()));
>                   let content: Option<tokio::fs::File> = None;
>                   Ok(Some((ze, content)))
>               } else {

Re: [pbs-devel] [PATCH v2 proxmox 1/1] fix #4995: compression: Include symlinks in zip file restore

[Lukas Wagner]

On 11/24/23 08:56, Dominik Csapak wrote:
> * if you want to go the extra mile, i guess this would be a good time to 
> add tests
>    that create a new zip from test data, to see if they don't break with 
> your changes
> 

I agree!

I think writing tests for changes should be the default and not be 
considered as 'the extra mile' - especially considering that it should 
be relatively easy to write tests for this crate (no weird
dependencies, no need to run as a certain user, etc.)

-- 
- Lukas

Re: [pbs-devel] [PATCH v2 proxmox 1/1] fix #4995: compression: Include symlinks in zip file restore

[Filip Schauer]

Patch v3 available:

https://lists.proxmox.com/pipermail/pbs-devel/2023-December/007538.html

On 24/11/2023 12:03, Lukas Wagner wrote:
> On 11/24/23 08:56, Dominik Csapak wrote:
>> * if you want to go the extra mile, i guess this would be a good time 
>> to add tests
>>    that create a new zip from test data, to see if they don't break 
>> with your changes
>>
>
> I agree!
>
> I think writing tests for changes should be the default and not be 
> considered as 'the extra mile' - especially considering that it should 
> be relatively easy to write tests for this crate (no weird
> dependencies, no need to run as a certain user, etc.)
>