From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id C79FA1FF396 for ; Thu, 6 Jun 2024 10:21:48 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id ECF91968A; Thu, 6 Jun 2024 10:22:19 +0200 (CEST) Date: Thu, 06 Jun 2024 10:21:42 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox Backup Server development discussion References: <20240605154155.2365-1-c.ebner@proxmox.com> <20240605154155.2365-2-c.ebner@proxmox.com> In-Reply-To: <20240605154155.2365-2-c.ebner@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1717661517.z9ivsk6e9o.astroid@yuna.none> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.058 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pbs-devel] [PATCH v2 stable-2 pxar 1/1] format/decoder/accessor: backport pxar entry type `Version` X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" 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 > --- > 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 { > } > > impl AccessorImpl { > - pub async fn new(input: T, size: u64) -> io::Result { > + pub async fn new(mut input: T, size: u64) -> io::Result { > if size < (size_of::() 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::() 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 AccessorImpl { > .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 DirectoryImpl { > .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 { > eof_after_entry: bool, > } > > +#[derive(Clone, PartialEq)] > enum State { > Begin, > Default, > @@ -236,7 +237,16 @@ impl DecoderImpl { > 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 DecoderImpl { > } > > async fn read_next_entry_or_eof(&mut self) -> io::Result> { > + let previous_state = self.state.clone(); > self.state = State::Default; > self.entry.clear_data(); > > @@ -373,6 +384,14 @@ impl DecoderImpl { > 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 DecoderImpl { > async fn read_quota_project_id(&mut self) -> io::Result { > self.read_simple_entry("quota project id").await > } > + > + async fn read_format_version(&mut self) -> io::Result { > + 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 { > + 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, > 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