* [PATCH v2 proxmox-backup 2/3] datastore: check that .i.bad files from S3 actually match that pattern
2026-07-23 9:54 [PATCH v2 proxmox-backup 0/3] improve handling of bad S3 chunks during GC Robert Obkircher
2026-07-23 9:54 ` [PATCH v2 proxmox-backup 1/3] datastore: fix local path for .i.bad chunk files during S3 GC Robert Obkircher
@ 2026-07-23 9:54 ` Robert Obkircher
2026-07-23 9:54 ` [PATCH v2 proxmox-backup 3/3] datastore: introduce helper function to parse digest from S3ObjectKey Robert Obkircher
2 siblings, 0 replies; 4+ messages in thread
From: Robert Obkircher @ 2026-07-23 9:54 UTC (permalink / raw)
To: pbs-devel
Compare the characters, not just the length, and introduce a helper
that can be tested in isolation.
Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
Reviewed-by: Christian Ebner <c.ebner@proxmox.com>
---
pbs-datastore/src/chunk_store.rs | 21 ++++++++++++++-------
pbs-datastore/src/datastore.rs | 7 +++----
2 files changed, 17 insertions(+), 11 deletions(-)
diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index a936f5034..7945ed856 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -360,13 +360,7 @@ impl ChunkStore {
}
// i-th bad chunk
- if bytes.len() == 64 + ".i.bad".len()
- && bytes[64] == b'.'
- && bytes[65] >= b'0'
- && bytes[65] <= b'9'
- && bytes[66] == b'.'
- && bytes.ends_with(b"bad")
- {
+ if is_bad_chunk_suffix(&bytes[64..]) {
return Some((Ok(entry), percentage, ChunkExt::Bad));
}
@@ -1028,6 +1022,19 @@ impl ChunkExt {
}
}
+pub(crate) fn is_bad_chunk_suffix(s: &[u8]) -> bool {
+ s.len() == ".i.bad".len() && s[0] == b'.' && s[1].is_ascii_digit() && &s[2..] == b".bad"
+}
+
+#[test]
+fn test_is_bad_chunk_suffix() {
+ assert!(is_bad_chunk_suffix(".0.bad".as_bytes()));
+ assert!(is_bad_chunk_suffix(".9.bad".as_bytes()));
+ for s in [".10.bad", ".a.bad", ".bad", "0.bad", ""] {
+ assert!(!is_bad_chunk_suffix(s.as_bytes()));
+ }
+}
+
#[test]
fn test_chunk_store1() {
use tempfile::TempDir;
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 3079fe833..bac5ce3b4 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -43,7 +43,7 @@ use proxmox_section_config::SectionConfigData;
use crate::backup_info::{
BackupDir, BackupGroup, BackupInfo, OLD_LOCKING, PROTECTED_MARKER_FILENAME,
};
-use crate::chunk_store::ChunkStore;
+use crate::chunk_store::{ChunkStore, is_bad_chunk_suffix};
use crate::dynamic_index::{DynamicIndexReader, DynamicIndexWriter};
use crate::fixed_index::{FixedIndexReader, FixedIndexWriter};
use crate::hierarchy::{ListGroups, ListGroupsType, ListNamespaces, ListNamespacesRecursive};
@@ -2680,13 +2680,12 @@ impl DataStore {
// file_name() should always be Some, as objects will have a filename
let file_name = path.file_name()?;
let bytes = file_name.as_bytes();
- let bad_ext_len = ".0.bad".len();
- let bad_chunk = if bytes.len() == 64 + bad_ext_len {
+ let bad_chunk = if is_bad_chunk_suffix(&bytes[64..]) {
true
} else if bytes.len() == 64 {
false
} else {
- return None;
+ return None; // unexpected suffix
};
if !bytes.iter().take(64).all(u8::is_ascii_hexdigit) {
return None;
--
2.47.3
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH v2 proxmox-backup 3/3] datastore: introduce helper function to parse digest from S3ObjectKey
2026-07-23 9:54 [PATCH v2 proxmox-backup 0/3] improve handling of bad S3 chunks during GC Robert Obkircher
2026-07-23 9:54 ` [PATCH v2 proxmox-backup 1/3] datastore: fix local path for .i.bad chunk files during S3 GC Robert Obkircher
2026-07-23 9:54 ` [PATCH v2 proxmox-backup 2/3] datastore: check that .i.bad files from S3 actually match that pattern Robert Obkircher
@ 2026-07-23 9:54 ` Robert Obkircher
2 siblings, 0 replies; 4+ messages in thread
From: Robert Obkircher @ 2026-07-23 9:54 UTC (permalink / raw)
To: pbs-devel
Do not rely on the platform-specific `Path::file_name` because S3
object keys are flat identifiers that always use `/` by convention.
It would, for example, be incorrect to ignore trailing slashes (or to
treat backslashes differently on Windows).
Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
Reviewed-by: Christian Ebner <c.ebner@proxmox.com>
---
pbs-datastore/src/datastore.rs | 28 ++++++------------------
pbs-datastore/src/s3.rs | 39 +++++++++++++++++++++++++++++++---
2 files changed, 43 insertions(+), 24 deletions(-)
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index bac5ce3b4..d106f5b52 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -48,7 +48,7 @@ use crate::dynamic_index::{DynamicIndexReader, DynamicIndexWriter};
use crate::fixed_index::{FixedIndexReader, FixedIndexWriter};
use crate::hierarchy::{ListGroups, ListGroupsType, ListNamespaces, ListNamespacesRecursive};
use crate::index::IndexFile;
-use crate::s3::S3_CONTENT_PREFIX;
+use crate::s3::{S3_CONTENT_PREFIX, digest_from_object_key};
use crate::task_tracking::{self, update_active_operations};
use crate::{DataBlob, LocalDatastoreLruCache};
@@ -2675,36 +2675,22 @@ impl DataStore {
&self,
object_key: &S3ObjectKey,
) -> Option<(PathBuf, [u8; 32], bool)> {
- // Check object is actually a chunk
- let path = Path::new::<str>(object_key);
- // file_name() should always be Some, as objects will have a filename
- let file_name = path.file_name()?;
- let bytes = file_name.as_bytes();
- let bad_chunk = if is_bad_chunk_suffix(&bytes[64..]) {
+ let (file_name, digest, suffix) = digest_from_object_key(object_key)?;
+
+ let bad_chunk = if is_bad_chunk_suffix(suffix.as_bytes()) {
true
- } else if bytes.len() == 64 {
+ } else if suffix.is_empty() {
false
} else {
return None; // unexpected suffix
};
- if !bytes.iter().take(64).all(u8::is_ascii_hexdigit) {
- return None;
- }
- // Safe since contains valid ascii hexdigits only as checked above.
- let digest_str = file_name.to_string_lossy();
- let hexdigit_prefix = unsafe { digest_str.get_unchecked(0..4) };
let mut chunk_path = self.base_path();
chunk_path.push(".chunks");
- chunk_path.push(hexdigit_prefix);
+ chunk_path.push(&file_name[..4]);
chunk_path.push(file_name);
- let mut digest_bytes = [0u8; 32];
- let digest = file_name.as_bytes();
- // safe to unwrap as already checked above
- hex::decode_to_slice(&digest[..64], &mut digest_bytes).unwrap();
-
- Some((chunk_path, digest_bytes, bad_chunk))
+ Some((chunk_path, digest, bad_chunk))
}
pub fn try_shared_chunk_store_lock(&self) -> Result<ProcessLockSharedGuard, Error> {
diff --git a/pbs-datastore/src/s3.rs b/pbs-datastore/src/s3.rs
index 09affc2e5..5b922f3a0 100644
--- a/pbs-datastore/src/s3.rs
+++ b/pbs-datastore/src/s3.rs
@@ -1,6 +1,7 @@
use std::path::{Path, PathBuf};
use anyhow::{Error, bail, format_err};
+use hex::FromHex;
use proxmox_s3_client::{DeleteObjectError, S3ObjectKey};
@@ -48,6 +49,16 @@ pub fn object_key_from_digest_with_suffix(
S3ObjectKey::try_from(object_key_string.as_str())
}
+/// Extract filename, digest, and suffix from the last part of an S3ObjectKey.
+pub(crate) fn digest_from_object_key(key: &S3ObjectKey) -> Option<(&str, [u8; 32], &str)> {
+ let filename = key.rsplit('/').next()?;
+ let (hex_digest, suffix) = filename.split_at_checked(64)?;
+
+ let digest = FromHex::from_hex(hex_digest).ok()?;
+
+ Some((filename, digest, suffix))
+}
+
/// Log errors from delete objects api calls
pub(crate) fn log_s3_delete_objects_errors(errors: &[DeleteObjectError]) {
for error in errors {
@@ -96,7 +107,6 @@ fn test_object_key_from_path_incorrect_filename() {
#[test]
fn test_object_key_from_digest() {
- use hex::FromHex;
let digest =
<[u8; 32]>::from_hex("bb9f8df61474d25e71fa00722318cd387396ca1736605e1248821cc0de3d3af8")
.unwrap();
@@ -108,7 +118,6 @@ fn test_object_key_from_digest() {
#[test]
fn test_object_key_from_digest_with_suffix() {
- use hex::FromHex;
let digest =
<[u8; 32]>::from_hex("bb9f8df61474d25e71fa00722318cd387396ca1736605e1248821cc0de3d3af8")
.unwrap();
@@ -122,9 +131,33 @@ fn test_object_key_from_digest_with_suffix() {
#[test]
fn test_object_key_from_digest_with_invalid_suffix() {
- use hex::FromHex;
let digest =
<[u8; 32]>::from_hex("bb9f8df61474d25e71fa00722318cd387396ca1736605e1248821cc0de3d3af8")
.unwrap();
assert!(object_key_from_digest_with_suffix(&digest, "/.0.bad").is_err());
}
+
+#[test]
+fn test_digest_from_object_key() {
+ let hex = "bb9f8df61474d25e71fa00722318cd387396ca1736605e1248821cc0de3d3af8";
+ let digest = <[u8; 32]>::from_hex(hex).unwrap();
+ let k = |s: &str| S3ObjectKey::try_from(s).unwrap();
+
+ assert_eq!(digest_from_object_key(&k(&hex[1..])), None);
+ let invalid_hex = hex.replace("b", "X");
+ assert_eq!(digest_from_object_key(&k(&invalid_hex)), None);
+
+ assert_eq!(digest_from_object_key(&k(hex)), Some((hex, digest, "")));
+
+ let k1 = object_key_from_digest_with_suffix(&digest, "suffix1").unwrap();
+ assert_eq!(
+ digest_from_object_key(&k1),
+ Some((format!("{hex}suffix1").as_str(), digest, "suffix1"))
+ );
+
+ let k2 = S3ObjectKey::try_from(format!(".chunks/bb9f/{hex}.0.bad").as_str()).unwrap();
+ assert_eq!(
+ digest_from_object_key(&k2),
+ Some((format!("{hex}.0.bad").as_str(), digest, ".0.bad"))
+ );
+}
--
2.47.3
^ permalink raw reply related [flat|nested] 4+ messages in thread