From c52b673bf854db4bf7e0a9de350ed9af1c99d575 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Wed, 24 Jan 2024 15:03:55 +0100 Subject: [PATCH] util: explicitly close all AutoFiles that have been written There is no way to report a close error from `AutoFile` destructor. Such an error could be serious if the file has been written to because it may mean the file is now corrupted (same as if write fails). So, change all users of `AutoFile` that use it to write data to explicitly close the file and handle a possible error. --- src/addrdb.cpp | 13 +++-- src/bench/streams_findbyte.cpp | 2 +- src/index/blockfilterindex.cpp | 10 +++- src/net.cpp | 5 ++ src/node/blockstorage.cpp | 62 ++++++++++++++++------ src/node/mempool_persist.cpp | 10 +++- src/policy/fees.cpp | 4 +- src/rpc/blockchain.cpp | 6 ++- src/streams.cpp | 1 + src/streams.h | 30 +++++++++-- src/test/flatfile_tests.cpp | 4 ++ src/test/fuzz/autofile.cpp | 7 ++- src/test/fuzz/policy_estimator.cpp | 1 + src/test/fuzz/policy_estimator_io.cpp | 1 + src/test/fuzz/utxo_snapshot.cpp | 1 + src/test/streams_tests.cpp | 31 ++++++----- src/test/util/chainstate.h | 2 +- src/wallet/test/fuzz/wallet_bdb_parser.cpp | 1 + 18 files changed, 145 insertions(+), 46 deletions(-) diff --git a/src/addrdb.cpp b/src/addrdb.cpp index 889f7b3859b..ad07e48d12c 100644 --- a/src/addrdb.cpp +++ b/src/addrdb.cpp @@ -24,6 +24,7 @@ #include #include #include +#include #include namespace { @@ -61,7 +62,6 @@ bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data FILE *file = fsbridge::fopen(pathTmp, "wb"); AutoFile fileout{file}; if (fileout.IsNull()) { - fileout.fclose(); remove(pathTmp); LogError("%s: Failed to open file %s\n", __func__, fs::PathToString(pathTmp)); return false; @@ -69,17 +69,22 @@ bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data // Serialize if (!SerializeDB(fileout, data)) { - fileout.fclose(); + (void)fileout.fclose(); remove(pathTmp); return false; } if (!fileout.Commit()) { - fileout.fclose(); + (void)fileout.fclose(); remove(pathTmp); LogError("%s: Failed to flush file %s\n", __func__, fs::PathToString(pathTmp)); return false; } - fileout.fclose(); + if (fileout.fclose() != 0) { + const int errno_save{errno}; + remove(pathTmp); + LogError("Failed to close file %s: %s", fs::PathToString(pathTmp), SysErrorString(errno_save)); + return false; + } // replace existing file, if any, with new file if (!RenameOver(pathTmp, path)) { diff --git a/src/bench/streams_findbyte.cpp b/src/bench/streams_findbyte.cpp index 004bf8ffc9d..c2bb3c3a310 100644 --- a/src/bench/streams_findbyte.cpp +++ b/src/bench/streams_findbyte.cpp @@ -28,7 +28,7 @@ static void FindByte(benchmark::Bench& bench) }); // Cleanup - file.fclose(); + assert(file.fclose() == 0); fs::remove("streams_tmp"); } diff --git a/src/index/blockfilterindex.cpp b/src/index/blockfilterindex.cpp index 5ce85e1f844..9e6ea8d7d00 100644 --- a/src/index/blockfilterindex.cpp +++ b/src/index/blockfilterindex.cpp @@ -151,7 +151,7 @@ bool BlockFilterIndex::CustomCommit(CDBBatch& batch) LogError("%s: Failed to open filter file %d\n", __func__, pos.nFile); return false; } - if (!file.Commit()) { + if (!file.Commit() || file.fclose() != 0) { LogError("%s: Failed to commit filter file %d\n", __func__, pos.nFile); return false; } @@ -205,7 +205,7 @@ size_t BlockFilterIndex::WriteFilterToDisk(FlatFilePos& pos, const BlockFilter& LogPrintf("%s: Failed to truncate filter file %d\n", __func__, pos.nFile); return 0; } - if (!last_file.Commit()) { + if (!last_file.Commit() || last_file.fclose() != 0) { LogPrintf("%s: Failed to commit filter file %d\n", __func__, pos.nFile); return 0; } @@ -229,6 +229,12 @@ size_t BlockFilterIndex::WriteFilterToDisk(FlatFilePos& pos, const BlockFilter& } fileout << filter.GetBlockHash() << filter.GetEncodedFilter(); + + if (fileout.fclose() != 0) { + LogPrintf("Failed to close filter file %d: %s", pos.nFile, SysErrorString(errno)); + return 0; + } + return data_size; } diff --git a/src/net.cpp b/src/net.cpp index fc0edc1a5c1..c0e8f6c85b6 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -4012,6 +4012,11 @@ static void CaptureMessageToFile(const CAddress& addr, uint32_t size = data.size(); ser_writedata32(f, size); f << data; + + if (f.fclose() != 0) { + throw std::ios_base::failure( + strprintf("Error closing %s after write, file contents are likely incomplete", fs::PathToString(path))); + } } std::functionGetBlockHash() << blockundo; - // Write undo data & checksum - fileout << blockundo << hasher.GetHash(); + BufferedWriter fileout{file}; + + // Write index header + fileout << GetParams().MessageStart() << blockundo_size; + pos.nPos += STORAGE_HEADER_BYTES; + { + // Calculate checksum + HashWriter hasher{}; + hasher << block.pprev->GetBlockHash() << blockundo; + // Write undo data & checksum + fileout << blockundo << hasher.GetHash(); + } } - fileout.flush(); // Make sure `AutoFile`/`BufferedWriter` go out of scope before we call `FlushUndoFile` + if (file.fclose() != 0) { + LogPrintLevel(BCLog::BLOCKSTORAGE, + BCLog::Level::Error, + "Failed to close block undo file %s: %s", + pos.ToString(), + SysErrorString(errno)); + return FatalError(m_opts.notifications, state, _("Failed to close block undo file.")); + } + // Make sure that the file is closed before we call `FlushUndoFile`. } // rev files are written in block height order, whereas blk files are written as blocks come in (often out of order) @@ -1093,13 +1103,26 @@ FlatFilePos BlockManager::WriteBlock(const CBlock& block, int nHeight) m_opts.notifications.fatalError(_("Failed to write block.")); return FlatFilePos(); } - BufferedWriter fileout{file}; + { + BufferedWriter fileout{file}; + + // Write index header + fileout << GetParams().MessageStart() << block_size; + pos.nPos += STORAGE_HEADER_BYTES; + // Write block + fileout << TX_WITH_WITNESS(block); + } + + if (file.fclose() != 0) { + LogPrintLevel(BCLog::BLOCKSTORAGE, + BCLog::Level::Error, + "Failed to close block file %s: %s", + pos.ToString(), + SysErrorString(errno)); + m_opts.notifications.fatalError(_("Failed to close file when writing block.")); + return FlatFilePos(); + } - // Write index header - fileout << GetParams().MessageStart() << block_size; - pos.nPos += STORAGE_HEADER_BYTES; - // Write block - fileout << TX_WITH_WITNESS(block); return pos; } @@ -1142,6 +1165,11 @@ static auto InitBlocksdirXorKey(const BlockManager::Options& opts) #endif )}; xor_key_file << xor_key; + if (xor_key_file.fclose() != 0) { + throw std::runtime_error{strprintf("Error closing XOR key file %s: %s\n", + fs::PathToString(xor_key_path), + SysErrorString(errno))}; + } } // If the user disabled the key, it must be zero. if (!opts.use_xor && xor_key != decltype(xor_key){}) { diff --git a/src/node/mempool_persist.cpp b/src/node/mempool_persist.cpp index ff7de8c64a2..2551a43166f 100644 --- a/src/node/mempool_persist.cpp +++ b/src/node/mempool_persist.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include @@ -168,7 +169,8 @@ bool DumpMempool(const CTxMemPool& pool, const fs::path& dump_path, FopenFn mock auto mid = SteadyClock::now(); - AutoFile file{mockable_fopen_function(dump_path + ".new", "wb")}; + const fs::path file_fspath{dump_path + ".new"}; + AutoFile file{mockable_fopen_function(file_fspath, "wb")}; if (file.IsNull()) { return false; } @@ -201,7 +203,10 @@ bool DumpMempool(const CTxMemPool& pool, const fs::path& dump_path, FopenFn mock if (!skip_file_commit && !file.Commit()) throw std::runtime_error("Commit failed"); - file.fclose(); + if (file.fclose() != 0) { + throw std::runtime_error( + strprintf("Error closing %s: %s", fs::PathToString(file_fspath), SysErrorString(errno))); + } if (!RenameOver(dump_path + ".new", dump_path)) { throw std::runtime_error("Rename failed"); } @@ -213,6 +218,7 @@ bool DumpMempool(const CTxMemPool& pool, const fs::path& dump_path, FopenFn mock fs::file_size(dump_path)); } catch (const std::exception& e) { LogInfo("Failed to dump mempool: %s. Continuing anyway.\n", e.what()); + (void)file.fclose(); return false; } return true; diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index e85b2f2caaa..ea763efc986 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -953,8 +953,8 @@ void CBlockPolicyEstimator::Flush() { void CBlockPolicyEstimator::FlushFeeEstimates() { AutoFile est_file{fsbridge::fopen(m_estimation_filepath, "wb")}; - if (est_file.IsNull() || !Write(est_file)) { - LogPrintf("Failed to write fee estimates to %s. Continue anyway.\n", fs::PathToString(m_estimation_filepath)); + if (est_file.IsNull() || !Write(est_file) || est_file.fclose() != 0) { + LogWarning("Failed to write fee estimates to %s. Continuing anyway.", fs::PathToString(m_estimation_filepath)); } else { LogPrintf("Flushed fee estimates to %s.\n", fs::PathToString(m_estimation_filepath.filename())); } diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 6e5c656f3d8..ae6b11c41f0 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -47,6 +47,7 @@ #include #include #include +#include #include #include #include @@ -3217,7 +3218,10 @@ UniValue WriteUTXOSnapshot( CHECK_NONFATAL(written_coins_count == maybe_stats->coins_count); - afile.fclose(); + if (afile.fclose() != 0) { + throw std::ios_base::failure( + strprintf("Error closing %s: %s", fs::PathToString(temppath), SysErrorString(errno))); + } UniValue result(UniValue::VOBJ); result.pushKV("coins_written", written_coins_count); diff --git a/src/streams.cpp b/src/streams.cpp index 19c2b474452..20df488e640 100644 --- a/src/streams.cpp +++ b/src/streams.cpp @@ -95,6 +95,7 @@ void AutoFile::write(std::span src) src = src.subspan(buf_now.size()); } } + m_was_written = true; } void AutoFile::write_buffer(std::span src) diff --git a/src/streams.h b/src/streams.h index 1ebcff3671f..c13cebc5c6c 100644 --- a/src/streams.h +++ b/src/streams.h @@ -6,10 +6,13 @@ #ifndef BITCOIN_STREAMS_H #define BITCOIN_STREAMS_H +#include #include #include #include +#include #include +#include #include #include @@ -386,7 +389,13 @@ public: * * Will automatically close the file when it goes out of scope if not null. * If you're returning the file pointer, return file.release(). - * If you need to close the file early, use file.fclose() instead of fclose(file). + * If you need to close the file early, use autofile.fclose() instead of fclose(underlying_FILE). + * + * @note If the file has been written to, then the caller must close it + * explicitly with the `fclose()` method, check if it returns an error and treat + * such an error as if the `write()` method failed. The OS's `fclose(3)` may + * fail to flush to disk data that has been previously written, rendering the + * file corrupt. */ class AutoFile { @@ -394,11 +403,26 @@ protected: std::FILE* m_file; std::vector m_xor; std::optional m_position; + bool m_was_written{false}; public: explicit AutoFile(std::FILE* file, std::vector data_xor={}); - ~AutoFile() { fclose(); } + ~AutoFile() + { + if (m_was_written) { + // Callers that wrote to the file must have closed it explicitly + // with the fclose() method and checked that the close succeeded. + // This is because here from the destructor we have no way to signal + // error due to close which, after write, could mean the file is + // corrupted and must be handled properly at the call site. + Assume(IsNull()); + } + + if (fclose() != 0) { + LogPrintLevel(BCLog::ALL, BCLog::Level::Error, "Failed to close file: %s", SysErrorString(errno)); + } + } // Disallow copies AutoFile(const AutoFile&) = delete; @@ -406,7 +430,7 @@ public: bool feof() const { return std::feof(m_file); } - int fclose() + [[nodiscard]] int fclose() { if (auto rel{release()}) return std::fclose(rel); return 0; diff --git a/src/test/flatfile_tests.cpp b/src/test/flatfile_tests.cpp index 21a36d9d434..d94cab640b8 100644 --- a/src/test/flatfile_tests.cpp +++ b/src/test/flatfile_tests.cpp @@ -46,6 +46,7 @@ BOOST_AUTO_TEST_CASE(flatfile_open) { AutoFile file{seq.Open(FlatFilePos(0, pos1))}; file << LIMITED_STRING(line1, 256); + BOOST_REQUIRE_EQUAL(file.fclose(), 0); } // Attempt to append to file opened in read-only mode. @@ -58,6 +59,7 @@ BOOST_AUTO_TEST_CASE(flatfile_open) { AutoFile file{seq.Open(FlatFilePos(0, pos2))}; file << LIMITED_STRING(line2, 256); + BOOST_REQUIRE_EQUAL(file.fclose(), 0); } // Read text from file in read-only mode. @@ -79,6 +81,7 @@ BOOST_AUTO_TEST_CASE(flatfile_open) file >> LIMITED_STRING(text, 256); BOOST_CHECK_EQUAL(text, line2); + BOOST_REQUIRE_EQUAL(file.fclose(), 0); } // Ensure another file in the sequence has no data. @@ -86,6 +89,7 @@ BOOST_AUTO_TEST_CASE(flatfile_open) std::string text; AutoFile file{seq.Open(FlatFilePos(1, pos2))}; BOOST_CHECK_THROW(file >> LIMITED_STRING(text, 256), std::ios_base::failure); + BOOST_REQUIRE_EQUAL(file.fclose(), 0); } } diff --git a/src/test/fuzz/autofile.cpp b/src/test/fuzz/autofile.cpp index 81761c7bf96..cdbae9c3cca 100644 --- a/src/test/fuzz/autofile.cpp +++ b/src/test/fuzz/autofile.cpp @@ -47,7 +47,7 @@ FUZZ_TARGET(autofile) } }, [&] { - auto_file.fclose(); + (void)auto_file.fclose(); }, [&] { ReadFromStream(fuzzed_data_provider, auto_file); @@ -62,5 +62,10 @@ FUZZ_TARGET(autofile) if (f != nullptr) { fclose(f); } + } else { + // FuzzedFileProvider::close() is expected to fail sometimes. Don't let + // the destructor of AutoFile be upset by a failing fclose(). Close it + // explicitly (and ignore any errors) so that the destructor is a noop. + (void)auto_file.fclose(); } } diff --git a/src/test/fuzz/policy_estimator.cpp b/src/test/fuzz/policy_estimator.cpp index 29427403952..09a777a3151 100644 --- a/src/test/fuzz/policy_estimator.cpp +++ b/src/test/fuzz/policy_estimator.cpp @@ -104,5 +104,6 @@ FUZZ_TARGET(policy_estimator, .init = initialize_policy_estimator) AutoFile fuzzed_auto_file{fuzzed_file_provider.open()}; block_policy_estimator.Write(fuzzed_auto_file); block_policy_estimator.Read(fuzzed_auto_file); + (void)fuzzed_auto_file.fclose(); } } diff --git a/src/test/fuzz/policy_estimator_io.cpp b/src/test/fuzz/policy_estimator_io.cpp index 3e7d0933439..813674e25c1 100644 --- a/src/test/fuzz/policy_estimator_io.cpp +++ b/src/test/fuzz/policy_estimator_io.cpp @@ -32,4 +32,5 @@ FUZZ_TARGET(policy_estimator_io, .init = initialize_policy_estimator_io) if (block_policy_estimator.Read(fuzzed_auto_file)) { block_policy_estimator.Write(fuzzed_auto_file); } + (void)fuzzed_auto_file.fclose(); } diff --git a/src/test/fuzz/utxo_snapshot.cpp b/src/test/fuzz/utxo_snapshot.cpp index 13f60881926..d339b820966 100644 --- a/src/test/fuzz/utxo_snapshot.cpp +++ b/src/test/fuzz/utxo_snapshot.cpp @@ -150,6 +150,7 @@ void utxo_snapshot_fuzz(FuzzBufferType buffer) WriteCompactSize(outfile, 999); // index of coin outfile << Coin{coinbase->vout[0], /*nHeightIn=*/999, /*fCoinBaseIn=*/0}; } + assert(outfile.fclose() == 0); } const auto ActivateFuzzedSnapshot{[&] { diff --git a/src/test/streams_tests.cpp b/src/test/streams_tests.cpp index c7b5cd353e0..880830d2940 100644 --- a/src/test/streams_tests.cpp +++ b/src/test/streams_tests.cpp @@ -39,6 +39,7 @@ BOOST_AUTO_TEST_CASE(xor_file) #endif AutoFile xor_file{raw_file(mode), xor_pat}; xor_file << test1 << test2; + BOOST_REQUIRE_EQUAL(xor_file.fclose(), 0); } { // Read raw from disk @@ -379,8 +380,8 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file) // by the rewind window (relative to our farthest read position, 40). BOOST_CHECK(bf.GetPos() <= 30U); - // We can explicitly close the file, or the destructor will do it. - file.fclose(); + // Explicitly close the file and check that the close succeeds. + BOOST_REQUIRE_EQUAL(file.fclose(), 0); fs::remove(streams_test_filename); } @@ -430,7 +431,7 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file_skip) bf.SkipTo(13); BOOST_CHECK_EQUAL(bf.GetPos(), 13U); - file.fclose(); + BOOST_REQUIRE_EQUAL(file.fclose(), 0); fs::remove(streams_test_filename); } @@ -551,6 +552,7 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file_rand) if (maxPos < currentPos) maxPos = currentPos; } + BOOST_REQUIRE_EQUAL(file.fclose(), 0); } fs::remove(streams_test_filename); } @@ -566,7 +568,9 @@ BOOST_AUTO_TEST_CASE(buffered_reader_matches_autofile_random_content) // Write out the file with random content { - AutoFile{test_file.Open(pos, /*read_only=*/false), obfuscation}.write(m_rng.randbytes(file_size)); + AutoFile f{test_file.Open(pos, /*read_only=*/false), obfuscation}; + f.write(m_rng.randbytes(file_size)); + BOOST_REQUIRE_EQUAL(f.fclose(), 0); } BOOST_CHECK_EQUAL(fs::file_size(test_file.FileName(pos)), file_size); @@ -623,18 +627,21 @@ BOOST_AUTO_TEST_CASE(buffered_writer_matches_autofile_random_content) AutoFile direct_file{test_direct.Open(pos, /*read_only=*/false), obfuscation}; AutoFile buffered_file{test_buffered.Open(pos, /*read_only=*/false), obfuscation}; - BufferedWriter buffered{buffered_file, buf_size}; + { + BufferedWriter buffered{buffered_file, buf_size}; - for (size_t total_written{0}; total_written < file_size;) { - const size_t write_size{Assert(std::min(1 + m_rng.randrange(m_rng.randbool() ? buf_size : 2 * buf_size), file_size - total_written))}; + for (size_t total_written{0}; total_written < file_size;) { + const size_t write_size{Assert(std::min(1 + m_rng.randrange(m_rng.randbool() ? buf_size : 2 * buf_size), file_size - total_written))}; - auto current_span = std::span{test_data}.subspan(total_written, write_size); - direct_file.write(current_span); - buffered.write(current_span); + auto current_span = std::span{test_data}.subspan(total_written, write_size); + direct_file.write(current_span); + buffered.write(current_span); - total_written += write_size; + total_written += write_size; + } } - // Destructors of AutoFile and BufferedWriter will flush/close here + BOOST_REQUIRE_EQUAL(buffered_file.fclose(), 0); + BOOST_REQUIRE_EQUAL(direct_file.fclose(), 0); } // Compare the resulting files diff --git a/src/test/util/chainstate.h b/src/test/util/chainstate.h index e604b44c16a..eebf504d985 100644 --- a/src/test/util/chainstate.h +++ b/src/test/util/chainstate.h @@ -48,7 +48,7 @@ CreateAndActivateUTXOSnapshot( AutoFile auto_outfile{outfile}; UniValue result = CreateUTXOSnapshot( - node, node.chainman->ActiveChainstate(), auto_outfile, snapshot_path, snapshot_path); + node, node.chainman->ActiveChainstate(), auto_outfile, snapshot_path, snapshot_path); // Will close auto_outfile. LogPrintf( "Wrote UTXO snapshot to %s: %s\n", fs::PathToString(snapshot_path.make_preferred()), result.write()); diff --git a/src/wallet/test/fuzz/wallet_bdb_parser.cpp b/src/wallet/test/fuzz/wallet_bdb_parser.cpp index c9e9ed95b50..f5336d9b4d4 100644 --- a/src/wallet/test/fuzz/wallet_bdb_parser.cpp +++ b/src/wallet/test/fuzz/wallet_bdb_parser.cpp @@ -45,6 +45,7 @@ FUZZ_TARGET(wallet_bdb_parser, .init = initialize_wallet_bdb_parser) { AutoFile outfile{fsbridge::fopen(wallet_path, "wb")}; outfile << std::span{buffer}; + assert(outfile.fclose() == 0); } const DatabaseOptions options{};