* [pbs-devel] [PATCH v2 stable-2 pxar proxmox-backup 0/2] backport pxar format version check @ 2024-06-05 15:41 Christian Ebner 2024-06-05 15:41 ` [pbs-devel] [PATCH v2 stable-2 pxar 1/1] format/decoder/accessor: backport pxar entry type `Version` Christian Ebner 2024-06-05 15:41 ` [pbs-devel] [PATCH v2 stable-2 proxmox-backup 2/2] client: pxar: bail on incompatible format versions Christian Ebner 0 siblings, 2 replies; 6+ messages in thread From: Christian Ebner @ 2024-06-05 15:41 UTC (permalink / raw) To: pbs-devel This patches backport the format version entry present in pxar archives to start with format version 2 and allows to early bail on archives when an incompatible format version is detected. changes since version 1: - adapted to FormatVersion deserializer as present in bumped pxar version - added check to cover also access via Accessor instances - removed display implementation and adapted bail message - added additional checks to pxar bin pxar: Christian Ebner (1): format/decoder/accessor: backport pxar entry type `Version` examples/mk-format-hashes.rs | 5 +++++ src/accessor/mod.rs | 28 ++++++++++++++++++++++++++-- src/decoder/mod.rs | 28 ++++++++++++++++++++++++++-- src/format/mod.rs | 19 +++++++++++++++++++ src/lib.rs | 3 +++ tests/simple/fs.rs | 1 + 6 files changed, 80 insertions(+), 4 deletions(-) proxmox-backup: Christian Ebner (1): client: pxar: bail on incompatible format versions pbs-client/src/catalog_shell.rs | 2 +- pbs-client/src/pxar/extract.rs | 3 +++ pbs-client/src/pxar/tools.rs | 14 ++++++++------ pxar-bin/src/main.rs | 6 +++++- 4 files changed, 17 insertions(+), 8 deletions(-) -- 2.30.2 _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* [pbs-devel] [PATCH v2 stable-2 pxar 1/1] format/decoder/accessor: backport pxar entry type `Version` 2024-06-05 15:41 [pbs-devel] [PATCH v2 stable-2 pxar proxmox-backup 0/2] backport pxar format version check Christian Ebner @ 2024-06-05 15:41 ` Christian Ebner 2024-06-06 8:21 ` Fabian Grünbichler 2024-06-05 15:41 ` [pbs-devel] [PATCH v2 stable-2 proxmox-backup 2/2] client: pxar: bail on incompatible format versions Christian Ebner 1 sibling, 1 reply; 6+ messages in thread From: Christian Ebner @ 2024-06-05 15:41 UTC (permalink / raw) To: pbs-devel Backports the pxar format entry type `Version` and the associated decoder methods. The format version entry is expected once as the first entry of the pxar archive, marked with a `PXAR_FORMAT_VERSION` header followed by the encoded version number for archives with format version 2 or higher. If not present, the default format version 1 is assumed as encoding format for the archive. The entry allows to early detect and bail if an incompatible archive version is encountered. The format version entry is not backwards compatible to pxar format version 1. Signed-off-by: Christian Ebner <c.ebner@proxmox.com> --- Note: This patch is intended to be applied on a dedicated branch to be forked from previous master commit 675ecff32fbeff0973eaea016c4b8f3877015adb examples/mk-format-hashes.rs | 5 +++++ src/accessor/mod.rs | 28 ++++++++++++++++++++++++++-- src/decoder/mod.rs | 28 ++++++++++++++++++++++++++-- src/format/mod.rs | 19 +++++++++++++++++++ src/lib.rs | 3 +++ tests/simple/fs.rs | 1 + 6 files changed, 80 insertions(+), 4 deletions(-) diff --git a/examples/mk-format-hashes.rs b/examples/mk-format-hashes.rs index 6e00654..afd0924 100644 --- a/examples/mk-format-hashes.rs +++ b/examples/mk-format-hashes.rs @@ -1,6 +1,11 @@ use pxar::format::hash_filename; const CONSTANTS: &[(&str, &str, &str)] = &[ + ( + "Pxar format version entry, fallback to version 1 if not present", + "PXAR_FORMAT_VERSION", + "__PROXMOX_FORMAT_VERSION__", + ), ( "Beginning of an entry (current version).", "PXAR_ENTRY", diff --git a/src/accessor/mod.rs b/src/accessor/mod.rs index 6a2de73..73d79e1 100644 --- a/src/accessor/mod.rs +++ b/src/accessor/mod.rs @@ -17,7 +17,7 @@ use endian_trait::Endian; use crate::binary_tree_array; use crate::decoder::{self, DecoderImpl}; -use crate::format::{self, GoodbyeItem}; +use crate::format::{self, FormatVersion, GoodbyeItem}; use crate::util; use crate::{Entry, EntryKind}; @@ -185,11 +185,23 @@ pub(crate) struct AccessorImpl<T> { } impl<T: ReadAt> AccessorImpl<T> { - pub async fn new(input: T, size: u64) -> io::Result<Self> { + pub async fn new(mut input: T, size: u64) -> io::Result<Self> { if size < (size_of::<GoodbyeItem>() as u64) { io_bail!("too small to contain a pxar archive"); } + let header: format::Header = read_entry_at(&mut input, 0).await?; + header.check_header_size()?; + + if header.htype == format::PXAR_FORMAT_VERSION { + let version: u64 = read_entry_at( + &mut input, + size_of::<format::Header>() as u64, + ) + .await?; + FormatVersion::deserialize(version)?; + } + Ok(Self { input, size, @@ -293,6 +305,12 @@ impl<T: Clone + ReadAt> AccessorImpl<T> { .next() .await .ok_or_else(|| io_format_err!("unexpected EOF while decoding file entry"))??; + + if let EntryKind::Version(_) = entry.kind() { + // client is incompatible with any format version entry (version 1 is never encoded) + io_bail!("got format version not compatible with this client."); + } + Ok(FileEntryImpl { input: self.input.clone(), entry, @@ -516,6 +534,12 @@ impl<T: Clone + ReadAt> DirectoryImpl<T> { .next() .await .ok_or_else(|| io_format_err!("unexpected EOF while decoding directory entry"))??; + + if let EntryKind::Version(_) = entry.kind() { + // client is incompatible with any format version entry (version 1 is never encoded) + io_bail!("got format version not compatible with this client."); + } + Ok((entry, decoder)) } diff --git a/src/decoder/mod.rs b/src/decoder/mod.rs index d1fb911..c6eae9f 100644 --- a/src/decoder/mod.rs +++ b/src/decoder/mod.rs @@ -17,7 +17,7 @@ use std::task::{Context, Poll}; use endian_trait::Endian; -use crate::format::{self, Header}; +use crate::format::{self, FormatVersion, Header}; use crate::util::{self, io_err_other}; use crate::{Entry, EntryKind, Metadata}; @@ -162,6 +162,7 @@ pub(crate) struct DecoderImpl<T> { eof_after_entry: bool, } +#[derive(Clone, PartialEq)] enum State { Begin, Default, @@ -236,7 +237,16 @@ impl<I: SeqRead> DecoderImpl<I> { loop { match self.state { State::Eof => return Ok(None), - State::Begin => return self.read_next_entry().await.map(Some), + State::Begin => { + let entry = self.read_next_entry().await.map(Some); + if let Ok(Some(ref entry)) = entry { + if let EntryKind::Version(_) = entry.kind() { + // client is incompatible with any format version entry (version 1 is never encoded) + io_bail!("got format version not compatible with this client."); + } + } + return entry; + } State::Default => { // we completely finished an entry, so now we're going "up" in the directory // hierarchy and parse the next PXAR_FILENAME or the PXAR_GOODBYE: @@ -354,6 +364,7 @@ impl<I: SeqRead> DecoderImpl<I> { } async fn read_next_entry_or_eof(&mut self) -> io::Result<Option<Entry>> { + let previous_state = self.state.clone(); self.state = State::Default; self.entry.clear_data(); @@ -373,6 +384,14 @@ impl<I: SeqRead> DecoderImpl<I> { self.entry.metadata = Metadata::default(); self.entry.kind = EntryKind::Hardlink(self.read_hardlink().await?); + Ok(Some(self.entry.take())) + } else if header.htype == format::PXAR_FORMAT_VERSION { + if previous_state != State::Begin { + io_bail!("Got format version entry at unexpected position"); + } + self.current_header = header; + self.entry.kind = EntryKind::Version(self.read_format_version().await?); + Ok(Some(self.entry.take())) } else if header.htype == format::PXAR_ENTRY || header.htype == format::PXAR_ENTRY_V1 { if header.htype == format::PXAR_ENTRY { @@ -661,6 +680,11 @@ impl<I: SeqRead> DecoderImpl<I> { async fn read_quota_project_id(&mut self) -> io::Result<format::QuotaProjectId> { self.read_simple_entry("quota project id").await } + + async fn read_format_version(&mut self) -> io::Result<format::FormatVersion> { + let version: u64 = seq_read_entry(&mut self.input).await?; + FormatVersion::deserialize(version) + } } /// Reader for file contents inside a pxar archive. diff --git a/src/format/mod.rs b/src/format/mod.rs index bfea9f6..2e21635 100644 --- a/src/format/mod.rs +++ b/src/format/mod.rs @@ -6,6 +6,7 @@ //! item data. //! //! An archive contains items in the following order: +//! * `FORMAT_VERSION` -- (optional for v1), version of encoding format //! * `ENTRY` -- containing general stat() data and related bits //! * `XATTR` -- one extended attribute //! * ... -- more of these when there are multiple defined @@ -79,6 +80,8 @@ pub mod mode { } // Generated by `cargo run --example mk-format-hashes` +/// Pxar format version entry, fallback to version 1 if not present +pub const PXAR_FORMAT_VERSION: u64 = 0x730f6c75df16a40d; /// Beginning of an entry (current version). pub const PXAR_ENTRY: u64 = 0xd5956474e588acef; /// Previous version of the entry struct @@ -177,6 +180,7 @@ impl Header { impl Display for Header { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let readable = match self.htype { + PXAR_FORMAT_VERSION => "FORMAT_VERSION", PXAR_FILENAME => "FILENAME", PXAR_SYMLINK => "SYMLINK", PXAR_HARDLINK => "HARDLINK", @@ -540,6 +544,21 @@ impl From<&std::fs::Metadata> for Stat { } } +#[derive(Clone, Debug, Default, PartialEq)] +pub enum FormatVersion { + #[default] + Version1, +} + +impl FormatVersion { + pub fn deserialize(version: u64) -> Result<FormatVersion, io::Error> { + match version { + 1u64 => Ok(FormatVersion::Version1), + version => io_bail!("incompatible format version {version}") + } + } +} + #[derive(Clone, Debug)] pub struct Filename { pub name: Vec<u8>, diff --git a/src/lib.rs b/src/lib.rs index 210c4b1..b63d43c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -342,6 +342,9 @@ impl Acl { /// Identifies whether the entry is a file, symlink, directory, etc. #[derive(Clone, Debug)] pub enum EntryKind { + /// Pxar file format version + Version(format::FormatVersion), + /// Symbolic links. Symlink(format::Symlink), diff --git a/tests/simple/fs.rs b/tests/simple/fs.rs index 9a89c4d..fd13e65 100644 --- a/tests/simple/fs.rs +++ b/tests/simple/fs.rs @@ -229,6 +229,7 @@ impl Entry { })?)) }; match item.kind() { + PxarEntryKind::Version(_) => continue, PxarEntryKind::GoodbyeTable => break, PxarEntryKind::File { size, .. } => { let mut data = Vec::new(); -- 2.30.2 _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pbs-devel] [PATCH v2 stable-2 pxar 1/1] format/decoder/accessor: backport pxar entry type `Version` 2024-06-05 15:41 ` [pbs-devel] [PATCH v2 stable-2 pxar 1/1] format/decoder/accessor: backport pxar entry type `Version` Christian Ebner @ 2024-06-06 8:21 ` Fabian Grünbichler 2024-06-06 8:49 ` Christian Ebner 0 siblings, 1 reply; 6+ messages in thread From: Fabian Grünbichler @ 2024-06-06 8:21 UTC (permalink / raw) To: Proxmox Backup Server development discussion On June 5, 2024 5:41 pm, Christian Ebner wrote: > Backports the pxar format entry type `Version` and the associated > decoder methods. The format version entry is expected once as the > first entry of the pxar archive, marked with a `PXAR_FORMAT_VERSION` > header followed by the encoded version number for archives with > format version 2 or higher. > If not present, the default format version 1 is assumed as encoding > format for the archive. > > The entry allows to early detect and bail if an incompatible archive > version is encountered. > > The format version entry is not backwards compatible to pxar format > version 1. > > Signed-off-by: Christian Ebner <c.ebner@proxmox.com> > --- > Note: > > This patch is intended to be applied on a dedicated branch to be forked > from previous master commit 675ecff32fbeff0973eaea016c4b8f3877015adb > > examples/mk-format-hashes.rs | 5 +++++ > src/accessor/mod.rs | 28 ++++++++++++++++++++++++++-- > src/decoder/mod.rs | 28 ++++++++++++++++++++++++++-- > src/format/mod.rs | 19 +++++++++++++++++++ > src/lib.rs | 3 +++ > tests/simple/fs.rs | 1 + > 6 files changed, 80 insertions(+), 4 deletions(-) > > diff --git a/examples/mk-format-hashes.rs b/examples/mk-format-hashes.rs > index 6e00654..afd0924 100644 > --- a/examples/mk-format-hashes.rs > +++ b/examples/mk-format-hashes.rs > @@ -1,6 +1,11 @@ > use pxar::format::hash_filename; > > const CONSTANTS: &[(&str, &str, &str)] = &[ > + ( > + "Pxar format version entry, fallback to version 1 if not present", > + "PXAR_FORMAT_VERSION", > + "__PROXMOX_FORMAT_VERSION__", > + ), > ( > "Beginning of an entry (current version).", > "PXAR_ENTRY", > diff --git a/src/accessor/mod.rs b/src/accessor/mod.rs > index 6a2de73..73d79e1 100644 > --- a/src/accessor/mod.rs > +++ b/src/accessor/mod.rs > @@ -17,7 +17,7 @@ use endian_trait::Endian; > > use crate::binary_tree_array; > use crate::decoder::{self, DecoderImpl}; > -use crate::format::{self, GoodbyeItem}; > +use crate::format::{self, FormatVersion, GoodbyeItem}; > use crate::util; > use crate::{Entry, EntryKind}; > > @@ -185,11 +185,23 @@ pub(crate) struct AccessorImpl<T> { > } > > impl<T: ReadAt> AccessorImpl<T> { > - pub async fn new(input: T, size: u64) -> io::Result<Self> { > + pub async fn new(mut input: T, size: u64) -> io::Result<Self> { > if size < (size_of::<GoodbyeItem>() as u64) { > io_bail!("too small to contain a pxar archive"); > } > > + let header: format::Header = read_entry_at(&mut input, 0).await?; > + header.check_header_size()?; > + > + if header.htype == format::PXAR_FORMAT_VERSION { > + let version: u64 = read_entry_at( > + &mut input, > + size_of::<format::Header>() as u64, > + ) > + .await?; > + FormatVersion::deserialize(version)?; > + } is there some other way to construct the AccessorImpl? if not, wouldn't this check here be enough and the ones below can actually never trigger/happen? see below as well, I think the deserialize could just be an io_bail > + > Ok(Self { > input, > size, > @@ -293,6 +305,12 @@ impl<T: Clone + ReadAt> AccessorImpl<T> { > .next() > .await > .ok_or_else(|| io_format_err!("unexpected EOF while decoding file entry"))??; > + > + if let EntryKind::Version(_) = entry.kind() { > + // client is incompatible with any format version entry (version 1 is never encoded) > + io_bail!("got format version not compatible with this client."); > + } since no encoded version can be deserialized by the stable-2 parser, this cannot happen since the deserializer would have bailed before? > + > Ok(FileEntryImpl { > input: self.input.clone(), > entry, > @@ -516,6 +534,12 @@ impl<T: Clone + ReadAt> DirectoryImpl<T> { > .next() > .await > .ok_or_else(|| io_format_err!("unexpected EOF while decoding directory entry"))??; > + > + if let EntryKind::Version(_) = entry.kind() { > + // client is incompatible with any format version entry (version 1 is never encoded) > + io_bail!("got format version not compatible with this client."); > + } same here > + > Ok((entry, decoder)) > } > > diff --git a/src/decoder/mod.rs b/src/decoder/mod.rs > index d1fb911..c6eae9f 100644 > --- a/src/decoder/mod.rs > +++ b/src/decoder/mod.rs > @@ -17,7 +17,7 @@ use std::task::{Context, Poll}; > > use endian_trait::Endian; > > -use crate::format::{self, Header}; > +use crate::format::{self, FormatVersion, Header}; > use crate::util::{self, io_err_other}; > use crate::{Entry, EntryKind, Metadata}; > > @@ -162,6 +162,7 @@ pub(crate) struct DecoderImpl<T> { > eof_after_entry: bool, > } > > +#[derive(Clone, PartialEq)] > enum State { > Begin, > Default, > @@ -236,7 +237,16 @@ impl<I: SeqRead> DecoderImpl<I> { > loop { > match self.state { > State::Eof => return Ok(None), > - State::Begin => return self.read_next_entry().await.map(Some), > + State::Begin => { > + let entry = self.read_next_entry().await.map(Some); > + if let Ok(Some(ref entry)) = entry { > + if let EntryKind::Version(_) = entry.kind() { > + // client is incompatible with any format version entry (version 1 is never encoded) > + io_bail!("got format version not compatible with this client."); do we want to include the version here? but see below, I think we can skip this altogether since we never ever will encounter a valid Version entry.. > + } > + } > + return entry; > + } > State::Default => { > // we completely finished an entry, so now we're going "up" in the directory > // hierarchy and parse the next PXAR_FILENAME or the PXAR_GOODBYE: > @@ -354,6 +364,7 @@ impl<I: SeqRead> DecoderImpl<I> { > } > > async fn read_next_entry_or_eof(&mut self) -> io::Result<Option<Entry>> { > + let previous_state = self.state.clone(); > self.state = State::Default; > self.entry.clear_data(); > > @@ -373,6 +384,14 @@ impl<I: SeqRead> DecoderImpl<I> { > self.entry.metadata = Metadata::default(); > self.entry.kind = EntryKind::Hardlink(self.read_hardlink().await?); > > + Ok(Some(self.entry.take())) > + } else if header.htype == format::PXAR_FORMAT_VERSION { > + if previous_state != State::Begin { > + io_bail!("Got format version entry at unexpected position"); > + } technically any position is unexpected, so we could drop this check here.. > + self.current_header = header; > + self.entry.kind = EntryKind::Version(self.read_format_version().await?); we can skip this, since there can never be a valid Version entry, and just inline read_format_version as a single call to seq_read_entry followed by bailing? > + > Ok(Some(self.entry.take())) > } else if header.htype == format::PXAR_ENTRY || header.htype == format::PXAR_ENTRY_V1 { > if header.htype == format::PXAR_ENTRY { > @@ -661,6 +680,11 @@ impl<I: SeqRead> DecoderImpl<I> { > async fn read_quota_project_id(&mut self) -> io::Result<format::QuotaProjectId> { > self.read_simple_entry("quota project id").await > } > + > + async fn read_format_version(&mut self) -> io::Result<format::FormatVersion> { > + let version: u64 = seq_read_entry(&mut self.input).await?; > + FormatVersion::deserialize(version) > + } > } > > /// Reader for file contents inside a pxar archive. > diff --git a/src/format/mod.rs b/src/format/mod.rs > index bfea9f6..2e21635 100644 > --- a/src/format/mod.rs > +++ b/src/format/mod.rs > @@ -6,6 +6,7 @@ > //! item data. > //! > //! An archive contains items in the following order: > +//! * `FORMAT_VERSION` -- (optional for v1), version of encoding format > //! * `ENTRY` -- containing general stat() data and related bits > //! * `XATTR` -- one extended attribute > //! * ... -- more of these when there are multiple defined > @@ -79,6 +80,8 @@ pub mod mode { > } > > // Generated by `cargo run --example mk-format-hashes` > +/// Pxar format version entry, fallback to version 1 if not present > +pub const PXAR_FORMAT_VERSION: u64 = 0x730f6c75df16a40d; > /// Beginning of an entry (current version). > pub const PXAR_ENTRY: u64 = 0xd5956474e588acef; > /// Previous version of the entry struct > @@ -177,6 +180,7 @@ impl Header { > impl Display for Header { > fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { > let readable = match self.htype { > + PXAR_FORMAT_VERSION => "FORMAT_VERSION", > PXAR_FILENAME => "FILENAME", > PXAR_SYMLINK => "SYMLINK", > PXAR_HARDLINK => "HARDLINK", > @@ -540,6 +544,21 @@ impl From<&std::fs::Metadata> for Stat { > } > } > > +#[derive(Clone, Debug, Default, PartialEq)] > +pub enum FormatVersion { > + #[default] > + Version1, > +} > + > +impl FormatVersion { > + pub fn deserialize(version: u64) -> Result<FormatVersion, io::Error> { > + match version { > + 1u64 => Ok(FormatVersion::Version1), the 1u64 here is wrong, right? it can't ever be encoded that way.. so this can go straight to io_bail!, or we can even skip the deserialize altogether and just inline that bail above in `read_format_version` > + version => io_bail!("incompatible format version {version}") > + } > + } > +} > + > #[derive(Clone, Debug)] > pub struct Filename { > pub name: Vec<u8>, > diff --git a/src/lib.rs b/src/lib.rs > index 210c4b1..b63d43c 100644 > --- a/src/lib.rs > +++ b/src/lib.rs > @@ -342,6 +342,9 @@ impl Acl { > /// Identifies whether the entry is a file, symlink, directory, etc. > #[derive(Clone, Debug)] > pub enum EntryKind { > + /// Pxar file format version > + Version(format::FormatVersion), > + if we never construct such an entry, since it is always considered invalid, we can skip this? > /// Symbolic links. > Symlink(format::Symlink), > > diff --git a/tests/simple/fs.rs b/tests/simple/fs.rs > index 9a89c4d..fd13e65 100644 > --- a/tests/simple/fs.rs > +++ b/tests/simple/fs.rs > @@ -229,6 +229,7 @@ impl Entry { > })?)) > }; > match item.kind() { > + PxarEntryKind::Version(_) => continue, and as a result, this? > PxarEntryKind::GoodbyeTable => break, > PxarEntryKind::File { size, .. } => { > let mut data = Vec::new(); > -- > 2.30.2 > > > > _______________________________________________ > pbs-devel mailing list > pbs-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel > > > _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pbs-devel] [PATCH v2 stable-2 pxar 1/1] format/decoder/accessor: backport pxar entry type `Version` 2024-06-06 8:21 ` Fabian Grünbichler @ 2024-06-06 8:49 ` Christian Ebner 2024-06-06 9:05 ` Fabian Grünbichler 0 siblings, 1 reply; 6+ messages in thread From: Christian Ebner @ 2024-06-06 8:49 UTC (permalink / raw) To: Proxmox Backup Server development discussion, Fabian Grünbichler On 6/6/24 10:21, Fabian Grünbichler wrote: > On June 5, 2024 5:41 pm, Christian Ebner wrote: >> Backports the pxar format entry type `Version` and the associated >> decoder methods. The format version entry is expected once as the >> first entry of the pxar archive, marked with a `PXAR_FORMAT_VERSION` >> header followed by the encoded version number for archives with >> format version 2 or higher. >> If not present, the default format version 1 is assumed as encoding >> format for the archive. >> >> The entry allows to early detect and bail if an incompatible archive >> version is encountered. >> >> The format version entry is not backwards compatible to pxar format >> version 1. >> >> Signed-off-by: Christian Ebner <c.ebner@proxmox.com> >> --- >> Note: >> >> This patch is intended to be applied on a dedicated branch to be forked >> from previous master commit 675ecff32fbeff0973eaea016c4b8f3877015adb >> >> examples/mk-format-hashes.rs | 5 +++++ >> src/accessor/mod.rs | 28 ++++++++++++++++++++++++++-- >> src/decoder/mod.rs | 28 ++++++++++++++++++++++++++-- >> src/format/mod.rs | 19 +++++++++++++++++++ >> src/lib.rs | 3 +++ >> tests/simple/fs.rs | 1 + >> 6 files changed, 80 insertions(+), 4 deletions(-) >> >> diff --git a/examples/mk-format-hashes.rs b/examples/mk-format-hashes.rs >> index 6e00654..afd0924 100644 >> --- a/examples/mk-format-hashes.rs >> +++ b/examples/mk-format-hashes.rs >> @@ -1,6 +1,11 @@ >> use pxar::format::hash_filename; >> >> const CONSTANTS: &[(&str, &str, &str)] = &[ >> + ( >> + "Pxar format version entry, fallback to version 1 if not present", >> + "PXAR_FORMAT_VERSION", >> + "__PROXMOX_FORMAT_VERSION__", >> + ), >> ( >> "Beginning of an entry (current version).", >> "PXAR_ENTRY", >> diff --git a/src/accessor/mod.rs b/src/accessor/mod.rs >> index 6a2de73..73d79e1 100644 >> --- a/src/accessor/mod.rs >> +++ b/src/accessor/mod.rs >> @@ -17,7 +17,7 @@ use endian_trait::Endian; >> >> use crate::binary_tree_array; >> use crate::decoder::{self, DecoderImpl}; >> -use crate::format::{self, GoodbyeItem}; >> +use crate::format::{self, FormatVersion, GoodbyeItem}; >> use crate::util; >> use crate::{Entry, EntryKind}; >> >> @@ -185,11 +185,23 @@ pub(crate) struct AccessorImpl<T> { >> } >> >> impl<T: ReadAt> AccessorImpl<T> { >> - pub async fn new(input: T, size: u64) -> io::Result<Self> { >> + pub async fn new(mut input: T, size: u64) -> io::Result<Self> { >> if size < (size_of::<GoodbyeItem>() as u64) { >> io_bail!("too small to contain a pxar archive"); >> } >> >> + let header: format::Header = read_entry_at(&mut input, 0).await?; >> + header.check_header_size()?; >> + >> + if header.htype == format::PXAR_FORMAT_VERSION { >> + let version: u64 = read_entry_at( >> + &mut input, >> + size_of::<format::Header>() as u64, >> + ) >> + .await?; >> + FormatVersion::deserialize(version)?; >> + } > > is there some other way to construct the AccessorImpl? if not, wouldn't > this check here be enough and the ones below can actually never > trigger/happen? see below as well, I think the deserialize could just be > an io_bail True, I just wanted to keep the logic the same as for the current master, but I am fine send a new version simply bailing here instead. > >> + >> Ok(Self { >> input, >> size, >> @@ -293,6 +305,12 @@ impl<T: Clone + ReadAt> AccessorImpl<T> { >> .next() >> .await >> .ok_or_else(|| io_format_err!("unexpected EOF while decoding file entry"))??; >> + >> + if let EntryKind::Version(_) = entry.kind() { >> + // client is incompatible with any format version entry (version 1 is never encoded) >> + io_bail!("got format version not compatible with this client."); >> + } > > since no encoded version can be deserialized by the stable-2 parser, > this cannot happen since the deserializer would have bailed before? Also true, have these in place as additional safeguard. But I can drop this in a new version. > >> + >> Ok(FileEntryImpl { >> input: self.input.clone(), >> entry, >> @@ -516,6 +534,12 @@ impl<T: Clone + ReadAt> DirectoryImpl<T> { >> .next() >> .await >> .ok_or_else(|| io_format_err!("unexpected EOF while decoding directory entry"))??; >> + >> + if let EntryKind::Version(_) = entry.kind() { >> + // client is incompatible with any format version entry (version 1 is never encoded) >> + io_bail!("got format version not compatible with this client."); >> + } > > same here same as above > >> + >> Ok((entry, decoder)) >> } >> >> diff --git a/src/decoder/mod.rs b/src/decoder/mod.rs >> index d1fb911..c6eae9f 100644 >> --- a/src/decoder/mod.rs >> +++ b/src/decoder/mod.rs >> @@ -17,7 +17,7 @@ use std::task::{Context, Poll}; >> >> use endian_trait::Endian; >> >> -use crate::format::{self, Header}; >> +use crate::format::{self, FormatVersion, Header}; >> use crate::util::{self, io_err_other}; >> use crate::{Entry, EntryKind, Metadata}; >> >> @@ -162,6 +162,7 @@ pub(crate) struct DecoderImpl<T> { >> eof_after_entry: bool, >> } >> >> +#[derive(Clone, PartialEq)] >> enum State { >> Begin, >> Default, >> @@ -236,7 +237,16 @@ impl<I: SeqRead> DecoderImpl<I> { >> loop { >> match self.state { >> State::Eof => return Ok(None), >> - State::Begin => return self.read_next_entry().await.map(Some), >> + State::Begin => { >> + let entry = self.read_next_entry().await.map(Some); >> + if let Ok(Some(ref entry)) = entry { >> + if let EntryKind::Version(_) = entry.kind() { >> + // client is incompatible with any format version entry (version 1 is never encoded) >> + io_bail!("got format version not compatible with this client."); > > do we want to include the version here? but see below, I think we can > skip this altogether since we never ever will encounter a valid Version > entry.. Yes, same as above. I keep this as safeguard, but can drop this as well > >> + } >> + } >> + return entry; >> + } >> State::Default => { >> // we completely finished an entry, so now we're going "up" in the directory >> // hierarchy and parse the next PXAR_FILENAME or the PXAR_GOODBYE: >> @@ -354,6 +364,7 @@ impl<I: SeqRead> DecoderImpl<I> { >> } >> >> async fn read_next_entry_or_eof(&mut self) -> io::Result<Option<Entry>> { >> + let previous_state = self.state.clone(); >> self.state = State::Default; >> self.entry.clear_data(); >> >> @@ -373,6 +384,14 @@ impl<I: SeqRead> DecoderImpl<I> { >> self.entry.metadata = Metadata::default(); >> self.entry.kind = EntryKind::Hardlink(self.read_hardlink().await?); >> >> + Ok(Some(self.entry.take())) >> + } else if header.htype == format::PXAR_FORMAT_VERSION { >> + if previous_state != State::Begin { >> + io_bail!("Got format version entry at unexpected position"); >> + } > > technically any position is unexpected, so we could drop this check > here.. > >> + self.current_header = header; >> + self.entry.kind = EntryKind::Version(self.read_format_version().await?); > > we can skip this, since there can never be a valid Version entry, and > just inline read_format_version as a single call to seq_read_entry > followed by bailing? Okay, will do that. > >> + >> Ok(Some(self.entry.take())) >> } else if header.htype == format::PXAR_ENTRY || header.htype == format::PXAR_ENTRY_V1 { >> if header.htype == format::PXAR_ENTRY { >> @@ -661,6 +680,11 @@ impl<I: SeqRead> DecoderImpl<I> { >> async fn read_quota_project_id(&mut self) -> io::Result<format::QuotaProjectId> { >> self.read_simple_entry("quota project id").await >> } >> + >> + async fn read_format_version(&mut self) -> io::Result<format::FormatVersion> { >> + let version: u64 = seq_read_entry(&mut self.input).await?; >> + FormatVersion::deserialize(version) >> + } >> } >> >> /// Reader for file contents inside a pxar archive. >> diff --git a/src/format/mod.rs b/src/format/mod.rs >> index bfea9f6..2e21635 100644 >> --- a/src/format/mod.rs >> +++ b/src/format/mod.rs >> @@ -6,6 +6,7 @@ >> //! item data. >> //! >> //! An archive contains items in the following order: >> +//! * `FORMAT_VERSION` -- (optional for v1), version of encoding format >> //! * `ENTRY` -- containing general stat() data and related bits >> //! * `XATTR` -- one extended attribute >> //! * ... -- more of these when there are multiple defined >> @@ -79,6 +80,8 @@ pub mod mode { >> } >> >> // Generated by `cargo run --example mk-format-hashes` >> +/// Pxar format version entry, fallback to version 1 if not present >> +pub const PXAR_FORMAT_VERSION: u64 = 0x730f6c75df16a40d; >> /// Beginning of an entry (current version). >> pub const PXAR_ENTRY: u64 = 0xd5956474e588acef; >> /// Previous version of the entry struct >> @@ -177,6 +180,7 @@ impl Header { >> impl Display for Header { >> fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { >> let readable = match self.htype { >> + PXAR_FORMAT_VERSION => "FORMAT_VERSION", >> PXAR_FILENAME => "FILENAME", >> PXAR_SYMLINK => "SYMLINK", >> PXAR_HARDLINK => "HARDLINK", >> @@ -540,6 +544,21 @@ impl From<&std::fs::Metadata> for Stat { >> } >> } >> >> +#[derive(Clone, Debug, Default, PartialEq)] >> +pub enum FormatVersion { >> + #[default] >> + Version1, >> +} >> + >> +impl FormatVersion { >> + pub fn deserialize(version: u64) -> Result<FormatVersion, io::Error> { >> + match version { >> + 1u64 => Ok(FormatVersion::Version1), > > the 1u64 here is wrong, right? it can't ever be encoded that way.. so > this can go straight to io_bail!, or we can even skip the deserialize > altogether and just inline that bail above in `read_format_version` Same as above, I tried to keep the logic similar to current master, but can drop this as well. > >> + version => io_bail!("incompatible format version {version}") >> + } >> + } >> +} >> + >> #[derive(Clone, Debug)] >> pub struct Filename { >> pub name: Vec<u8>, >> diff --git a/src/lib.rs b/src/lib.rs >> index 210c4b1..b63d43c 100644 >> --- a/src/lib.rs >> +++ b/src/lib.rs >> @@ -342,6 +342,9 @@ impl Acl { >> /// Identifies whether the entry is a file, symlink, directory, etc. >> #[derive(Clone, Debug)] >> pub enum EntryKind { >> + /// Pxar file format version >> + Version(format::FormatVersion), >> + > > if we never construct such an entry, since it is always considered > invalid, we can skip this? Will drop this as well > >> /// Symbolic links. >> Symlink(format::Symlink), >> >> diff --git a/tests/simple/fs.rs b/tests/simple/fs.rs >> index 9a89c4d..fd13e65 100644 >> --- a/tests/simple/fs.rs >> +++ b/tests/simple/fs.rs >> @@ -229,6 +229,7 @@ impl Entry { >> })?)) >> }; >> match item.kind() { >> + PxarEntryKind::Version(_) => continue, > > and as a result, this? Same, given that I think this would not even require the patches on the pbs side anymore, as the decoder/accessor will always fail anyway. > >> PxarEntryKind::GoodbyeTable => break, >> PxarEntryKind::File { size, .. } => { >> let mut data = Vec::new(); >> -- >> 2.30.2 >> >> >> >> _______________________________________________ >> pbs-devel mailing list >> pbs-devel@lists.proxmox.com >> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel >> >> >> > > > _______________________________________________ > pbs-devel mailing list > pbs-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel > > _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pbs-devel] [PATCH v2 stable-2 pxar 1/1] format/decoder/accessor: backport pxar entry type `Version` 2024-06-06 8:49 ` Christian Ebner @ 2024-06-06 9:05 ` Fabian Grünbichler 0 siblings, 0 replies; 6+ messages in thread From: Fabian Grünbichler @ 2024-06-06 9:05 UTC (permalink / raw) To: Christian Ebner, Proxmox Backup Server development discussion On June 6, 2024 10:49 am, Christian Ebner wrote: > On 6/6/24 10:21, Fabian Grünbichler wrote: >> On June 5, 2024 5:41 pm, Christian Ebner wrote: >>> Backports the pxar format entry type `Version` and the associated >>> decoder methods. The format version entry is expected once as the >>> first entry of the pxar archive, marked with a `PXAR_FORMAT_VERSION` >>> header followed by the encoded version number for archives with >>> format version 2 or higher. >>> If not present, the default format version 1 is assumed as encoding >>> format for the archive. >>> >>> The entry allows to early detect and bail if an incompatible archive >>> version is encountered. >>> >>> The format version entry is not backwards compatible to pxar format >>> version 1. >>> >>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com> >> >>> /// Symbolic links. >>> Symlink(format::Symlink), >>> >>> diff --git a/tests/simple/fs.rs b/tests/simple/fs.rs >>> index 9a89c4d..fd13e65 100644 >>> --- a/tests/simple/fs.rs >>> +++ b/tests/simple/fs.rs >>> @@ -229,6 +229,7 @@ impl Entry { >>> })?)) >>> }; >>> match item.kind() { >>> + PxarEntryKind::Version(_) => continue, >> >> and as a result, this? > > Same, given that I think this would not even require the patches on the > pbs side anymore, as the decoder/accessor will always fail anyway. that was my conclusion as well (still needs a stable-2 rebuild with the bumped stable-2 pxar of course). _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* [pbs-devel] [PATCH v2 stable-2 proxmox-backup 2/2] client: pxar: bail on incompatible format versions 2024-06-05 15:41 [pbs-devel] [PATCH v2 stable-2 pxar proxmox-backup 0/2] backport pxar format version check Christian Ebner 2024-06-05 15:41 ` [pbs-devel] [PATCH v2 stable-2 pxar 1/1] format/decoder/accessor: backport pxar entry type `Version` Christian Ebner @ 2024-06-05 15:41 ` Christian Ebner 1 sibling, 0 replies; 6+ messages in thread From: Christian Ebner @ 2024-06-05 15:41 UTC (permalink / raw) To: pbs-devel Implementing the change detection mode with reusable file payloads required a breaking change in the pxar file format. Unfortunately, there initial format does not encode a version for clients to check compatibility to. Therefore, a pxar format version entry is introduced in version 2 of the file format. Add a compatibility check based on that entry also for the client, making it possible to bail early when an incompatible archive is encountered. Signed-off-by: Christian Ebner <c.ebner@proxmox.com> --- pbs-client/src/catalog_shell.rs | 2 +- pbs-client/src/pxar/extract.rs | 3 +++ pbs-client/src/pxar/tools.rs | 14 ++++++++------ pxar-bin/src/main.rs | 6 +++++- 4 files changed, 17 insertions(+), 8 deletions(-) diff --git a/pbs-client/src/catalog_shell.rs b/pbs-client/src/catalog_shell.rs index 98af5699..8bb2f3e0 100644 --- a/pbs-client/src/catalog_shell.rs +++ b/pbs-client/src/catalog_shell.rs @@ -767,7 +767,7 @@ impl Shell { let file = Self::walk_pxar_archive(&self.accessor, &mut stack).await?; std::io::stdout() - .write_all(crate::pxar::format_multi_line_entry(file.entry()).as_bytes())?; + .write_all(crate::pxar::format_multi_line_entry(file.entry())?.as_bytes())?; Ok(()) } diff --git a/pbs-client/src/pxar/extract.rs b/pbs-client/src/pxar/extract.rs index f6c1991f..e8412c3d 100644 --- a/pbs-client/src/pxar/extract.rs +++ b/pbs-client/src/pxar/extract.rs @@ -119,6 +119,9 @@ where None => current_match, }; match (did_match, entry.kind()) { + (_, EntryKind::Version(_)) => { + bail!("got format version incompatible with this client"); + } (_, EntryKind::Directory) => { callback(entry.path()); diff --git a/pbs-client/src/pxar/tools.rs b/pbs-client/src/pxar/tools.rs index 844a0f73..afc17dff 100644 --- a/pbs-client/src/pxar/tools.rs +++ b/pbs-client/src/pxar/tools.rs @@ -120,12 +120,13 @@ fn format_mtime(mtime: &StatxTimestamp) -> String { format!("{}.{}", mtime.secs, mtime.nanos) } -pub fn format_single_line_entry(entry: &Entry) -> String { +pub fn format_single_line_entry(entry: &Entry) -> Result<String, Error> { let mode_string = mode_string(entry); let meta = entry.metadata(); let (size, link) = match entry.kind() { + EntryKind::Version(_) => bail!("got format version incompatible with this client"), EntryKind::File { size, .. } => (format!("{}", *size), String::new()), EntryKind::Symlink(link) => ("0".to_string(), format!(" -> {:?}", link.as_os_str())), EntryKind::Hardlink(link) => ("0".to_string(), format!(" -> {:?}", link.as_os_str())), @@ -135,7 +136,7 @@ pub fn format_single_line_entry(entry: &Entry) -> String { let owner_string = format!("{}/{}", meta.stat.uid, meta.stat.gid); - format!( + Ok(format!( "{} {:<13} {} {:>8} {:?}{}", mode_string, owner_string, @@ -143,15 +144,16 @@ pub fn format_single_line_entry(entry: &Entry) -> String { size, entry.path(), link, - ) + )) } -pub fn format_multi_line_entry(entry: &Entry) -> String { +pub fn format_multi_line_entry(entry: &Entry) -> Result<String, Error> { let mode_string = mode_string(entry); let meta = entry.metadata(); let (size, link, type_name) = match entry.kind() { + EntryKind::Version(_) => bail!("got format version incompatible with this client"), EntryKind::File { size, .. } => (format!("{}", *size), String::new(), "file"), EntryKind::Symlink(link) => ( "0".to_string(), @@ -185,7 +187,7 @@ pub fn format_multi_line_entry(entry: &Entry) -> String { Err(_) => std::borrow::Cow::Owned(format!("{:?}", entry.path())), }; - format!( + Ok(format!( " File: {}{}\n \ Size: {:<13} Type: {}\n\ Access: ({:o}/{}) Uid: {:<5} Gid: {:<5}\n\ @@ -199,5 +201,5 @@ pub fn format_multi_line_entry(entry: &Entry) -> String { meta.stat.uid, meta.stat.gid, format_mtime(&meta.stat.mtime), - ) + )) } diff --git a/pxar-bin/src/main.rs b/pxar-bin/src/main.rs index 90887321..9b7de8f4 100644 --- a/pxar-bin/src/main.rs +++ b/pxar-bin/src/main.rs @@ -13,6 +13,7 @@ use tokio::signal::unix::{signal, SignalKind}; use pathpatterns::{MatchEntry, MatchType, PatternFlag}; use pbs_client::pxar::{format_single_line_entry, Flags, PxarExtractOptions, ENCODER_MAX_ENTRIES}; +use pxar::EntryKind; use proxmox_router::cli::*; use proxmox_schema::api; @@ -409,9 +410,12 @@ async fn mount_archive(archive: String, mountpoint: String, verbose: bool) -> Re fn dump_archive(archive: String) -> Result<(), Error> { for entry in pxar::decoder::Decoder::open(archive)? { let entry = entry?; + if let EntryKind::Version(_) = entry.kind() { + bail!("got format version incompatible with this client"); + } if log::log_enabled!(log::Level::Debug) { - log::debug!("{}", format_single_line_entry(&entry)); + log::debug!("{}", format_single_line_entry(&entry)?); } else { log::info!("{:?}", entry.path()); } -- 2.30.2 _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-06-06 9:05 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-06-05 15:41 [pbs-devel] [PATCH v2 stable-2 pxar proxmox-backup 0/2] backport pxar format version check Christian Ebner 2024-06-05 15:41 ` [pbs-devel] [PATCH v2 stable-2 pxar 1/1] format/decoder/accessor: backport pxar entry type `Version` Christian Ebner 2024-06-06 8:21 ` Fabian Grünbichler 2024-06-06 8:49 ` Christian Ebner 2024-06-06 9:05 ` Fabian Grünbichler 2024-06-05 15:41 ` [pbs-devel] [PATCH v2 stable-2 proxmox-backup 2/2] client: pxar: bail on incompatible format versions Christian Ebner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox