Make (Read/Write)BinaryFile work with char vector, use AutoFile

src/Makefile.test.include

@@ -126,6 +126,7 @@ BITCOIN_TESTS =\
   test/raii_event_tests.cpp \
   test/random_tests.cpp \
   test/rbf_tests.cpp \
+  test/readwritefile_tests.cpp \
   test/rest_tests.cpp \
   test/result_tests.cpp \
   test/reverselock_tests.cpp \

src/i2p.cpp

@@ -434,9 +434,9 @@ void Session::CreateIfNotCreatedAlready()
     } else {
         // Read our persistent destination (private key) from disk or generate
         // one and save it to disk. Then use it when creating the session.
-        const auto& [read_ok, data] = ReadBinaryFile(m_private_key_file);
-        if (read_ok) {
-            m_private_key.assign(data.begin(), data.end());
+        std::optional<std::string> data{ReadBinaryFile<std::string>(m_private_key_file)};
+        if (data) {
+            m_private_key.assign(data->begin(), data->end());
         } else {
             GenerateAndSavePrivateKey(*sock);
         }

src/test/readwritefile_tests.cpp

@@ -0,0 +1,86 @@
+// Copyright (c) 2024 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <common/args.h>
+#include <test/util/setup_common.h>
+#include <util/readwritefile.h>
+
+#include <fstream>
+#include <optional>
+#include <stdint.h>
+#include <string.h>
+#include <vector>
+
+#include <boost/test/unit_test.hpp>
+
+BOOST_FIXTURE_TEST_SUITE(readwritefile_tests, BasicTestingSetup)
+
+BOOST_AUTO_TEST_CASE(util_ReadBinaryFile)
+{
+    fs::path tmpfolder = m_args.GetDataDirBase();
+    fs::path tmpfile = tmpfolder / "read_binary.dat";
+    std::string expected_text;
+    for (int i = 0; i < 30; i++) {
+        expected_text += "0123456789";
+    }
+    {
+        std::ofstream file{tmpfile};
+        file << expected_text;
+    }
+    {
+        // read all contents in file
+        auto text{ReadBinaryFile<std::string>(tmpfile)};
+        BOOST_REQUIRE(text);
+        BOOST_CHECK_EQUAL(text.value(), expected_text);
+    }
+    {
+        // read half contents in file
+        auto text{ReadBinaryFile<std::string>(tmpfile, expected_text.size() / 2)};
+        BOOST_REQUIRE(text);
+        BOOST_CHECK_EQUAL(text.value(), expected_text.substr(0, expected_text.size() / 2));
+    }
+    {
+        // read from non-existent file
+        fs::path invalid_file = tmpfolder / "invalid_binary.dat";
+        auto text{ReadBinaryFile<std::string>(invalid_file)};
+        BOOST_REQUIRE(!text);
+    }
+    {
+        std::vector<unsigned char> expected_data;
+        for (int i = 0; i < 30; i++) {
+            // store result as bytes (ASCII "0" is character 48)
+            expected_data.insert(expected_data.end(), {48,49,50,51,52,53,54,55,56,57});
+        }
+        auto data{ReadBinaryFile<std::vector<unsigned char>>(tmpfile)};
+        BOOST_REQUIRE(data);
+        BOOST_REQUIRE(data.value() == expected_data);
+    }
+}
+
+BOOST_AUTO_TEST_CASE(util_WriteBinaryFile)
+{
+    fs::path tmpfolder = m_args.GetDataDirBase();
+    fs::path tmpfile = tmpfolder / "write_binary.dat";
+    std::string expected_text = "bitcoin";
+    {
+        auto valid = WriteBinaryFile(tmpfile, expected_text);
+        BOOST_CHECK(valid);
+        std::string actual_text;
+        std::ifstream file{tmpfile};
+        file >> actual_text;
+        BOOST_CHECK_EQUAL(actual_text, expected_text);
+    }
+    {
+        std::vector<unsigned char> bytes{98, 105, 116, 99, 111, 105, 110}; // "bitcoin"
+        auto valid = WriteBinaryFile(tmpfile, bytes);
+        BOOST_CHECK(valid);
+        std::string actual_text;
+        std::ifstream file{tmpfile};
+        file >> actual_text;
+        BOOST_CHECK_EQUAL(actual_text, expected_text);
+    }
+
+}
+
+BOOST_AUTO_TEST_SUITE_END()

src/test/util_tests.cpp

@@ -15,7 +15,6 @@
 #include <util/message.h> // For MessageSign(), MessageVerify(), MESSAGE_MAGIC
 #include <util/moneystr.h>
 #include <util/overflow.h>
-#include <util/readwritefile.h>
 #include <util/spanparsing.h>
 #include <util/strencodings.h>
 #include <util/string.h>
@@ -1745,52 +1744,6 @@ BOOST_AUTO_TEST_CASE(util_ParseByteUnits)
     BOOST_CHECK(!ParseByteUnits("1x", noop));
 }
 
-BOOST_AUTO_TEST_CASE(util_ReadBinaryFile)
-{
-    fs::path tmpfolder = m_args.GetDataDirBase();
-    fs::path tmpfile = tmpfolder / "read_binary.dat";
-    std::string expected_text;
-    for (int i = 0; i < 30; i++) {
-        expected_text += "0123456789";
-    }
-    {
-        std::ofstream file{tmpfile};
-        file << expected_text;
-    }
-    {
-        // read all contents in file
-        auto [valid, text] = ReadBinaryFile(tmpfile);
-        BOOST_CHECK(valid);
-        BOOST_CHECK_EQUAL(text, expected_text);
-    }
-    {
-        // read half contents in file
-        auto [valid, text] = ReadBinaryFile(tmpfile, expected_text.size() / 2);
-        BOOST_CHECK(valid);
-        BOOST_CHECK_EQUAL(text, expected_text.substr(0, expected_text.size() / 2));
-    }
-    {
-        // read from non-existent file
-        fs::path invalid_file = tmpfolder / "invalid_binary.dat";
-        auto [valid, text] = ReadBinaryFile(invalid_file);
-        BOOST_CHECK(!valid);
-        BOOST_CHECK(text.empty());
-    }
-}
-
-BOOST_AUTO_TEST_CASE(util_WriteBinaryFile)
-{
-    fs::path tmpfolder = m_args.GetDataDirBase();
-    fs::path tmpfile = tmpfolder / "write_binary.dat";
-    std::string expected_text = "bitcoin";
-    auto valid = WriteBinaryFile(tmpfile, expected_text);
-    std::string actual_text;
-    std::ifstream file{tmpfile};
-    file >> actual_text;
-    BOOST_CHECK(valid);
-    BOOST_CHECK_EQUAL(actual_text, expected_text);
-}
-
 BOOST_AUTO_TEST_CASE(clearshrink_test)
 {
     {

src/torcontrol.cpp

@@ -328,10 +328,10 @@ TorController::TorController(struct event_base* _base, const std::string& tor_co
         LogPrintf("tor: Initiating connection to Tor control port %s failed\n", m_tor_control_center);
     }
     // Read service private key if cached
-    std::pair<bool,std::string> pkf = ReadBinaryFile(GetPrivateKeyFile());
-    if (pkf.first) {
+    auto data{ReadBinaryFile<std::string>(GetPrivateKeyFile())};
+    if (data) {
         LogPrint(BCLog::TOR, "Reading cached private key from %s\n", fs::PathToString(GetPrivateKeyFile()));
-        private_key = pkf.second;
+        private_key = data.value();
     }
 }
 
@@ -586,15 +586,15 @@ void TorController::protocolinfo_cb(TorControlConnection& _conn, const TorContro
         } else if (methods.count("SAFECOOKIE")) {
             // Cookie: hexdump -e '32/1 "%02x""\n"'  ~/.tor/control_auth_cookie
             LogPrint(BCLog::TOR, "Using SAFECOOKIE authentication, reading cookie authentication from %s\n", cookiefile);
-            std::pair<bool,std::string> status_cookie = ReadBinaryFile(fs::PathFromString(cookiefile), TOR_COOKIE_SIZE);
-            if (status_cookie.first && status_cookie.second.size() == TOR_COOKIE_SIZE) {
-                // _conn.Command("AUTHENTICATE " + HexStr(status_cookie.second), std::bind(&TorController::auth_cb, this, std::placeholders::_1, std::placeholders::_2));
-                cookie = std::vector<uint8_t>(status_cookie.second.begin(), status_cookie.second.end());
+            auto status_cookie = ReadBinaryFile<std::string>(fs::PathFromString(cookiefile), TOR_COOKIE_SIZE);
+            if (status_cookie && status_cookie->size() == TOR_COOKIE_SIZE) {
+                // _conn.Command("AUTHENTICATE " + HexStr(status_cookie), std::bind(&TorController::auth_cb, this, std::placeholders::_1, std::placeholders::_2));
+                cookie = std::vector<uint8_t>(status_cookie->begin(), status_cookie->end());
                 clientNonce = std::vector<uint8_t>(TOR_NONCE_SIZE, 0);
                 GetRandBytes(clientNonce);
                 _conn.Command("AUTHCHALLENGE SAFECOOKIE " + HexStr(clientNonce), std::bind(&TorController::authchallenge_cb, this, std::placeholders::_1, std::placeholders::_2));
             } else {
-                if (status_cookie.first) {
+                if (status_cookie) {
                     LogPrintf("tor: Authentication cookie %s is not exactly %i bytes, as is required by the spec\n", cookiefile, TOR_COOKIE_SIZE);
                 } else {
                     LogPrintf("tor: Authentication cookie %s could not be opened (check permissions)\n", cookiefile);

src/util/readwritefile.cpp

@@ -5,46 +5,45 @@
 
 #include <util/readwritefile.h>
 
+#include <streams.h>
 #include <util/fs.h>
 
 #include <algorithm>
 #include <cstdio>
 #include <limits>
 #include <string>
 #include <utility>
+#include <vector>
 
-std::pair<bool,std::string> ReadBinaryFile(const fs::path &filename, size_t maxsize)
+template <typename T>
+std::optional<T> ReadBinaryFile(const fs::path& filename, size_t maxsize)
 {
-    FILE *f = fsbridge::fopen(filename, "rb");
-    if (f == nullptr)
-        return std::make_pair(false,"");
-    std::string retval;
-    char buffer[128];
-    do {
-        const size_t n = fread(buffer, 1, std::min(sizeof(buffer), maxsize - retval.size()), f);
-        // Check for reading errors so we don't return any data if we couldn't
-        // read the entire file (or up to maxsize)
-        if (ferror(f)) {
-            fclose(f);
-            return std::make_pair(false,"");
-        }
-        retval.append(buffer, buffer+n);
-    } while (!feof(f) && retval.size() < maxsize);
-    fclose(f);
-    return std::make_pair(true,retval);
+    std::FILE *f = fsbridge::fopen(filename, "rb");
+    if (f == nullptr) return {};
+    T output{};
+    size_t file_size = fs::file_size(filename);
+    output.resize(std::min(file_size, maxsize));
+    try {
+        AutoFile{f} >> Span{output};
+    } catch (const std::ios_base::failure&) {
+        return {};
+    }
+    return output;
 }
 
-bool WriteBinaryFile(const fs::path &filename, const std::string &data)
+template std::optional<std::string> ReadBinaryFile(const fs::path &filename, size_t maxsize);
+template std::optional<std::vector<unsigned char>> ReadBinaryFile(const fs::path &filename, size_t maxsize);
+
+template <typename T>
+bool WriteBinaryFile(const fs::path& filename, const T& data)
 {
-    FILE *f = fsbridge::fopen(filename, "wb");
-    if (f == nullptr)
-        return false;
-    if (fwrite(data.data(), 1, data.size(), f) != data.size()) {
-        fclose(f);
-        return false;
-    }
-    if (fclose(f) != 0) {
+    try {
+        AutoFile{fsbridge::fopen(filename, "wb")} << Span{data};
+    } catch (const std::ios_base::failure&) {
         return false;
     }
     return true;
 }
+
+template bool WriteBinaryFile(const fs::path& filename, const std::string& data);
+template bool WriteBinaryFile(const fs::path& filename, const std::vector<unsigned char>& data);

src/util/readwritefile.h

@@ -8,21 +8,27 @@
 #include <util/fs.h>
 
 #include <limits>
+#include <optional>
 #include <string>
 #include <utility>
 
-/** Read full contents of a file and return them in a std::string.
- * Returns a pair <status, string>.
- * If an error occurred, status will be false, otherwise status will be true and the data will be returned in string.
+/**
+ * Read full contents of a file and return one of the following formats:
+ * 1. std::vector<unsigned char>
+ * 2. std::string
  *
- * @param maxsize Puts a maximum size limit on the file that is read. If the file is larger than this, truncated data
- *         (with len > maxsize) will be returned.
+ * @param[in] filename Filename. Returns false it doesn't exist.
+ * @param[in] maxsize  Puts a maximum size limit on the file that is read. If the file
+ *                 is larger than this, truncated data (with len > maxsize) will be returned.
+ * @return result if successful, std::nullopt otherwise
  */
-std::pair<bool,std::string> ReadBinaryFile(const fs::path &filename, size_t maxsize=std::numeric_limits<size_t>::max());
+template<class T>
+std::optional<T> ReadBinaryFile(const fs::path& filename, size_t maxsize=std::numeric_limits<size_t>::max());
 
-/** Write contents of std::string to a file.
+/** Write contents of std::string or std::vector<unsigned char> to a file.
  * @return true on success.
  */
-bool WriteBinaryFile(const fs::path &filename, const std::string &data);
+template <class T>
+bool WriteBinaryFile(const fs::path& filename, const T& data);
 
 #endif // BITCOIN_UTIL_READWRITEFILE_H