util: check for error from fclose(3) in explicit calls

Explicit callers of `AutoFile::fclose()` should check whether
`fclose(3)` failed because this may be due to a failure to
flush written data to disk. Such failures after writing to
a file should be treated as failures to `fwrite(3)`.

Github-Pull: #29307
Rebased-From: e9abf95ab52f1f2d6f73ecca63133c29c253b7d9

src/addrdb.cpp

@@ -22,6 +22,7 @@
 #include <univalue.h>
 #include <util/fs.h>
 #include <util/fs_helpers.h>
+#include <util/syserror.h>
 #include <util/translation.h>
 
 namespace {
@@ -58,23 +59,25 @@ 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);
         return error("%s: Failed to open file %s", __func__, fs::PathToString(pathTmp));
     }
 
     // Serialize
     if (!SerializeDB(fileout, data)) {
-        fileout.fclose();
+        (void)fileout.fclose();
         remove(pathTmp);
         return false;
     }
     if (!FileCommit(fileout.Get())) {
-        fileout.fclose();
+        (void)fileout.fclose();
         remove(pathTmp);
         return error("%s: Failed to flush file %s", __func__, fs::PathToString(pathTmp));
     }
-    fileout.fclose();
+    if (fileout.fclose() != 0) {
+        remove(pathTmp);
+        return error("%s: Failed to close file %s: %s", __func__, fs::PathToString(pathTmp), SysErrorString(errno));
+    }
 
     // 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();
+    (void)file.fclose();
     fs::remove("streams_tmp");
 }
 

src/kernel/mempool_persist.cpp

@@ -16,6 +16,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>
 
@@ -152,7 +153,8 @@ bool DumpMempool(const CTxMemPool& pool, const fs::path& dump_path, FopenFn mock
     auto mid = SteadyClock::now();
 
     try {
-        FILE* filestr{mockable_fopen_function(dump_path + ".new", "wb")};
+        const fs::path file_fspath{dump_path + ".new"};
+        FILE* filestr{mockable_fopen_function(file_fspath, "wb")};
         if (!filestr) {
             return false;
         }
@@ -177,7 +179,10 @@ 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) {
+            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");
         }

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>
@@ -2686,7 +2687,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.h

@@ -476,7 +476,12 @@ protected:
 public:
     explicit AutoFile(std::FILE* file, std::vector<std::byte> data_xor={}) : m_file{file}, m_xor{std::move(data_xor)} {}
 
-    ~AutoFile() { fclose(); }
+    ~AutoFile()
+    {
+        (void)fclose(); /* XXX ignoring the result of fclose() may hide failures to store data on disk via
+                           fflush(3). Can't throw from the destructor because this may cause an "exception
+                           within exception". */
+    }
 
     // Disallow copies
     AutoFile(const AutoFile&) = delete;
@@ -484,7 +489,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/fuzz/autofile.cpp

@@ -43,7 +43,7 @@ FUZZ_TARGET(autofile)
                 }
             },
             [&] {
-                auto_file.fclose();
+                (void)auto_file.fclose();
             },
             [&] {
                 ReadFromStream(fuzzed_data_provider, auto_file);

src/test/streams_tests.cpp

@@ -375,7 +375,7 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file)
     BOOST_CHECK(bf.GetPos() <= 30U);
 
     // We can explicitly close the file, or the destructor will do it.
-    file.fclose();
+    (void)file.fclose();
 
     fs::remove(streams_test_filename);
 }
@@ -425,7 +425,7 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file_skip)
     bf.SkipTo(13);
     BOOST_CHECK_EQUAL(bf.GetPos(), 13U);
 
-    file.fclose();
+    (void)file.fclose();
     fs::remove(streams_test_filename);
 }