From 31de127cf42438b1fac04c161c47264aa1fb1060 Mon Sep 17 00:00:00 2001 From: David Steele Date: Tue, 19 Sep 2023 11:30:29 -0400 Subject: [PATCH] Fix issue restoring block incremental without a block list. It is currently possible for a block map to be written without any accompanying blocks. This happens when a file timestamp is updated but the file has not changed. On restore, this caused problems when encryption was enabled, the block map was bundled after a file that had been stored without block incremental, and both files were included in a single bundle read. In this case the block map would not be decrypted and the encrypted data was passed to blockMapNewRead() with unpredictable results. In many cases built-in retries would rectify this problem as long as delta was enabled since block maps would move to the beginning of the bundle read and be decrypted properly. If enough files were affected, however, it could overwhelm the retries and throw an error. Subsequent delta restores would eventually be able to produce a valid result. Fix this by moving block map decryption so it works correctly no matter where the block map is located in the read. This has the additional benefit of limiting how far the block map can read so it will error earlier if corrupt. Though in this case there was no repository corruption involved, it appeared that way to blockMapNewRead() since it was reading encrypted data. Arguably block maps without blocks should not be written at all, but it would be better to consider that as a separate change. This pattern clearly exists in the wild and needs to be handled, plus the new implementation has other benefits. --- doc/xml/release/2023/2.48.xml | 17 ++++++++++++++ doc/xml/release/contributor.xml | 5 +++++ src/command/restore/file.c | 23 ++++++++++--------- test/src/module/command/restoreTest.c | 32 +++++++++++++++++++++++++-- 4 files changed, 65 insertions(+), 12 deletions(-) diff --git a/doc/xml/release/2023/2.48.xml b/doc/xml/release/2023/2.48.xml index 96a11aed6..5bd6051ad 100644 --- a/doc/xml/release/2023/2.48.xml +++ b/doc/xml/release/2023/2.48.xml @@ -1,5 +1,22 @@ + + + + + + + + + + + + + +

Fix issue restoring block incremental without a block list.

+ + + diff --git a/doc/xml/release/contributor.xml b/doc/xml/release/contributor.xml index 817c5de39..037016d4b 100644 --- a/doc/xml/release/contributor.xml +++ b/doc/xml/release/contributor.xml @@ -940,6 +940,11 @@ seadba + + Burak Yurdakul + threenotrump + + ucando ucando diff --git a/src/command/restore/file.c b/src/command/restore/file.c index b748bc924..70652e7c2 100644 --- a/src/command/restore/file.c +++ b/src/command/restore/file.c @@ -19,6 +19,7 @@ Restore File #include "common/io/filter/group.h" #include "common/io/filter/size.h" #include "common/io/io.h" +#include "common/io/limitRead.h" #include "common/log.h" #include "config/config.h" #include "info/manifest.h" @@ -265,15 +266,6 @@ restoreFile( .compressible = repoFileCompressType == compressTypeNone && cipherPass == NULL, .offset = file->offset, .limit = repoFileLimit != 0 ? VARUINT64(repoFileLimit) : NULL); - // Add decryption filter for block incremental map - if (cipherPass != NULL && file->blockIncrMapSize != 0) - { - ioFilterGroupAdd( - ioReadFilterGroup(storageReadIo(repoFileRead)), - cipherBlockNewP( - cipherModeDecrypt, cipherTypeAes256Cbc, BUFSTR(cipherPass), .raw = true)); - } - ioReadOpen(storageReadIo(repoFileRead)); } MEM_CONTEXT_PRIOR_END(); @@ -294,8 +286,19 @@ restoreFile( // Read block map. This will be compared to the block checksum list already created to determine which // blocks need to be fetched from the repository. If we got here there must be at least one block to fetch. + IoRead *const blockMapRead = ioLimitReadNew(storageReadIo(repoFileRead), varUInt64(file->limit)); + + if (cipherPass != NULL) + { + ioFilterGroupAdd( + ioReadFilterGroup(blockMapRead), + cipherBlockNewP(cipherModeDecrypt, cipherTypeAes256Cbc, BUFSTR(cipherPass), .raw = true)); + } + + ioReadOpen(blockMapRead); + const BlockMap *const blockMap = blockMapNewRead( - storageReadIo(repoFileRead), file->blockIncrSize, file->blockIncrChecksumSize); + blockMapRead, file->blockIncrSize, file->blockIncrChecksumSize); // The repo file needs to be closed so that block lists can be read from the remote protocol ioReadClose(storageReadIo(repoFileRead)); diff --git a/test/src/module/command/restoreTest.c b/test/src/module/command/restoreTest.c index 473e216df..416d729dd 100644 --- a/test/src/module/command/restoreTest.c +++ b/test/src/module/command/restoreTest.c @@ -3101,14 +3101,15 @@ testRun(void) TEST_TITLE("full backup with block incr"); // Zeroed file large enough to use block incr + time_t timeBase = time(NULL); Buffer *relation = bufNew(256 * 1024); memset(bufPtr(relation), 0, bufSize(relation)); bufUsedSet(relation, bufSize(relation)); - HRN_STORAGE_PUT(storagePgWrite(), PG_PATH_BASE "/1/2", relation); + HRN_STORAGE_PUT(storagePgWrite(), PG_PATH_BASE "/1/2", relation, .timeModified = timeBase - 2); // Add postgresql.auto.conf to contain recovery settings - HRN_STORAGE_PUT_EMPTY(storagePgWrite(), PG_FILE_POSTGRESQLAUTOCONF); + HRN_STORAGE_PUT_EMPTY(storagePgWrite(), PG_FILE_POSTGRESQLAUTOCONF, .timeModified = timeBase - 1); // Backup argList = strLstNew(); @@ -3126,6 +3127,32 @@ testRun(void) TEST_RESULT_VOID(hrnCmdBackup(), "backup"); + // ------------------------------------------------------------------------------------------------------------------------- + TEST_TITLE("diff backup with block incr"); + + // Update timestamps so base/1/3 is slightly older than base/1/2. This will cause base/1/2 to be placed after base/1/3 in + // the bundle and since base/1/2 did not change it will be stored as a map only, which will cause restore to include it in a + // single read with base/1/3 (which is too small to be block incremental). This is to test for regression in the fix to + // separately decrypt block maps when they are in the middle/end of a bundle read. + HRN_STORAGE_TIME(storagePgWrite(), PG_PATH_BASE "/1/2", timeBase); + HRN_STORAGE_PUT_Z(storagePgWrite(), PG_PATH_BASE "/1/3", "contents", .timeModified = timeBase - 1); + + // Backup + argList = strLstNew(); + hrnCfgArgRawZ(argList, cfgOptStanza, "test1"); + hrnCfgArgRaw(argList, cfgOptRepoPath, repoPath); + hrnCfgArgRaw(argList, cfgOptPgPath, pgPath); + hrnCfgArgRawZ(argList, cfgOptRepoRetentionFull, "1"); + hrnCfgArgRawStrId(argList, cfgOptType, backupTypeDiff); + hrnCfgArgRawBool(argList, cfgOptRepoBundle, true); + hrnCfgArgRawBool(argList, cfgOptRepoBlock, true); + hrnCfgArgRawBool(argList, cfgOptOnline, false); + hrnCfgArgRawZ(argList, cfgOptRepoCipherType, "aes-256-cbc"); + hrnCfgEnvRawZ(cfgOptRepoCipherPass, TEST_CIPHER_PASS); + HRN_CFG_LOAD(cfgCmdBackup, argList); + + TEST_RESULT_VOID(hrnCmdBackup(), "backup"); + // ------------------------------------------------------------------------------------------------------------------------- TEST_TITLE("restore with block incr"); @@ -3148,6 +3175,7 @@ testRun(void) "base/\n" "base/1/\n" "base/1/2\n" + "base/1/3\n" "global/\n" "global/pg_control\n" "postgresql.auto.conf\n",