db/pgbackrest
1
0
Fork 0
mirror of https://github.com/pgbackrest/pgbackrest.git synced 2025-09-16 09:06:18 +02:00
Code Issues Releases Activity

Fix protocol error on short read of remote file.

Browse Source 
If a remote file read was stopped before the read was complete or if an error occurred in the middle of the read then the protocol would end up in a bad state and produce this error:

ProtocolError: client state is 'data-get' but expected 'idle'

Prevent this by reading the rest of the file on close() or free() to leave the protocol in an idle state for the next command.

This was a possible issue for bundling because the amount to read is known in advance and therefore eof may not be reached. However, I was only able to reproduce this issue with unreleased code.

On error this issue would cause the original error to be lost. The process may still fail with this fix (if the error comes from another source) but hopefully we'll get better information about the original error.
This commit is contained in:
David Steele
2022-10-18 11:09:00 +13:00
committed by GitHub
parent ddd966cadc
commit 7967c750d8
3 changed files with 113 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files

11
doc/xml/release.xml
View File

@@ -31,6 +31,17 @@
<p>Fix memory leak in file bundle <cmd>backup</cmd>/<cmd>restore</cmd>.</p>
</release-item>
<release-item>
<github-pull-request id="1900"/>
<release-item-contributor-list>
<release-item-contributor id="david.steele"/>
<release-item-reviewer id="stephen.frost"/>
</release-item-contributor-list>
<p>Fix protocol error on short read of remote file.</p>
</release-item>
</release-bug-list>
<release-improvement-list>

75
src/storage/remote/read.c
View File

@@ -8,6 +8,7 @@ Remote Storage Read
#include "common/compress/helper.h"
#include "common/debug.h"
#include "common/io/io.h"
#include "common/io/read.h"
#include "common/log.h"
#include "common/type/convert.h"
@@ -43,6 +44,53 @@ Macros for function logging
#define FUNCTION_LOG_STORAGE_READ_REMOTE_FORMAT(value, buffer, bufferSize) \
objToLog(value, "StorageReadRemote", buffer, bufferSize)
/***********************************************************************************************************************************
Clear protocol if the entire file is not read or an error occurs before the read is complete. This is required to clear the
protocol state so a subsequent command can succeed.
***********************************************************************************************************************************/
static void
storageReadRemoteFreeResource(THIS_VOID)
{
THIS(StorageReadRemote);
FUNCTION_LOG_BEGIN(logLevelTrace);
FUNCTION_LOG_PARAM(STORAGE_READ_REMOTE, this);
FUNCTION_LOG_END();
ASSERT(this != NULL);
// Read if eof has not been reached
if (!this->eof)
{
do
{
MEM_CONTEXT_TEMP_BEGIN()
{
PackRead *const read = protocolClientDataGet(this->client);
pckReadNext(read);
// If binary then discard
if (pckReadType(read) == pckTypeBin)
{
pckReadBinP(read);
}
// Else read is complete so discard the filter list
else
{
pckReadPackP(read);
protocolClientDataEndGet(this->client);
this->eof = true;
}
}
MEM_CONTEXT_TEMP_END();
}
while (!this->eof);
}
FUNCTION_LOG_RETURN_VOID();
}
/***********************************************************************************************************************************
Read from a file
***********************************************************************************************************************************/
@@ -199,6 +247,9 @@ storageReadRemoteOpen(THIS_VOID)
// If the file is compressible add decompression filter locally
if (this->interface.compressible)
ioFilterGroupAdd(ioReadFilterGroup(storageReadIo(this->read)), decompressFilter(compressTypeGz));
// Set free callback to ensure the protocol is cleared on a short read
memContextCallbackSet(objMemContext(this), storageReadRemoteFreeResource, this);
}
// Else nothing to do
else
@@ -209,6 +260,27 @@ storageReadRemoteOpen(THIS_VOID)
FUNCTION_LOG_RETURN(BOOL, result);
}
/***********************************************************************************************************************************
Close the file and read any remaining data. It is possible that all data has been read but if the amount of data is exactly
divisible by the buffer size then the eof may not have been received.
***********************************************************************************************************************************/
static void
storageReadRemoteClose(THIS_VOID)
{
THIS(StorageReadRemote);
FUNCTION_LOG_BEGIN(logLevelTrace);
FUNCTION_LOG_PARAM(STORAGE_READ_REMOTE, this);
FUNCTION_LOG_END();
ASSERT(this != NULL);
memContextCallbackClear(objMemContext(this));
storageReadRemoteFreeResource(this);
FUNCTION_LOG_RETURN_VOID();
}
/**********************************************************************************************************************************/
StorageRead *
storageReadRemoteNew(
@@ -232,7 +304,7 @@ storageReadRemoteNew(
StorageReadRemote *this = NULL;
OBJ_NEW_BEGIN(StorageReadRemote, .childQty = MEM_CONTEXT_QTY_MAX, .allocQty = MEM_CONTEXT_QTY_MAX)
OBJ_NEW_BEGIN(StorageReadRemote, .childQty = MEM_CONTEXT_QTY_MAX, .allocQty = MEM_CONTEXT_QTY_MAX, .callbackQty = 1)
{
this = OBJ_NEW_ALLOC();
@@ -253,6 +325,7 @@ storageReadRemoteNew(
.ioInterface = (IoReadInterface)
{
.close = storageReadRemoteClose,
.eof = storageReadRemoteEof,
.open = storageReadRemoteOpen,
.read = storageReadRemote,

28
test/src/module/storage/remoteTest.c
View File

@@ -276,6 +276,34 @@ testRun(void)
TEST_RESULT_STR_Z(strNewBuf(storageGetP(fileRead)), "BABABABABAB", "check contents");
TEST_RESULT_UINT(((StorageReadRemote *)fileRead->driver)->protocolReadBytes, 11, "check read size");
// -------------------------------------------------------------------------------------------------------------------------
TEST_TITLE("read partial file then close");
size_t bufferOld = ioBufferSize();
ioBufferSizeSet(11);
Buffer *buffer = bufNew(11);
TEST_ASSIGN(fileRead, storageNewReadP(storageRepo, STRDEF("test.txt"), .limit = VARUINT64(11)), "get file");
TEST_RESULT_BOOL(ioReadOpen(storageReadIo(fileRead)), true, "open read");
TEST_RESULT_UINT(ioRead(storageReadIo(fileRead), buffer), 11, "partial read");
TEST_RESULT_STR_Z(strNewBuf(buffer), "BABABABABAB", "check contents");
TEST_RESULT_BOOL(ioReadEof(storageReadIo(fileRead)), false, "no eof");
TEST_RESULT_VOID(ioReadClose(storageReadIo(fileRead)), "close");
ioBufferSizeSet(bufferOld);
// -------------------------------------------------------------------------------------------------------------------------
TEST_TITLE("read partial file then free");
buffer = bufNew(6);
TEST_ASSIGN(fileRead, storageNewReadP(storageRepo, STRDEF("test.txt")), "get file");
TEST_RESULT_BOOL(ioReadOpen(storageReadIo(fileRead)), true, "open read");
TEST_RESULT_UINT(ioRead(storageReadIo(fileRead), buffer), 6, "partial read");
TEST_RESULT_STR_Z(strNewBuf(buffer), "BABABA", "check contents");
TEST_RESULT_BOOL(ioReadEof(storageReadIo(fileRead)), false, "no eof");
TEST_RESULT_VOID(storageReadFree(fileRead), "free");
// -------------------------------------------------------------------------------------------------------------------------
TEST_TITLE("read file with compression");