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 A9C6B1FF183 for ; Wed, 27 Aug 2025 11:47:17 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 08A2E16378; Wed, 27 Aug 2025 11:47:19 +0200 (CEST) From: Christian Ebner To: pbs-devel@lists.proxmox.com Date: Wed, 27 Aug 2025 11:46:33 +0200 Message-ID: <20250827094633.239975-4-c.ebner@proxmox.com> X-Mailer: git-send-email 2.47.2 In-Reply-To: <20250827094633.239975-1-c.ebner@proxmox.com> References: <20250827094633.239975-1-c.ebner@proxmox.com> MIME-Version: 1.0 X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1756287999452 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.043 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 Subject: [pbs-devel] [PATCH pxar 3/3] encoder: improve consistency check for payload reference offsets 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" The pxar encoder for split metadata and payload archives cannot keep exactly track of the payload stream and payload reference offset to be in sync since it must allow for injecting payloads steming from reused chunks in PBS. Therefore, currently it is only checked that the offset to encode in the payload reference is greater than the current payload writer position (the chunk data is always injected after advancing the metadata archive). This check is however not good enough to protect against encoding an archive with not strict monotonically increasing payload reference offset, which cannot be restored by the sequential decoder. Guard against this by keeping track of the previously encoded payload offset and check for the new one to be always greater. Move the pre-existing check together with the stricter check into a common helper and us it also for checking the offsets when creating files being encoded with split pxar archives, not just when encoding a reused file in the metadata archive as pxar payload reference. This will avoid the encoding of corrupt archives as reported in the community forum [0]. [0] https://forum.proxmox.com/threads/170211/ Signed-off-by: Christian Ebner --- src/encoder/mod.rs | 66 +++++++++++++++++++++++++++++++++++----------- 1 file changed, 50 insertions(+), 16 deletions(-) diff --git a/src/encoder/mod.rs b/src/encoder/mod.rs index 4e7daef..131cd69 100644 --- a/src/encoder/mod.rs +++ b/src/encoder/mod.rs @@ -243,6 +243,9 @@ struct EncoderState { /// Track the bytes written to the payload writer payload_write_position: u64, + /// Previously generated payload offset + previous_payload_offset: Option, + /// Mark the encoder state as correctly finished, ready to be dropped finished: bool, } @@ -258,6 +261,46 @@ impl EncoderState { self.payload_write_position } + #[inline] + fn previous_payload_offset(&self) -> Option { + self.previous_payload_offset + } + + /// Generate a PayloadRef from the given payload offset and file size. + /// + /// Checks if the provided payload offset is greater than the current payload writeer position + /// and larger than the previously checked payload offset. Both are required for the sequential + /// decoder to be able to restore contents. + fn payload_ref_from( + &mut self, + payload_offset: PayloadOffset, + size: u64, + ) -> io::Result { + let payload_position = self.payload_position(); + if payload_offset.raw() < payload_position { + io_bail!( + "offset smaller than current position: {} < {payload_position}", + payload_offset.raw() + ); + } + + if let Some(previous_payload_offset) = self.previous_payload_offset() { + if payload_offset <= previous_payload_offset { + io_bail!( + "unexpected payload offset {} not larger than previous offset {}", + payload_offset.raw(), + previous_payload_offset.raw(), + ); + } + } + self.previous_payload_offset = Some(payload_offset); + + Ok(PayloadRef { + offset: payload_offset.raw(), + size, + }) + } + fn merge_error(&mut self, error: Option) { // one error is enough: if self.encode_error.is_none() { @@ -454,17 +497,13 @@ impl<'a, T: SeqWrite + 'a> EncoderImpl<'a, T> { if let Some(payload_output) = output.payload_mut() { // payload references must point to the position prior to the payload header, // separating payload entries in the payload stream - let payload_position = state.payload_position(); + let payload_position = PayloadOffset(state.payload_position()); + let payload_ref = state.payload_ref_from(payload_position, file_size)?; let header = format::Header::with_content_size(format::PXAR_PAYLOAD, file_size); header.check_header_size()?; seq_write_struct(payload_output, header, &mut state.payload_write_position).await?; - let payload_ref = PayloadRef { - offset: payload_position, - size: file_size, - }; - seq_write_pxar_entry( output.archive_mut(), format::PXAR_PAYLOAD_REF, @@ -566,16 +605,9 @@ impl<'a, T: SeqWrite + 'a> EncoderImpl<'a, T> { io_bail!("unable to add payload reference"); } - let offset = payload_offset.raw(); - let payload_position = self.state()?.payload_position(); - if offset < payload_position { - io_bail!("offset smaller than current position: {offset} < {payload_position}"); - } - - let payload_ref = PayloadRef { - offset, - size: file_size, - }; + let payload_ref = self + .state_mut()? + .payload_ref_from(payload_offset, file_size)?; let this_offset: LinkOffset = self .add_file_entry( Some(metadata), @@ -749,6 +781,7 @@ impl<'a, T: SeqWrite + 'a> EncoderImpl<'a, T> { // the child will write to OUR state now: let write_position = state.position(); let payload_write_position = state.payload_position(); + let previous_payload_offset = state.previous_payload_offset(); self.state.push(EncoderState { items: Vec::new(), @@ -759,6 +792,7 @@ impl<'a, T: SeqWrite + 'a> EncoderImpl<'a, T> { file_hash, write_position, payload_write_position, + previous_payload_offset, finished: false, }); -- 2.47.2 _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel