From 8bb34f07df9ad45faf25c32c99a4dd70759b25be Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Wed, 24 Jan 2024 15:03:55 +0100 Subject: [PATCH] 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{};