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

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

[Robert Obkircher]

Improve error messages and avoid custom unsafe code.

I forgot to send these patches from about 2 months ago. At the time
Fabian suggested that we could potentially remove the read entirely and
simply ignore these collisions.

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 | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

-- 
2.47.3

[PATCH v1 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..3f637730c 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 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 v1 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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index 3f637730c..d9d0c9a26 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -718,7 +718,7 @@ 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."
                     );
                 }
 
@@ -726,7 +726,7 @@ impl ChunkStore {
                 // their sizes are different, one of them *must* be invalid
                 if magic == ENCRYPTED_BLOB_MAGIC_1_0 && !chunk.is_compressed() {
                     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}'. Consider running verification and garbage-collection because it may be corrupted.",
                     )
                 }
 
-- 
2.47.3

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

[Christian Ebner]

On 8/7/26 11:58 AM, Robert Obkircher wrote:
> 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..3f637730c 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 header of existing chunk '{digest_str}' on store '{name}: {e}"))?;

nit: this does not read the chunk header 
(DataBlobHeader/EncryptedDataBlobHeader), just the magic number. So 
maybe better to also refer to it in the error message as such.

>   
>                   // going from unencrypted to encrypted can never be right, since the digest
>                   // includes data derived from the encryption key

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

[Christian Ebner]

On 8/7/26 11:58 AM, Robert Obkircher wrote:
> 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 | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
> index 3f637730c..d9d0c9a26 100644
> --- a/pbs-datastore/src/chunk_store.rs
> +++ b/pbs-datastore/src/chunk_store.rs
> @@ -718,7 +718,7 @@ 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."
>                       );
>                   }
>   
> @@ -726,7 +726,7 @@ impl ChunkStore {
>                   // their sizes are different, one of them *must* be invalid
>                   if magic == ENCRYPTED_BLOB_MAGIC_1_0 && !chunk.is_compressed() {
>                       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}'. Consider running verification and garbage-collection because it may be corrupted.",

question: pre-existing but since one of the chunks must be the corrupt 
one here, and the server checks the CRC sum on upload, the new chunk is 
most likely to be the fine one. Might be worth to decode the full 
on-disk chunk then and double check its CRC? An only bail if both are 
fine, which is very unlikely?

anyways, I suggest to modify the error message to be a bit more concise:

"Unexpected encrypted chunk {} with different size already present on 
store {}. Run verification to detect possibly corrupt chunks."

>                       )
>                   }
>