refactor: Drop unused fclose() from BufferedFile

This was only explicitly used in the tests, where it can be replaced by
wrapping the original raw file pointer into a CAutoFile on creation and
then calling CAutoFile::fclose().

Also, it was used in LoadExternalBlockFile(), where it can also be
replaced by the (implicit call to the) CAutoFile destructor after
wrapping the original raw file pointer in a CAutoFile.

Author: MarcoFalke

src/bench/load_external.cpp

@@ -5,6 +5,7 @@
 #include <bench/bench.h>
 #include <bench/data.h>
 #include <chainparams.h>
+#include <clientversion.h>
 #include <test/util/setup_common.h>
 #include <util/chaintype.h>
 #include <validation.h>
@@ -54,8 +55,8 @@ static void LoadExternalBlockFile(benchmark::Bench& bench)
     bench.run([&] {
         // "rb" is "binary, O_RDONLY", positioned to the start of the file.
         // The file will be closed by LoadExternalBlockFile().
-        FILE* file{fsbridge::fopen(blkfile, "rb")};
-        testing_setup->m_node.chainman->LoadExternalBlockFile(file, &pos, &blocks_with_unknown_parent);
+        CAutoFile file{fsbridge::fopen(blkfile, "rb"), CLIENT_VERSION};
+        testing_setup->m_node.chainman->LoadExternalBlockFile(file.Get(), &pos, &blocks_with_unknown_parent);
     });
     fs::remove(blkfile);
 }

src/bench/streams_findbyte.cpp

@@ -4,27 +4,31 @@
 
 #include <bench/bench.h>
 
-#include <util/fs.h>
 #include <streams.h>
+#include <util/fs.h>
+
+#include <cstddef>
+#include <cstdint>
+#include <cstdio>
 
 static void FindByte(benchmark::Bench& bench)
 {
     // Setup
-    FILE* file = fsbridge::fopen("streams_tmp", "w+b");
+    CAutoFile file{fsbridge::fopen("streams_tmp", "w+b"), 0};
     const size_t file_size = 200;
     uint8_t data[file_size] = {0};
     data[file_size-1] = 1;
-    fwrite(&data, sizeof(uint8_t), file_size, file);
-    rewind(file);
-    BufferedFile bf{file, /*nBufSize=*/file_size + 1, /*nRewindIn=*/file_size, 0};
+    file << data;
+    std::rewind(file.Get());
+    BufferedFile bf{file.Get(), /*nBufSize=*/file_size + 1, /*nRewindIn=*/file_size, 0};
 
     bench.run([&] {
         bf.SetPos(0);
         bf.FindByte(std::byte(1));
     });
 
     // Cleanup
-    bf.fclose();
+    file.fclose();
     fs::remove("streams_tmp");
 }
 

src/node/blockstorage.cpp

@@ -1015,12 +1015,12 @@ void ImportBlocks(ChainstateManager& chainman, std::vector<fs::path> vImportFile
                 if (!fs::exists(chainman.m_blockman.GetBlockPosFilename(pos))) {
                     break; // No block files left to reindex
                 }
-                FILE* file = chainman.m_blockman.OpenBlockFile(pos, true);
-                if (!file) {
+                CAutoFile file{chainman.m_blockman.OpenBlockFile(pos, true), CLIENT_VERSION};
+                if (file.IsNull()) {
                     break; // This error is logged in OpenBlockFile
                 }
                 LogPrintf("Reindexing block file blk%05u.dat...\n", (unsigned int)nFile);
-                chainman.LoadExternalBlockFile(file, &pos, &blocks_with_unknown_parent);
+                chainman.LoadExternalBlockFile(file.Get(), &pos, &blocks_with_unknown_parent);
                 if (chainman.m_interrupt) {
                     LogPrintf("Interrupt requested. Exit %s\n", __func__);
                     return;
@@ -1036,10 +1036,10 @@ void ImportBlocks(ChainstateManager& chainman, std::vector<fs::path> vImportFile
 
         // -loadblock=
         for (const fs::path& path : vImportFiles) {
-            FILE* file = fsbridge::fopen(path, "rb");
-            if (file) {
+            CAutoFile file{fsbridge::fopen(path, "rb"), CLIENT_VERSION};
+            if (!file.IsNull()) {
                 LogPrintf("Importing blocks file %s...\n", fs::PathToString(path));
-                chainman.LoadExternalBlockFile(file);
+                chainman.LoadExternalBlockFile(file.Get());
                 if (chainman.m_interrupt) {
                     LogPrintf("Interrupt requested. Exit %s\n", __func__);
                     return;

src/streams.h

@@ -637,25 +637,8 @@ class BufferedFile
         src = fileIn;
     }
 
-    ~BufferedFile()
-    {
-        fclose();
-    }
-
-    // Disallow copies
-    BufferedFile(const BufferedFile&) = delete;
-    BufferedFile& operator=(const BufferedFile&) = delete;
-
     int GetVersion() const { return nVersion; }
 
-    void fclose()
-    {
-        if (src) {
-            ::fclose(src);
-            src = nullptr;
-        }
-    }
-
     //! check whether we're at the end of the source file
     bool eof() const {
         return m_read_pos == nSrcPos && feof(src);

src/test/fuzz/buffered_file.cpp

@@ -19,15 +19,12 @@ FUZZ_TARGET(buffered_file)
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
     FuzzedFileProvider fuzzed_file_provider = ConsumeFile(fuzzed_data_provider);
     std::optional<BufferedFile> opt_buffered_file;
-    FILE* fuzzed_file = fuzzed_file_provider.open();
+    CAutoFile fuzzed_file{fuzzed_file_provider.open(), 0};
     try {
-        opt_buffered_file.emplace(fuzzed_file, fuzzed_data_provider.ConsumeIntegralInRange<uint64_t>(0, 4096), fuzzed_data_provider.ConsumeIntegralInRange<uint64_t>(0, 4096), fuzzed_data_provider.ConsumeIntegral<int>());
+        opt_buffered_file.emplace(fuzzed_file.Get(), fuzzed_data_provider.ConsumeIntegralInRange<uint64_t>(0, 4096), fuzzed_data_provider.ConsumeIntegralInRange<uint64_t>(0, 4096), fuzzed_data_provider.ConsumeIntegral<int>());
     } catch (const std::ios_base::failure&) {
-        if (fuzzed_file != nullptr) {
-            fclose(fuzzed_file);
-        }
     }
-    if (opt_buffered_file && fuzzed_file != nullptr) {
+    if (opt_buffered_file && !fuzzed_file.IsNull()) {
         bool setpos_fail = false;
         LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000) {
             CallOneOf(

src/test/fuzz/load_external_block_file.cpp

@@ -3,6 +3,7 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <chainparams.h>
+#include <clientversion.h>
 #include <flatfile.h>
 #include <test/fuzz/FuzzedDataProvider.h>
 #include <test/fuzz/fuzz.h>
@@ -27,17 +28,17 @@ FUZZ_TARGET(load_external_block_file, .init = initialize_load_external_block_fil
 {
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
     FuzzedFileProvider fuzzed_file_provider = ConsumeFile(fuzzed_data_provider);
-    FILE* fuzzed_block_file = fuzzed_file_provider.open();
-    if (fuzzed_block_file == nullptr) {
+    CAutoFile fuzzed_block_file{fuzzed_file_provider.open(), CLIENT_VERSION};
+    if (fuzzed_block_file.IsNull()) {
         return;
     }
     if (fuzzed_data_provider.ConsumeBool()) {
         // Corresponds to the -reindex case (track orphan blocks across files).
         FlatFilePos flat_file_pos;
         std::multimap<uint256, FlatFilePos> blocks_with_unknown_parent;
-        g_setup->m_node.chainman->LoadExternalBlockFile(fuzzed_block_file, &flat_file_pos, &blocks_with_unknown_parent);
+        g_setup->m_node.chainman->LoadExternalBlockFile(fuzzed_block_file.Get(), &flat_file_pos, &blocks_with_unknown_parent);
     } else {
         // Corresponds to the -loadblock= case (orphan blocks aren't tracked across files).
-        g_setup->m_node.chainman->LoadExternalBlockFile(fuzzed_block_file);
+        g_setup->m_node.chainman->LoadExternalBlockFile(fuzzed_block_file.Get());
     }
 }

src/test/streams_tests.cpp

@@ -249,26 +249,26 @@ BOOST_AUTO_TEST_CASE(streams_serializedata_xor)
 BOOST_AUTO_TEST_CASE(streams_buffered_file)
 {
     fs::path streams_test_filename = m_args.GetDataDirBase() / "streams_test_tmp";
-    FILE* file = fsbridge::fopen(streams_test_filename, "w+b");
+    CAutoFile file{fsbridge::fopen(streams_test_filename, "w+b"), 333};
 
     // The value at each offset is the offset.
     for (uint8_t j = 0; j < 40; ++j) {
-        fwrite(&j, 1, 1, file);
+        file << j;
     }
-    rewind(file);
+    std::rewind(file.Get());
 
     // The buffer size (second arg) must be greater than the rewind
     // amount (third arg).
     try {
-        BufferedFile bfbad{file, 25, 25, 333};
+        BufferedFile bfbad{file.Get(), 25, 25, 333};
         BOOST_CHECK(false);
     } catch (const std::exception& e) {
         BOOST_CHECK(strstr(e.what(),
                         "Rewind limit must be less than buffer size") != nullptr);
     }
 
     // The buffer is 25 bytes, allow rewinding 10 bytes.
-    BufferedFile bf{file, 25, 10, 333};
+    BufferedFile bf{file.Get(), 25, 10, 333};
     BOOST_CHECK(!bf.eof());
 
     // This member has no functional effect.
@@ -375,23 +375,23 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file)
     BOOST_CHECK(bf.GetPos() <= 30U);
 
     // We can explicitly close the file, or the destructor will do it.
-    bf.fclose();
+    file.fclose();
 
     fs::remove(streams_test_filename);
 }
 
 BOOST_AUTO_TEST_CASE(streams_buffered_file_skip)
 {
     fs::path streams_test_filename = m_args.GetDataDirBase() / "streams_test_tmp";
-    FILE* file = fsbridge::fopen(streams_test_filename, "w+b");
+    CAutoFile file{fsbridge::fopen(streams_test_filename, "w+b"), 333};
     // The value at each offset is the byte offset (e.g. byte 1 in the file has the value 0x01).
     for (uint8_t j = 0; j < 40; ++j) {
-        fwrite(&j, 1, 1, file);
+        file << j;
     }
-    rewind(file);
+    std::rewind(file.Get());
 
     // The buffer is 25 bytes, allow rewinding 10 bytes.
-    BufferedFile bf{file, 25, 10, 333};
+    BufferedFile bf{file.Get(), 25, 10, 333};
 
     uint8_t i;
     // This is like bf >> (7-byte-variable), in that it will cause data
@@ -425,7 +425,7 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file_skip)
     bf.SkipTo(13);
     BOOST_CHECK_EQUAL(bf.GetPos(), 13U);
 
-    bf.fclose();
+    file.fclose();
     fs::remove(streams_test_filename);
 }
 
@@ -436,16 +436,16 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file_rand)
 
     fs::path streams_test_filename = m_args.GetDataDirBase() / "streams_test_tmp";
     for (int rep = 0; rep < 50; ++rep) {
-        FILE* file = fsbridge::fopen(streams_test_filename, "w+b");
+        CAutoFile file{fsbridge::fopen(streams_test_filename, "w+b"), 333};
         size_t fileSize = InsecureRandRange(256);
         for (uint8_t i = 0; i < fileSize; ++i) {
-            fwrite(&i, 1, 1, file);
+            file << i;
         }
-        rewind(file);
+        std::rewind(file.Get());
 
         size_t bufSize = InsecureRandRange(300) + 1;
         size_t rewindSize = InsecureRandRange(bufSize);
-        BufferedFile bf{file, bufSize, rewindSize, 333};
+        BufferedFile bf{file.Get(), bufSize, rewindSize, 333};
         size_t currentPos = 0;
         size_t maxPos = 0;
         for (int step = 0; step < 100; ++step) {

src/validation.cpp

@@ -4589,7 +4589,6 @@ void ChainstateManager::LoadExternalBlockFile(
 
     int nLoaded = 0;
     try {
-        // This takes over fileIn and calls fclose() on it in the BufferedFile destructor
         BufferedFile blkdat{fileIn, 2 * MAX_BLOCK_SERIALIZED_SIZE, MAX_BLOCK_SERIALIZED_SIZE + 8, CLIENT_VERSION};
         // nRewind indicates where to resume scanning in case something goes wrong,
         // such as a block fails to deserialize.