From a69c4098b273b6db5d2212ba91cfc713c1634c5d Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Thu, 22 May 2025 11:59:50 +0200 Subject: [PATCH 1/4] rpc: take ownership of the file by WriteUTXOSnapshot() Have `WriteUTXOSnapshot()` take rvalue reference to make it obvious that it takes ownership of the file. --- src/rpc/blockchain.cpp | 24 +++++++++++++++++++----- src/rpc/blockchain.h | 2 +- src/test/util/chainstate.h | 7 +++++-- 3 files changed, 25 insertions(+), 8 deletions(-) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index dd70c499fd7..241cc367105 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -82,7 +82,7 @@ UniValue WriteUTXOSnapshot( CCoinsViewCursor* pcursor, CCoinsStats* maybe_stats, const CBlockIndex* tip, - AutoFile& afile, + AutoFile&& afile, const fs::path& path, const fs::path& temppath, const std::function& interruption_point = {}); @@ -3114,7 +3114,14 @@ static RPCHelpMan dumptxoutset() } } - UniValue result = WriteUTXOSnapshot(*chainstate, cursor.get(), &stats, tip, afile, path, temppath, node.rpc_interruption_point); + UniValue result = WriteUTXOSnapshot(*chainstate, + cursor.get(), + &stats, + tip, + std::move(afile), + path, + temppath, + node.rpc_interruption_point); fs::rename(temppath, path); result.pushKV("path", path.utf8string()); @@ -3166,7 +3173,7 @@ UniValue WriteUTXOSnapshot( CCoinsViewCursor* pcursor, CCoinsStats* maybe_stats, const CBlockIndex* tip, - AutoFile& afile, + AutoFile&& afile, const fs::path& path, const fs::path& temppath, const std::function& interruption_point) @@ -3240,12 +3247,19 @@ UniValue WriteUTXOSnapshot( UniValue CreateUTXOSnapshot( node::NodeContext& node, Chainstate& chainstate, - AutoFile& afile, + AutoFile&& afile, const fs::path& path, const fs::path& tmppath) { auto [cursor, stats, tip]{WITH_LOCK(::cs_main, return PrepareUTXOSnapshot(chainstate, node.rpc_interruption_point))}; - return WriteUTXOSnapshot(chainstate, cursor.get(), &stats, tip, afile, path, tmppath, node.rpc_interruption_point); + return WriteUTXOSnapshot(chainstate, + cursor.get(), + &stats, + tip, + std::move(afile), + path, + tmppath, + node.rpc_interruption_point); } static RPCHelpMan loadtxoutset() diff --git a/src/rpc/blockchain.h b/src/rpc/blockchain.h index b42fe96fd30..0e42bed9479 100644 --- a/src/rpc/blockchain.h +++ b/src/rpc/blockchain.h @@ -51,7 +51,7 @@ void CalculatePercentilesByWeight(CAmount result[NUM_GETBLOCKSTATS_PERCENTILES], UniValue CreateUTXOSnapshot( node::NodeContext& node, Chainstate& chainstate, - AutoFile& afile, + AutoFile&& afile, const fs::path& path, const fs::path& tmppath); diff --git a/src/test/util/chainstate.h b/src/test/util/chainstate.h index e604b44c16a..d5100d15b16 100644 --- a/src/test/util/chainstate.h +++ b/src/test/util/chainstate.h @@ -47,8 +47,11 @@ CreateAndActivateUTXOSnapshot( FILE* outfile{fsbridge::fopen(snapshot_path, "wb")}; AutoFile auto_outfile{outfile}; - UniValue result = CreateUTXOSnapshot( - node, node.chainman->ActiveChainstate(), auto_outfile, snapshot_path, snapshot_path); + UniValue result = CreateUTXOSnapshot(node, + node.chainman->ActiveChainstate(), + std::move(auto_outfile), // Will close auto_outfile. + snapshot_path, + snapshot_path); LogPrintf( "Wrote UTXO snapshot to %s: %s\n", fs::PathToString(snapshot_path.make_preferred()), result.write()); From 8bb34f07df9ad45faf25c32c99a4dd70759b25be Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Wed, 24 Jan 2024 15:03:55 +0100 Subject: [PATCH 2/4] 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 | 17 ++++++++ src/net.cpp | 5 +++ src/node/blockstorage.cpp | 46 ++++++++++++++++------ src/node/mempool_persist.cpp | 14 +++++-- src/policy/fees.cpp | 10 ++++- src/rpc/blockchain.cpp | 6 ++- src/test/flatfile_tests.cpp | 4 ++ src/test/fuzz/autofile.cpp | 4 +- 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 | 34 ++++++++++------ src/wallet/test/fuzz/wallet_bdb_parser.cpp | 1 + 15 files changed, 121 insertions(+), 38 deletions(-) diff --git a/src/addrdb.cpp b/src/addrdb.cpp index 889f7b3859b..7602aaba5da 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 after commit: %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 de43e8e271a..1c1175326a5 100644 --- a/src/index/blockfilterindex.cpp +++ b/src/index/blockfilterindex.cpp @@ -13,6 +13,7 @@ #include #include #include +#include /* The index database stores three items for each block: the disk location of the encoded filter, * its dSHA256 hash, and the header. Those belonging to blocks on the active chain are indexed by @@ -159,6 +160,11 @@ bool BlockFilterIndex::CustomCommit(CDBBatch& batch) } if (!file.Commit()) { LogError("%s: Failed to commit filter file %d\n", __func__, pos.nFile); + (void)file.fclose(); + return false; + } + if (file.fclose() != 0) { + LogError("Failed to close filter file %d after commit: %s", pos.nFile, SysErrorString(errno)); return false; } @@ -213,6 +219,11 @@ size_t BlockFilterIndex::WriteFilterToDisk(FlatFilePos& pos, const BlockFilter& } if (!last_file.Commit()) { LogPrintf("%s: Failed to commit filter file %d\n", __func__, pos.nFile); + (void)last_file.fclose(); + return 0; + } + if (last_file.fclose() != 0) { + LogError("Failed to close filter file %d after commit: %s", pos.nFile, SysErrorString(errno)); return 0; } @@ -235,6 +246,12 @@ size_t BlockFilterIndex::WriteFilterToDisk(FlatFilePos& pos, const BlockFilter& } fileout << filter.GetBlockHash() << filter.GetEncodedFilter(); + + if (fileout.fclose() != 0) { + LogError("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 217d9a89037..490b766cb1d 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -4005,6 +4005,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::function #include #include +#include #include #include @@ -941,13 +942,13 @@ bool BlockManager::WriteBlockUndo(const CBlockUndo& blockundo, BlockValidationSt return false; } + // Open history file to append + AutoFile file{OpenUndoFile(pos)}; + if (file.IsNull()) { + LogError("OpenUndoFile failed for %s while writing block undo", pos.ToString()); + return FatalError(m_opts.notifications, state, _("Failed to write undo data.")); + } { - // Open history file to append - AutoFile file{OpenUndoFile(pos)}; - if (file.IsNull()) { - LogError("OpenUndoFile failed for %s while writing block undo", pos.ToString()); - return FatalError(m_opts.notifications, state, _("Failed to write undo data.")); - } BufferedWriter fileout{file}; // Write index header @@ -960,8 +961,13 @@ bool BlockManager::WriteBlockUndo(const CBlockUndo& blockundo, BlockValidationSt // Write undo data & checksum fileout << blockundo << hasher.GetHash(); } + // BufferedWriter will flush pending data to file when fileout goes out of scope. + } - fileout.flush(); // Make sure `AutoFile`/`BufferedWriter` go out of scope before we call `FlushUndoFile` + // Make sure that the file is closed before we call `FlushUndoFile`. + if (file.fclose() != 0) { + LogError("Failed to close block undo file %s: %s", pos.ToString(), SysErrorString(errno)); + return FatalError(m_opts.notifications, state, _("Failed to close block undo file.")); } // rev files are written in block height order, whereas blk files are written as blocks come in (often out of order) @@ -1094,13 +1100,22 @@ 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) { + LogError("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; } @@ -1143,6 +1158,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", + 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..d4d9263973e 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; } @@ -199,9 +201,14 @@ bool DumpMempool(const CTxMemPool& pool, const fs::path& dump_path, FopenFn mock LogInfo("Writing %d unbroadcast transactions to file.\n", unbroadcast_txids.size()); file << unbroadcast_txids; - if (!skip_file_commit && !file.Commit()) + if (!skip_file_commit && !file.Commit()) { + (void)file.fclose(); 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 +220,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 95fe2aff970..feb91662296 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include @@ -963,9 +964,14 @@ 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)); - } else { - LogPrintf("Flushed fee estimates to %s.\n", fs::PathToString(m_estimation_filepath.filename())); + (void)est_file.fclose(); + return; } + if (est_file.fclose() != 0) { + LogError("Failed to close fee estimates file %s: %s. Continuing anyway.", fs::PathToString(m_estimation_filepath), SysErrorString(errno)); + return; + } + LogPrintf("Flushed fee estimates to %s.\n", fs::PathToString(m_estimation_filepath.filename())); } bool CBlockPolicyEstimator::Write(AutoFile& fileout) const diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 241cc367105..be953cefdea 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -47,6 +47,7 @@ #include #include #include +#include #include #include #include @@ -3232,7 +3233,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/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..8d17624da66 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,7 @@ FUZZ_TARGET(autofile) if (f != nullptr) { fclose(f); } + } else { + (void)auto_file.fclose(); } } diff --git a/src/test/fuzz/policy_estimator.cpp b/src/test/fuzz/policy_estimator.cpp index cda7725a136..37e37964182 100644 --- a/src/test/fuzz/policy_estimator.cpp +++ b/src/test/fuzz/policy_estimator.cpp @@ -111,5 +111,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 02dc5fec867..36a1b5f89d8 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 16196b0e25f..0916c6258c7 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..9e8281d26a7 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,7 @@ 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(); + BOOST_REQUIRE_EQUAL(file.fclose(), 0); fs::remove(streams_test_filename); } @@ -430,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); } @@ -551,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); } @@ -566,7 +567,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 +626,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 @@ -671,12 +677,14 @@ BOOST_AUTO_TEST_CASE(buffered_writer_reader) const fs::path test_file{m_args.GetDataDirBase() / "test_buffered_write_read.bin"}; // Write out the values through a precisely sized BufferedWriter + AutoFile file{fsbridge::fopen(test_file, "w+b")}; { - AutoFile file{fsbridge::fopen(test_file, "w+b")}; BufferedWriter f(file, sizeof(v1) + sizeof(v2) + sizeof(v3)); f << v1 << v2; f.write(std::as_bytes(std::span{&v3, 1})); } + BOOST_REQUIRE_EQUAL(file.fclose(), 0); + // Read back and verify using BufferedReader { uint32_t _v1{0}, _v2{0}, _v3{0}; diff --git a/src/wallet/test/fuzz/wallet_bdb_parser.cpp b/src/wallet/test/fuzz/wallet_bdb_parser.cpp index ba644076077..f15548d4d6c 100644 --- a/src/wallet/test/fuzz/wallet_bdb_parser.cpp +++ b/src/wallet/test/fuzz/wallet_bdb_parser.cpp @@ -36,6 +36,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{}; From 4bb5dd78ea4b578922a3316b37b486f96cb0beec Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Fri, 2 May 2025 16:22:41 +0200 Subject: [PATCH 3/4] util: check that a file has been closed before ~AutoFile() is called If an `AutoFile` has been written to, then expect callers to have closed it explicitly via the `AutoFile::fclose()` method. This is because if the destructor calls `std::fclose()` and encounters an error, then it is too late to indicate this to the caller in a meaningful way. --- src/streams.cpp | 3 +++ src/streams.h | 32 +++++++++++++++++++++++++++++--- 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/src/streams.cpp b/src/streams.cpp index 19c2b474452..2c873e50d3b 100644 --- a/src/streams.cpp +++ b/src/streams.cpp @@ -85,6 +85,7 @@ void AutoFile::write(std::span src) if (std::fwrite(src.data(), 1, src.size(), m_file) != src.size()) { throw std::ios_base::failure("AutoFile::write: write failed"); } + m_was_written = true; if (m_position.has_value()) *m_position += src.size(); } else { std::array buf; @@ -107,6 +108,7 @@ void AutoFile::write_buffer(std::span src) if (std::fwrite(src.data(), 1, src.size(), m_file) != src.size()) { throw std::ios_base::failure("AutoFile::write_buffer: write failed"); } + m_was_written = true; if (m_position) *m_position += src.size(); } @@ -117,6 +119,7 @@ bool AutoFile::Commit() bool AutoFile::Truncate(unsigned size) { + m_was_written = true; return ::TruncateFile(m_file, size); } diff --git a/src/streams.h b/src/streams.h index 5a596047121..27d25220fbe 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,28 @@ 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 in the destructor we have no way to signal + // errors from fclose() which, after write, could mean the file is + // corrupted and must be handled properly at the call site. + // Destructors in C++ cannot signal an error to the callers because + // they do not return a value and are not allowed to throw exceptions. + Assume(IsNull()); + } + + if (fclose() != 0) { + LogError("Failed to close file: %s", SysErrorString(errno)); + } + } // Disallow copies AutoFile(const AutoFile&) = delete; @@ -406,7 +432,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; From c10e382d2a3b76b70ebb8a4eb5cd99fc9f14d702 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Thu, 22 May 2025 12:27:38 +0200 Subject: [PATCH 4/4] flatfile: check whether the file has been closed successfully --- src/flatfile.cpp | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/flatfile.cpp b/src/flatfile.cpp index 388b30efae6..df6596e9405 100644 --- a/src/flatfile.cpp +++ b/src/flatfile.cpp @@ -46,7 +46,9 @@ FILE* FlatFileSeq::Open(const FlatFilePos& pos, bool read_only) const } if (pos.nPos && fseek(file, pos.nPos, SEEK_SET)) { LogPrintf("Unable to seek to position %u of %s\n", pos.nPos, fs::PathToString(path)); - fclose(file); + if (fclose(file) != 0) { + LogError("Unable to close file %s", fs::PathToString(path)); + } return nullptr; } return file; @@ -68,7 +70,10 @@ size_t FlatFileSeq::Allocate(const FlatFilePos& pos, size_t add_size, bool& out_ if (file) { LogDebug(BCLog::VALIDATION, "Pre-allocating up to position 0x%x in %s%05u.dat\n", new_size, m_prefix, pos.nFile); AllocateFileRange(file, pos.nPos, inc_size); - fclose(file); + if (fclose(file) != 0) { + LogError("Cannot close file %s%05u.dat after extending it with %u bytes", m_prefix, pos.nFile, new_size); + return 0; + } return inc_size; } } else { @@ -86,17 +91,24 @@ bool FlatFileSeq::Flush(const FlatFilePos& pos, bool finalize) const return false; } if (finalize && !TruncateFile(file, pos.nPos)) { - fclose(file); LogError("%s: failed to truncate file %d\n", __func__, pos.nFile); + if (fclose(file) != 0) { + LogError("Failed to close file %d", pos.nFile); + } return false; } if (!FileCommit(file)) { - fclose(file); LogError("%s: failed to commit file %d\n", __func__, pos.nFile); + if (fclose(file) != 0) { + LogError("Failed to close file %d", pos.nFile); + } return false; } DirectoryCommit(m_dir); - fclose(file); + if (fclose(file) != 0) { + LogError("Failed to close file %d after flush", pos.nFile); + return false; + } return true; }