[PATCH v2 proxmox-backup 0/2] minor chunk insert improvements

[PATCH v2 proxmox-backup 0/2] minor chunk insert improvements

[Robert Obkircher]

Improve error messages and avoid custom unsafe code.

Changes since [v1]:
* improve error messages as suggested by @Chris
  * mention "magic number"
  * reprase to "verify and garbage-collect" 

[v1] https://lore.proxmox.com/pbs-devel/20260807095803.3051-1-r.obkircher@proxmox.com/

Robert Obkircher (2):
  datastore: prefer standard library over custom unsafe code
  datastore: rephrase error messages for failed chunk insert

 pbs-datastore/src/chunk_store.rs | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

-- 
2.47.3

[PATCH v2 proxmox-backup 1/2] datastore: prefer standard library over custom unsafe code

[Robert Obkircher]

The allocation is unnecessary overhead, and the unsafe code in
read_exact_allocated looks a bit dangerous.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/chunk_store.rs | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index 6d2ffdbde..e5cfd130f 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -1,3 +1,4 @@
+use std::io::Read;
 use std::os::unix::fs::MetadataExt;
 use std::os::unix::io::AsRawFd;
 use std::path::{Path, PathBuf};
@@ -10,7 +11,6 @@ use tracing::{info, warn};
 
 use pbs_api_types::{DatastoreFSyncLevel, GarbageCollectionStatus};
 use pbs_config::BackupLockGuard;
-use proxmox_io::ReadExt;
 use proxmox_s3_client::S3Client;
 use proxmox_sys::fs::{CreateOptions, create_dir, create_path, file_type_from_file_stat};
 use proxmox_sys::process_locker::{
@@ -708,8 +708,11 @@ impl ChunkStore {
                 }
             } else if chunk.is_encrypted() {
                 // incoming chunk is encrypted, possible attack or hash collision!
-                let mut existing_file = std::fs::File::open(&chunk_path)?;
-                let magic = existing_file.read_exact_allocated(8)?;
+
+                let mut magic = [0u8; 8];
+                std::fs::File::open(&chunk_path)
+                    .and_then(|mut f| f.read_exact(&mut magic))
+                    .map_err(|e| format_err!("Failed to read magic number from header of existing chunk '{digest_str}' on store '{name}: {e}"))?;
 
                 // going from unencrypted to encrypted can never be right, since the digest
                 // includes data derived from the encryption key
-- 
2.47.3

[PATCH v2 proxmox-backup 2/2] datastore: rephrase error messages for failed chunk insert

[Robert Obkircher]

The "not allowed" phrasing sounds like an access-right problem, when
the actual problem is likely file corruption.

Link: https://forum.proxmox.com/threads/184140
Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/chunk_store.rs | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index e5cfd130f..a3cc47d54 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -718,15 +718,16 @@ impl ChunkStore {
                 // includes data derived from the encryption key
                 if magic == UNCOMPRESSED_BLOB_MAGIC_1_0 || magic == COMPRESSED_BLOB_MAGIC_1_0 {
                     bail!(
-                        "Overwriting unencrypted chunk '{digest_str}' on store '{name}' with encrypted chunk with same digest not allowed!"
+                        "Cannot overwrite existing unencrypted chunk '{digest_str}' on store '{name}' with encrypted chunk."
                     );
                 }
 
                 // if both chunks are uncompressed and encrypted and have the same digest, but
                 // their sizes are different, one of them *must* be invalid
                 if magic == ENCRYPTED_BLOB_MAGIC_1_0 && !chunk.is_compressed() {
+                    // gc is also mentioned because verification doesn't check unreferenced chunks
                     bail!(
-                        "Overwriting existing (encrypted) chunk '{digest_str}' on store '{name}' is not allowed!"
+                        "Found existing encrypted chunk '{digest_str}' of different size on store '{name}'. Verify and garbage-collect the full datastore to clean up possibly corrupted chunks.",
                     )
                 }
 
-- 
2.47.3