util: check that a file has been closed before ~AutoFile() is called

If an `AutoFile` has been written to, then expect callers to have closed
it explicitly via the `AutoFile::fclose()` method. This is because if
the destructor calls `std::fclose()` and encounters an error, then it
is too late to indicate this to the caller in a meaningful way.

Author: vasild

src/streams.cpp

@@ -85,6 +85,7 @@ void AutoFile::write(std::span<const std::byte> src)
         if (std::fwrite(src.data(), 1, src.size(), m_file) != src.size()) {
             throw std::ios_base::failure("AutoFile::write: write failed");
         }
+        m_was_written = true;
         if (m_position.has_value()) *m_position += src.size();
     } else {
         std::array<std::byte, 4096> buf;
@@ -107,6 +108,7 @@ void AutoFile::write_buffer(std::span<std::byte> src)
     if (std::fwrite(src.data(), 1, src.size(), m_file) != src.size()) {
         throw std::ios_base::failure("AutoFile::write_buffer: write failed");
     }
+    m_was_written = true;
     if (m_position) *m_position += src.size();
 }
 
@@ -117,6 +119,7 @@ bool AutoFile::Commit()
 
 bool AutoFile::Truncate(unsigned size)
 {
+    m_was_written = true;
     return ::TruncateFile(m_file, size);
 }
 

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 <cassert>
@@ -386,27 +389,50 @@ class BitStreamWriter
  *
  * 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
 {
 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 in the destructor we have no way to signal
+            // errors from fclose() which, after write, could mean the file is
+            // corrupted and must be handled properly at the call site.
+            // Destructors in C++ cannot signal an error to the callers because
+            // they do not return a value and are not allowed to throw exceptions.
+            Assume(IsNull());
+        }
+
+        if (fclose() != 0) {
+            LogError("Failed to close file: %s", SysErrorString(errno));
+        }
+    }
 
     // Disallow copies
     AutoFile(const AutoFile&) = delete;
     AutoFile& operator=(const AutoFile&) = delete;
 
     bool feof() const { return std::feof(m_file); }
 
-    int fclose()
+    [[nodiscard]] int fclose()
     {
         if (auto rel{release()}) return std::fclose(rel);
         return 0;