From 4533445fd7ddc9cbdbd582f19088c940257dbe80 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 | 12 ++++++---- src/bench/streams_findbyte.cpp | 2 +- src/index/blockfilterindex.cpp | 10 +++++++-- src/net.cpp | 5 +++++ src/node/blockstorage.cpp | 10 +++++++++ src/node/mempool_persist.cpp | 10 +++++++-- src/policy/fees.cpp | 2 +- src/rpc/blockchain.cpp | 6 ++++- src/streams.cpp | 1 + src/streams.h | 26 +++++++++++++++++++--- 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 | 8 ++++--- src/test/util/chainstate.h | 2 +- src/wallet/test/fuzz/wallet_bdb_parser.cpp | 1 + 18 files changed, 90 insertions(+), 19 deletions(-) diff --git a/src/addrdb.cpp b/src/addrdb.cpp index e9838d722291e2..82a0778d2bc662 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,21 @@ 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 (!FileCommit(fileout.Get())) { - 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) { + remove(pathTmp); + LogError("%s: Failed to close file %s: %s\n", __func__, fs::PathToString(pathTmp), SysErrorString(errno)); + 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 5098262e9afa63..2ba9c9728d9e97 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(); + (void)file.fclose(); fs::remove("streams_tmp"); } diff --git a/src/index/blockfilterindex.cpp b/src/index/blockfilterindex.cpp index 41bdca9df562a0..dbc08129608d56 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 (!FileCommit(file.Get())) { + if (!FileCommit(file.Get()) || 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 (!FileCommit(last_file.Get())) { + if (!FileCommit(last_file.Get()) || 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("%s: Failed to close filter file %d\n", __func__, pos.nFile); + return 0; + } + return data_size; } diff --git a/src/net.cpp b/src/net.cpp index 3d3f9f4ba7d05a..8bc9698bd724a1 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3943,6 +3943,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 is likely incomplete", fs::PathToString(path))); + } } std::function #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 && !FileCommit(file.Get())) throw std::runtime_error("FileCommit 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 5f1d15c5f27a59..5b6fe2e32cfd7f 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -950,7 +950,7 @@ void CBlockPolicyEstimator::Flush() { void CBlockPolicyEstimator::FlushFeeEstimates() { AutoFile est_file{fsbridge::fopen(m_estimation_filepath, "wb")}; - if (est_file.IsNull() || !Write(est_file)) { + if (est_file.IsNull() || !Write(est_file) || est_file.fclose() != 0) { LogPrintf("Failed to write fee estimates to %s. Continue anyway.\n", 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 9899a13a1e3bce..66a3a5720a0b66 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -45,6 +45,7 @@ #include #include #include +#include #include #include #include @@ -2788,7 +2789,10 @@ UniValue CreateUTXOSnapshot( 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 cdd36a86feb17e..d5a3e399fc58b0 100644 --- a/src/streams.cpp +++ b/src/streams.cpp @@ -85,4 +85,5 @@ void AutoFile::write(Span src) current_pos += buf_now.size(); } } + m_was_written = true; } diff --git a/src/streams.h b/src/streams.h index c2a9dea2878d1f..dc0761c9e19ccd 100644 --- a/src/streams.h +++ b/src/streams.h @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -383,18 +384,37 @@ class BitStreamWriter * * 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 { protected: std::FILE* m_file; std::vector m_xor; + bool m_was_written{false}; public: explicit AutoFile(std::FILE* file, std::vector data_xor={}) : m_file{file}, m_xor{std::move(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()); + } + + (void)fclose(); + } // Disallow copies AutoFile(const AutoFile&) = delete; @@ -402,7 +422,7 @@ class AutoFile 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 21a36d9d4343ce..d94cab640b8264 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 45316b6b218dbb..95ff4a26a5a5ef 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); @@ -63,5 +63,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 a4e1947b9f6e30..611bbed1d41871 100644 --- a/src/test/fuzz/policy_estimator.cpp +++ b/src/test/fuzz/policy_estimator.cpp @@ -95,5 +95,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 3e7d0933439411..813674e25c1ebc 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 522c9c54eed4b4..e4210efeb0c476 100644 --- a/src/test/fuzz/utxo_snapshot.cpp +++ b/src/test/fuzz/utxo_snapshot.cpp @@ -69,6 +69,7 @@ FUZZ_TARGET(utxo_snapshot, .init = initialize_chain) height++; } } + assert(outfile.fclose() == 0); } const auto ActivateFuzzedSnapshot{[&] { diff --git a/src/test/streams_tests.cpp b/src/test/streams_tests.cpp index eed932b6d2992f..4968660268b643 100644 --- a/src/test/streams_tests.cpp +++ b/src/test/streams_tests.cpp @@ -38,6 +38,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 @@ -378,8 +379,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); } @@ -429,7 +430,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); } @@ -550,6 +551,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); } diff --git a/src/test/util/chainstate.h b/src/test/util/chainstate.h index a4636365ca3e39..2d5718201c2316 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 6fbd695fc5ee2e..35f52fa33e31f9 100644 --- a/src/wallet/test/fuzz/wallet_bdb_parser.cpp +++ b/src/wallet/test/fuzz/wallet_bdb_parser.cpp @@ -38,6 +38,7 @@ FUZZ_TARGET(wallet_bdb_parser, .init = initialize_wallet_bdb_parser) { AutoFile outfile{fsbridge::fopen(wallet_path, "wb")}; outfile << Span{buffer}; + assert(outfile.fclose() == 0); } const DatabaseOptions options{};