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{};