Merge 29307 via AutoFile_error_check-28

src/addrdb.cpp

@@ -24,6 +24,7 @@
 #include <univalue.h>
 #include <util/fs.h>
 #include <util/fs_helpers.h>
+#include <util/syserror.h>
 #include <util/translation.h>
 
 namespace {
@@ -79,7 +80,11 @@ bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data
         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)) {

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("%s: Failed to close filter file %d\n", __func__, pos.nFile);
+        return 0;
+    }
+
     return data_size;
 }
 

src/net.cpp

@@ -3947,6 +3947,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<void(const CAddress& addr,

src/node/blockstorage.cpp

@@ -697,6 +697,11 @@ bool BlockManager::UndoWriteToDisk(const CBlockUndo& blockundo, FlatFilePos& pos
     hasher << blockundo;
     fileout << hasher.GetHash();
 
+    if (fileout.fclose() != 0) {
+        LogError("%s: fclose failed\n", __func__);
+        return false;
+    }
+
     return true;
 }
 
@@ -989,6 +994,11 @@ bool BlockManager::WriteBlockToDisk(const CBlock& block, FlatFilePos& pos) const
     pos.nPos = (unsigned int)fileOutPos;
     fileout << TX_WITH_WITNESS(block);
 
+    if (fileout.fclose() != 0) {
+        LogError("WriteBlockToDisk: fclose failed\n");
+        return false;
+    }
+
     return true;
 }
 
@@ -1172,6 +1182,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){}) {

src/node/mempool_persist.cpp

@@ -17,6 +17,7 @@
 #include <util/fs.h>
 #include <util/fs_helpers.h>
 #include <util/signalinterrupt.h>
+#include <util/syserror.h>
 #include <util/time.h>
 #include <validation.h>
 
@@ -201,7 +202,11 @@ 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) {
+            const fs::path file_fspath{dump_path + ".new"};
+            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;

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()));

src/rpc/blockchain.cpp

@@ -46,6 +46,7 @@
 #include <util/check.h>
 #include <util/fs.h>
 #include <util/strencodings.h>
+#include <util/syserror.h>
 #include <util/translation.h>
 #include <validation.h>
 #include <validationinterface.h>
@@ -2803,7 +2804,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);

src/streams.cpp

@@ -99,6 +99,7 @@ void AutoFile::write(Span<const std::byte> src)
             *m_position += buf_now.size();
         }
     }
+    m_was_written = true;
 }
 
 bool AutoFile::Commit()

src/streams.h

@@ -6,11 +6,14 @@
 #ifndef BITCOIN_STREAMS_H
 #define BITCOIN_STREAMS_H
 
+#include <logging.h>
 #include <serialize.h>
 #include <span.h>
 #include <support/allocators/zeroafterfree.h>
+#include <util/check.h>
 #include <util/fs_helpers.h>
 #include <util/overflow.h>
+#include <util/syserror.h>
 
 #include <algorithm>
 #include <assert.h>
@@ -392,11 +395,26 @@ protected:
     std::FILE* m_file;
     std::vector<std::byte> m_xor;
     std::optional<int64_t> m_position;
+    bool m_was_written{false};
 
 public:
     explicit AutoFile(std::FILE* file, std::vector<std::byte> 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\n", SysErrorString(errno));
+        }
+    }
 
     // Disallow copies
     AutoFile(const AutoFile&) = delete;

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

src/test/fuzz/autofile.cpp

@@ -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();
     }
 }

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();
     }
 }

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();
 }

src/test/fuzz/utxo_snapshot.cpp

@@ -118,6 +118,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{[&] {

src/test/streams_tests.cpp

@@ -37,6 +37,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
@@ -377,8 +378,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);
 }
@@ -428,7 +429,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);
 }
 
@@ -549,6 +550,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);
 }

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