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

[Robert Obkircher <r.obkircher@proxmox.com>]

On 07.08.26 14:43, Christian Ebner wrote:
> 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? 

It is not necessarily the case that one chunk must be corrupt here. It
could also be an attack from a malicious client that learned about an
existing digest and is trying to override it. So, especially if both
CRCs were valid, we must keep the older one.

I do agree that decoding to find out more details makes sense, because
this should be extremely rare in practice.

But this is also the critical section of the most heavily contended
lock, so removing the magic read entirely might be worth it. Fabian
seemed convinced that this would be safe.


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

I think verification alone wouldn't be sufficient for unreferenced chunks.

The user on the forum post mentioned being unable to verify the backup
as it has been deleted via the gui.


>
>>                       )
>>                   }
>>   
>