From d211c2b8b51ae5b796bbb581d21a4a406e3ed972 Mon Sep 17 00:00:00 2001 From: David Steele Date: Fri, 15 Feb 2019 11:52:39 +0200 Subject: [PATCH] Fix possible truncated WAL segments when an error occurs mid-write. The file write object destructors called close() and finalized the file even if it was not completely written. This was an issue in both the C and Perl code. Rewrite the destructors to simply free resources (like file handles) rather than calling the close() method. This leaves the temp file in place for filesystems that use temp files. Add unit tests to prevent regression. Reported by blogh. --- doc/xml/release.xml | 10 ++++++++++ lib/pgBackRest/Common/Io/Base.pm | 5 ----- lib/pgBackRest/Common/Io/Handle.pm | 2 +- lib/pgBackRest/Storage/Posix/FileWrite.pm | 14 ++++++++++++++ src/perl/embed.auto.c | 13 +++++++++++-- src/storage/driver/posix/fileWrite.c | 9 +++++++-- .../Module/Common/CommonIoHandlePerlTest.pm | 1 + .../Module/Storage/StoragePosixPerlTest.pm | 14 ++++++++++++++ .../Module/Storage/StorageS3PerlTest.pm | 11 +++++++++++ test/src/module/storage/posixTest.c | 18 ++++++++++++++++++ 10 files changed, 87 insertions(+), 10 deletions(-) diff --git a/doc/xml/release.xml b/doc/xml/release.xml index 28863fa1a..b75c21a4b 100644 --- a/doc/xml/release.xml +++ b/doc/xml/release.xml @@ -14,6 +14,16 @@ + + + + + + +

Fix possible truncated WAL segments when an error occurs mid-write.

+ + + diff --git a/lib/pgBackRest/Common/Io/Base.pm b/lib/pgBackRest/Common/Io/Base.pm index e278c9b73..b5f6475f2 100644 --- a/lib/pgBackRest/Common/Io/Base.pm +++ b/lib/pgBackRest/Common/Io/Base.pm @@ -105,11 +105,6 @@ sub resultSet $self->{rhResult}{$strModule} = $xResult; } -#################################################################################################################################### -# DESTROY - call close() -#################################################################################################################################### -sub DESTROY {shift->close()} - #################################################################################################################################### # Getters #################################################################################################################################### diff --git a/lib/pgBackRest/Common/Io/Handle.pm b/lib/pgBackRest/Common/Io/Handle.pm index b6ce367e8..8f70fee24 100644 --- a/lib/pgBackRest/Common/Io/Handle.pm +++ b/lib/pgBackRest/Common/Io/Handle.pm @@ -157,7 +157,7 @@ sub write } #################################################################################################################################### -# close/DESTROY - record read/write size +# close - record read/write size #################################################################################################################################### sub close { diff --git a/lib/pgBackRest/Storage/Posix/FileWrite.pm b/lib/pgBackRest/Storage/Posix/FileWrite.pm index 64a92ac77..79ddb52a0 100644 --- a/lib/pgBackRest/Storage/Posix/FileWrite.pm +++ b/lib/pgBackRest/Storage/Posix/FileWrite.pm @@ -185,6 +185,20 @@ sub close return true; } +#################################################################################################################################### +# Close the handle if it is open (in case close() was never called) +#################################################################################################################################### +sub DESTROY +{ + my $self = shift; + + if (defined($self->handle())) + { + CORE::close($self->handle()); + undef($self->{fhFile}); + } +} + #################################################################################################################################### # Getters #################################################################################################################################### diff --git a/src/perl/embed.auto.c b/src/perl/embed.auto.c index c146c9e86..7644b127b 100644 --- a/src/perl/embed.auto.c +++ b/src/perl/embed.auto.c @@ -6477,8 +6477,6 @@ static const EmbeddedModule embeddedModule[] = "$self->{rhResult}{$strModule} = $xResult;\n" "}\n" "\n\n\n\n" - "sub DESTROY {shift->close()}\n" - "\n\n\n\n" "sub className {blessed(shift)}\n" "sub id {shift->{strId}}\n" "\n" @@ -19808,6 +19806,17 @@ static const EmbeddedModule embeddedModule[] = "return true;\n" "}\n" "\n\n\n\n" + "sub DESTROY\n" + "{\n" + "my $self = shift;\n" + "\n" + "if (defined($self->handle()))\n" + "{\n" + "CORE::close($self->handle());\n" + "undef($self->{fhFile});\n" + "}\n" + "}\n" + "\n\n\n\n" "sub handle {shift->{fhFile}}\n" "sub opened {shift->{bOpened}}\n" "sub name {shift->{strName}}\n" diff --git a/src/storage/driver/posix/fileWrite.c b/src/storage/driver/posix/fileWrite.c index 1d42d0ac7..3bcaf5683 100644 --- a/src/storage/driver/posix/fileWrite.c +++ b/src/storage/driver/posix/fileWrite.c @@ -352,9 +352,14 @@ storageDriverPosixFileWriteFree(StorageDriverPosixFileWrite *this) if (this != NULL) { - storageDriverPosixFileWriteClose(this); - memContextCallbackClear(this->memContext); + + // Close the temp file. *Close() must be called explicitly in order for the file to be sycn'ed, renamed, etc. If *Free() + // is called first the assumption is that some kind of error occurred and we should only close the handle to free + // resources. + if (this->handle != -1) + storageDriverPosixFileClose(this->handle, this->name, true); + memContextFree(this->memContext); } diff --git a/test/lib/pgBackRestTest/Module/Common/CommonIoHandlePerlTest.pm b/test/lib/pgBackRestTest/Module/Common/CommonIoHandlePerlTest.pm index a61d92c41..17e1f89b7 100644 --- a/test/lib/pgBackRestTest/Module/Common/CommonIoHandlePerlTest.pm +++ b/test/lib/pgBackRestTest/Module/Common/CommonIoHandlePerlTest.pm @@ -197,6 +197,7 @@ sub run my $oIoHandle = $self->testResult(sub {new pgBackRest::Common::Io::Handle("'$strFile'", $fhRead)}, '[object]', 'open read'); $self->testResult(sub {$oIoHandle->read(\$tContent, $iFileLengthHalf)}, $iFileLengthHalf, ' read'); $self->testResult(sub {$oIoHandle->close()}, true, ' close'); + $self->testResult(sub {$oIoHandle->close()}, true, ' close again'); $self->testResult(sub {$oIoHandle->result(COMMON_IO_HANDLE)}, $iFileLengthHalf, ' check result'); diff --git a/test/lib/pgBackRestTest/Module/Storage/StoragePosixPerlTest.pm b/test/lib/pgBackRestTest/Module/Storage/StoragePosixPerlTest.pm index a216f1e10..efc2600f8 100644 --- a/test/lib/pgBackRestTest/Module/Storage/StoragePosixPerlTest.pm +++ b/test/lib/pgBackRestTest/Module/Storage/StoragePosixPerlTest.pm @@ -304,6 +304,20 @@ sub run undef($oPosixIo); + # Test that a premature destroy (from error or otherwise) does not rename the file + #--------------------------------------------------------------------------------------------------------------------------- + my $strFileAbort = $self->testPath() . '/file-abort.txt'; + my $strFileAbortTmp = "${strFileAbort}.tmp"; + + $oPosixIo = $self->testResult( + sub {new pgBackRest::Storage::Posix::FileWrite($oPosix, $strFileAbort, {bAtomic => true})}, '[object]', 'open'); + + $oPosixIo->write(\$strFileContent); + undef($oPosixIo); + + $self->testResult(sub {$oPosix->exists($strFileAbort)}, false, 'destination file does not exist'); + $self->testResult(sub {$oPosix->exists($strFileAbortTmp)}, true, 'destination file tmp exists'); + #--------------------------------------------------------------------------------------------------------------------------- $oPosixIo = $self->testResult( sub {new pgBackRest::Storage::Posix::FileWrite($oPosix, $strFile, {lTimestamp => time()})}, '[object]', 'open'); diff --git a/test/lib/pgBackRestTest/Module/Storage/StorageS3PerlTest.pm b/test/lib/pgBackRestTest/Module/Storage/StorageS3PerlTest.pm index 9d1c93671..25bf32013 100644 --- a/test/lib/pgBackRestTest/Module/Storage/StorageS3PerlTest.pm +++ b/test/lib/pgBackRestTest/Module/Storage/StorageS3PerlTest.pm @@ -193,6 +193,17 @@ sub run $self->testResult(sub {$oFileWrite->write(\$strFileContent)}, $iFileLength, ' write'); $oFileWrite->close(); + $self->testResult(sub {$oS3->exists("/path/to/${strFile}" . '.@')}, true, 'destination file exists'); + + # Test that a premature destroy (from error or otherwise) does not rename the file + #--------------------------------------------------------------------------------------------------------------------------- + $oFileWrite = $self->testResult(sub {$oS3->openWrite("/path/to/abort.file" . '.@')}, '[object]', 'open write'); + $self->testResult(sub {$oFileWrite->write()}, 0, ' write undef'); + $self->testResult(sub {$oFileWrite->write(\$strFileContent)}, $iFileLength, ' write'); + + undef($oFileWrite); + $self->testResult(sub {$oS3->exists("/path/to/abort.file")}, false, 'destination file does not exist'); + #--------------------------------------------------------------------------------------------------------------------------- $oFileWrite = $self->testResult(sub {$oS3->openWrite("/path/to/${strFile}")}, '[object]', 'open write'); diff --git a/test/src/module/storage/posixTest.c b/test/src/module/storage/posixTest.c index 31cd2fe24..4893a0834 100644 --- a/test/src/module/storage/posixTest.c +++ b/test/src/module/storage/posixTest.c @@ -571,6 +571,21 @@ testRun(void) TEST_RESULT_INT(storageInfoNP(storageTest, strPath(fileName)).mode, 0750, " check path mode"); TEST_RESULT_INT(storageInfoNP(storageTest, fileName).mode, 0640, " check file mode"); + // Test that a premature free (from error or otherwise) does not rename the file + // ------------------------------------------------------------------------------------------------------------------------- + fileName = strNewFmt("%s/sub1/testfile-abort", testPath()); + String *fileNameTmp = strNewFmt("%s." STORAGE_FILE_TEMP_EXT, strPtr(fileName)); + + TEST_ASSIGN(file, storageNewWriteNP(storageTest, fileName), "new write file (defaults)"); + TEST_RESULT_VOID(ioWriteOpen(storageFileWriteIo(file)), " open file"); + TEST_RESULT_VOID(ioWrite(storageFileWriteIo(file), bufNewStr(strNew("TESTDATA"))), "write data"); + TEST_RESULT_VOID(ioWriteFlush(storageFileWriteIo(file)), "flush data"); + TEST_RESULT_VOID(ioWriteFree(storageFileWriteIo(file)), " free file"); + + TEST_RESULT_BOOL(storageExistsNP(storageTest, fileName), false, "destination file does not exist"); + TEST_RESULT_BOOL(storageExistsNP(storageTest, fileNameTmp), true, "destination tmp file exists"); + TEST_RESULT_INT(storageInfoNP(storageTest, fileNameTmp).size, 8, " check temp file size"); + // ------------------------------------------------------------------------------------------------------------------------- fileName = strNewFmt("%s/sub2/testfile", testPath()); @@ -578,6 +593,9 @@ testRun(void) file, storageNewWriteP(storageTest, fileName, .modePath = 0700, .modeFile = 0600), "new write file (set mode)"); TEST_RESULT_VOID(ioWriteOpen(storageFileWriteIo(file)), " open file"); TEST_RESULT_VOID(ioWriteClose(storageFileWriteIo(file)), " close file"); + TEST_RESULT_VOID( + storageDriverPosixFileWriteClose((StorageDriverPosixFileWrite *)storageFileWriteFileDriver(file)), + " close file again"); TEST_RESULT_INT(storageInfoNP(storageTest, strPath(fileName)).mode, 0700, " check path mode"); TEST_RESULT_INT(storageInfoNP(storageTest, fileName).mode, 0600, " check file mode");