Merge 29307 via AutoFile_error_check-27

src/addrdb.cpp

@@ -26,6 +26,7 @@
 #include <univalue.h>
 #include <util/fs.h>
 #include <util/fs_helpers.h>
+#include <util/syserror.h>
 #include <util/translation.h>
 
 namespace {
@@ -78,7 +79,11 @@ bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data
         remove(pathTmp);
         return error("%s: Failed to flush file %s", __func__, fs::PathToString(pathTmp));
     }
-    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

@@ -139,7 +139,7 @@ bool BlockFilterIndex::CustomCommit(CDBBatch& batch)
     if (file.IsNull()) {
         return error("%s: Failed to open filter file %d", __func__, pos.nFile);
     }
-    if (!FileCommit(file.Get())) {
+    if (!FileCommit(file.Get()) || file.fclose() != 0) {
         return error("%s: Failed to commit filter file %d", __func__, pos.nFile);
     }
 
@@ -188,7 +188,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;
         }
@@ -212,6 +212,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/kernel/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>
 
@@ -199,7 +200,11 @@ 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) {
+            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");
         }
@@ -210,6 +215,7 @@ bool DumpMempool(const CTxMemPool& pool, const fs::path& dump_path, FopenFn mock
                   Ticks<SecondsDouble>(last - mid));
     } catch (const std::exception& e) {
         LogPrintf("Failed to dump mempool: %s. Continuing anyway.\n", e.what());
+        (void)file.fclose();
         return false;
     }
     return true;

src/net.cpp

@@ -3890,6 +3890,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

@@ -692,6 +692,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;
 }
 
@@ -981,6 +986,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;
 }
 

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

@@ -42,6 +42,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>
@@ -2687,7 +2688,10 @@ UniValue CreateUTXOSnapshot(
         pcursor->Next();
     }
 
-    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", maybe_stats->coins_count);

src/streams.cpp

@@ -63,4 +63,5 @@ void AutoFile::write(Span<const std::byte> src)
             current_pos += buf_now.size();
         }
     }
+    m_was_written = true;
 }

src/streams.h

@@ -9,6 +9,7 @@
 #include <serialize.h>
 #include <span.h>
 #include <support/allocators/zeroafterfree.h>
+#include <util/check.h>
 #include <util/fs_helpers.h>
 #include <util/overflow.h>
 
@@ -391,11 +392,24 @@ class AutoFile
 protected:
     std::FILE* m_file;
     std::vector<std::byte> m_xor;
+    bool m_was_written{false};
 
 public:
     explicit AutoFile(std::FILE* file, std::vector<std::byte> 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;

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

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

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

@@ -43,6 +43,7 @@ FUZZ_TARGET(utxo_snapshot, .init = initialize_chain)
         AutoFile outfile{fsbridge::fopen(snapshot_path, "wb")};
         const auto file_data{ConsumeRandomLengthByteVector(fuzzed_data_provider)};
         outfile << Span{file_data};
+        assert(outfile.fclose() == 0);
     }
 
     const auto ActivateFuzzedSnapshot{[&] {

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