1
0
mirror of https://github.com/pgbackrest/pgbackrest.git synced 2024-12-14 10:13:05 +02:00

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.
This commit is contained in:
David Steele 2023-09-19 11:30:29 -04:00 committed by GitHub
parent 88edea4571
commit 31de127cf4
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 65 additions and 12 deletions
doc/xml/release
src/command/restore
test/src/module/command

View File

@ -1,5 +1,22 @@
<release date="XXXX-XX-XX" version="2.48dev" title="Under Development">
<release-core-list>
<release-bug-list>
<release-item>
<github-issue id="2182"/>
<github-pull-request id="2184"/>
<release-item-contributor-list>
<release-item-ideator id="burak.yurdakul"/>
<release-item-contributor id="david.steele"/>
<release-item-reviewer id="stephen.frost"/>
<!-- Actually tester, but we don't have a tag for that yet -->
<release-item-reviewer id="burak.yurdakul"/>
</release-item-contributor-list>
<p>Fix issue restoring block incremental without a block list.</p>
</release-item>
</release-bug-list>
<release-feature-list>
<release-item>
<github-issue id="2150"/>

View File

@ -940,6 +940,11 @@
<contributor-id type="github">seadba</contributor-id>
</contributor>
<contributor id="burak.yurdakul">
<contributor-name-display>Burak Yurdakul</contributor-name-display>
<contributor-id type="github">threenotrump</contributor-id>
</contributor>
<contributor id="ucando">
<contributor-name-display>ucando</contributor-name-display>
<contributor-id type="github">ucando</contributor-id>

View File

@ -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));

View File

@ -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",