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.

Author: vasild

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,25 +62,29 @@ 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;
     }
 
     // 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)) {

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

@@ -13,6 +13,7 @@
 #include <node/blockstorage.h>
 #include <undo.h>
 #include <util/fs_helpers.h>
+#include <util/syserror.h>
 
 /* 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;
 }
 

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

src/node/blockstorage.cpp

@@ -33,6 +33,7 @@
 #include <util/fs.h>
 #include <util/signalinterrupt.h>
 #include <util/strencodings.h>
+#include <util/syserror.h>
 #include <util/translation.h>
 #include <validation.h>
 
@@ -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){}) {

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

src/policy/fees.cpp

@@ -19,6 +19,7 @@
 #include <uint256.h>
 #include <util/fs.h>
 #include <util/serfloat.h>
+#include <util/syserror.h>
 #include <util/time.h>
 
 #include <algorithm>
@@ -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

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

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,13 +81,15 @@ 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.
     {
         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,7 @@ FUZZ_TARGET(autofile)
         if (f != nullptr) {
             fclose(f);
         }
+    } else {
+        (void)auto_file.fclose();
     }
 }