[PATCH v1 proxmox-backup 0/3] improve handling of bad S3 chunks during GC

[PATCH v1 proxmox-backup 0/3] improve handling of bad S3 chunks during GC

[Robert Obkircher]

Fix an issue where the local path of .i.bad files was computed
incorrectly, and refactor the related code a bit.

I discovered this issue while grepping for "unsafe" to see if it would
make sense to (explicitly) depend on the zerocopy crate for the index
file readers. The removed unsafe statement in the first patch looked
suspicious because it could have been out-of-bounds if to_string_lossy
wasn't guaranteed to be called on an utf8 OsStr.

Robert Obkircher (3):
  datastore: fix local path for .i.bad chunk files during S3 GC
  datastore: check that .i.bad files from S3 actually match that pattern
  datastore: introduce helper function to parse digest from S3ObjectKey

 pbs-datastore/src/chunk_store.rs | 21 +++++++++++------
 pbs-datastore/src/datastore.rs   | 39 ++++++++------------------------
 pbs-datastore/src/s3.rs          | 39 +++++++++++++++++++++++++++++---
 3 files changed, 60 insertions(+), 39 deletions(-)

-- 
2.47.3

[PATCH v1 proxmox-backup 1/3] datastore: fix local path for .i.bad chunk files during S3 GC

[Robert Obkircher]

Do not push an additional path segment with the extension, and rename
the confusing variable. The additional segment caused a rather cryptic
"TASK ERROR: Not a directory (os error 20)" error during Phase 2 if
the .i.bad file existed locally.

If it doesn't exist locally, the S3 object was and continues to be
deleted, unless it is the very first one and the corresponding chunk
expected marker exists. This behavior might not be desirable if the
files are missing because the datastore was recreated with the "reuse
existing datastore" option.

Fixes: 27e3f5b7b ("GC: fix: don't drop bad extension for S3 object to chunk path helper")
Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/datastore.rs | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index e2d1ae67c..3079fe833 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -2678,8 +2678,8 @@ impl DataStore {
         // 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 digest = path.file_name()?;
-        let bytes = digest.as_bytes();
+        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 {
             true
@@ -2693,19 +2693,15 @@ impl DataStore {
         }
 
         // Safe since contains valid ascii hexdigits only as checked above.
-        let digest_str = digest.to_string_lossy();
+        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(digest);
-        if bad_chunk {
-            let extension = unsafe { digest_str.get_unchecked(64..64 + bad_ext_len) };
-            chunk_path.push(extension);
-        }
+        chunk_path.push(file_name);
 
         let mut digest_bytes = [0u8; 32];
-        let digest = digest.as_bytes();
+        let digest = file_name.as_bytes();
         // safe to unwrap as already checked above
         hex::decode_to_slice(&digest[..64], &mut digest_bytes).unwrap();
 
-- 
2.47.3

[PATCH v1 proxmox-backup 2/3] datastore: check that .i.bad files from S3 actually match that pattern

[Robert Obkircher]

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>
---
 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..f40c58f7e 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 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

[PATCH v1 proxmox-backup 3/3] datastore: introduce helper function to parse digest from S3ObjectKey

[Robert Obkircher]

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>
---
 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..c4209d60a 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 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