Re: [pbs-devel] [PATCH proxmox-backup] fix #2998: encode mtime as i64 instead of u64

[Dominik Csapak <d.csapak@proxmox.com>]

On 10/21/20 12:05 PM, Dominik Csapak wrote:
> saves files mtime as i64 instead of u64 which enables backup of
> files with negative mtime
> 
> the catalog_decode_i64 is compatible to encoded u64 values
> but not reverse, so all "old" catalogs can be read with the new
> decoder, but catalogs that contain negative mtimes will decode wrongly
> on older clients
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>   src/backup/catalog.rs | 76 +++++++++++++++++++++++++++++++++++++++----
>   src/pxar/catalog.rs   |  2 +-
>   src/pxar/create.rs    |  2 +-
>   3 files changed, 72 insertions(+), 8 deletions(-)
> 
> diff --git a/src/backup/catalog.rs b/src/backup/catalog.rs
> index d1f519e5..dd892a88 100644
> --- a/src/backup/catalog.rs
> +++ b/src/backup/catalog.rs
> @@ -78,7 +78,7 @@ pub struct DirEntry {
>   #[derive(Clone, Debug, PartialEq)]
>   pub enum DirEntryAttribute {
>       Directory { start: u64 },
> -    File { size: u64, mtime: u64 },
> +    File { size: u64, mtime: i64 },
>       Symlink,
>       Hardlink,
>       BlockDevice,
> @@ -89,7 +89,7 @@ pub enum DirEntryAttribute {
>   
>   impl DirEntry {
>   
> -    fn new(etype: CatalogEntryType, name: Vec<u8>, start: u64, size: u64, mtime:u64) -> Self {
> +    fn new(etype: CatalogEntryType, name: Vec<u8>, start: u64, size: u64, mtime: i64) -> Self {
>           match etype {
>               CatalogEntryType::Directory => {
>                   DirEntry { name, attr: DirEntryAttribute::Directory { start } }
> @@ -184,7 +184,7 @@ impl DirInfo {
>                   catalog_encode_u64(writer, name.len() as u64)?;
>                   writer.write_all(name)?;
>                   catalog_encode_u64(writer, *size)?;
> -                catalog_encode_u64(writer, *mtime)?;
> +                catalog_encode_i64(writer, *mtime)?;
>               }
>               DirEntry { name, attr: DirEntryAttribute::Symlink } => {
>                   writer.write_all(&[CatalogEntryType::Symlink as u8])?;
> @@ -234,7 +234,7 @@ impl DirInfo {
>           Ok((self.name, data))
>       }
>   
> -    fn parse<C: FnMut(CatalogEntryType, &[u8], u64, u64, u64) -> Result<bool, Error>>(
> +    fn parse<C: FnMut(CatalogEntryType, &[u8], u64, u64, i64) -> Result<bool, Error>>(
>           data: &[u8],
>           mut callback: C,
>       ) -> Result<(), Error> {
> @@ -265,7 +265,7 @@ impl DirInfo {
>                   }
>                   CatalogEntryType::File => {
>                       let size = catalog_decode_u64(&mut cursor)?;
> -                    let mtime = catalog_decode_u64(&mut cursor)?;
> +                    let mtime = catalog_decode_i64(&mut cursor)?;
>                       callback(etype, name, 0, size, mtime)?
>                   }
>                   _ => {
> @@ -362,7 +362,7 @@ impl <W: Write> BackupCatalogWriter for CatalogWriter<W> {
>           Ok(())
>       }
>   
> -    fn add_file(&mut self, name: &CStr, size: u64, mtime: u64) -> Result<(), Error> {
> +    fn add_file(&mut self, name: &CStr, size: u64, mtime: i64) -> Result<(), Error> {
>           let dir = self.dirstack.last_mut().ok_or_else(|| format_err!("outside root"))?;
>           let name = name.to_bytes().to_vec();
>           dir.entries.push(DirEntry { name, attr: DirEntryAttribute::File { size, mtime } });
> @@ -587,6 +587,70 @@ impl <R: Read + Seek> CatalogReader<R> {
>       }
>   }
>   
> +/// Serialize i64 as short, variable length byte sequence
> +///
> +/// Stores 7 bits per byte, Bit 8 indicates the end of the sequence (when not set).
> +/// We limit values to a maximum of 2^63.
> +/// If the first byte is 128 (all zero but the end marker), the number is negative

typo here: s/first/last/ and 0x0 instead of 128
i can send a v2 or a follow-up

> +pub fn catalog_encode_i64<W: Write>(writer: &mut W, v: i64) -> Result<(), Error> {
> +    let mut enc = Vec::new();
> +
> +    let mut d = if v < 0 {
> +        (-1 * v) as u64
> +    } else {
> +        v as u64
> +    };
> +
> +    loop {
> +        if d < 128 {
> +            if v < 0 {
> +                enc.push(128 | d as u8);
> +                enc.push(0u8);
> +            } else {
> +                enc.push(d as u8);
> +            }
> +            break;
> +        }
> +        enc.push((128 | (d & 127)) as u8);
> +        d = d >> 7;
> +    }
> +    writer.write_all(&enc)?;
> +
> +    Ok(())
> +}
> +
> +/// Deserialize i64 from variable length byte sequence
> +///
> +/// We currently read maximal 10 bytes, which give a maximum of 63 bits + sign.
> +/// this method is compatible with catalog_encode_u64, meaning it can
> +/// properly decode values that are encoded with catalog_encode_u64
> +/// (which are <2^63)
> +pub fn catalog_decode_i64<R: Read>(reader: &mut R) -> Result<i64, Error> {
> +
> +    let mut v: u64 = 0;
> +    let mut buf = [0u8];
> +
> +    for i in 0..10 { // only allow 10 bytes (63 bits + sign marker)
> +        if buf.is_empty() {
> +            bail!("decode_i64 failed - unexpected EOB");
> +        }
> +        reader.read_exact(&mut buf)?;
> +
> +        let t = buf[0];
> +
> +        if t == 0 {
> +            return Ok(v as i64 * -1);
> +        } else if t < 128 {
> +            v |= (t as u64) << (i*7);
> +            return Ok(v as i64);
> +        } else {
> +            v |= ((t & 127) as u64) << (i*7);
> +        }
> +    }
> +
> +    bail!("decode_i64 failed - missing end marker");
> +}
> +
>   /// Serialize u64 as short, variable length byte sequence
>   ///
>   /// Stores 7 bits per byte, Bit 8 indicates the end of the sequence (when not set).
> diff --git a/src/pxar/catalog.rs b/src/pxar/catalog.rs
> index 96b42427..02b57dfa 100644
> --- a/src/pxar/catalog.rs
> +++ b/src/pxar/catalog.rs
> @@ -9,7 +9,7 @@ use std::ffi::CStr;
>   pub trait BackupCatalogWriter {
>       fn start_directory(&mut self, name: &CStr) -> Result<(), Error>;
>       fn end_directory(&mut self) -> Result<(), Error>;
> -    fn add_file(&mut self, name: &CStr, size: u64, mtime: u64) -> Result<(), Error>;
> +    fn add_file(&mut self, name: &CStr, size: u64, mtime: i64) -> Result<(), Error>;
>       fn add_symlink(&mut self, name: &CStr) -> Result<(), Error>;
>       fn add_hardlink(&mut self, name: &CStr) -> Result<(), Error>;
>       fn add_block_device(&mut self, name: &CStr) -> Result<(), Error>;
> diff --git a/src/pxar/create.rs b/src/pxar/create.rs
> index 74c49050..6ba6f15c 100644
> --- a/src/pxar/create.rs
> +++ b/src/pxar/create.rs
> @@ -535,7 +535,7 @@ impl<'a, 'b> Archiver<'a, 'b> {
>   
>                   let file_size = stat.st_size as u64;
>                   if let Some(ref mut catalog) = self.catalog {
> -                    catalog.add_file(c_file_name, file_size, stat.st_mtime as u64)?;
> +                    catalog.add_file(c_file_name, file_size, stat.st_mtime)?;
>                   }
>   
>                   let offset: LinkOffset =
>