Merge c52b673bf8 into c5e44a0435

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 {
@@ -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)) {

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

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

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::function<void(const CAddress& addr,

src/node/blockstorage.cpp

@@ -947,20 +947,30 @@ bool BlockManager::WriteBlockUndo(const CBlockUndo& blockundo, BlockValidationSt
                 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
-            fileout << GetParams().MessageStart() << blockundo_size;
-            pos.nPos += STORAGE_HEADER_BYTES;
             {
-                // Calculate checksum
+                BufferedWriter fileout{file};
-                HashWriter hasher{};
+
-                hasher << block.pprev->GetBlockHash() << blockundo;
+                // Write index header
-                // Write undo data & checksum
+                fileout << GetParams().MessageStart() << blockundo_size;
-                fileout << blockundo << hasher.GetHash();
+                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){}) {

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>
 
@@ -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;

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

src/rpc/blockchain.cpp

@@ -47,6 +47,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>
@@ -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);

src/streams.cpp

@@ -95,6 +95,7 @@ void AutoFile::write(std::span<const std::byte> src)
             src = src.subspan(buf_now.size());
         }
     }
+    m_was_written = true;
 }
 
 void AutoFile::write_buffer(std::span<std::byte> src)

src/streams.h

@@ -6,10 +6,13 @@
 #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/overflow.h>
+#include <util/syserror.h>
 
 #include <algorithm>
 #include <assert.h>
@@ -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<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", 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;

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

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

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

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

@@ -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{[&] {

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.
+    // Explicitly close the file and check that the close succeeds.
-    file.fclose();
+    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<std::byte>(file_size));
+        AutoFile f{test_file.Open(pos, /*read_only=*/false), obfuscation};
+        f.write(m_rng.randbytes<std::byte>(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;) {
+            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))};
+                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);
+                auto current_span = std::span{test_data}.subspan(total_written, write_size);
-            direct_file.write(current_span);
+                direct_file.write(current_span);
-            buffered.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

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

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