Merge dashpay#6398: backport: 24306, 24312, 24371

ba7e500 refactor: more of 24306 (GetPathArg in Dash-specific code) (UdjinM6)
562e3f7 Merge bitcoin#24371: util: Fix ReadBinaryFile reading beyond maxsize (MarcoFalke)
3383b79 Merge bitcoin#24312: addrman: Log too low compat value (MarcoFalke)
d96983a Merge bitcoin#24306: util: Make ArgsManager::GetPathArg more widely usable (MarcoFalke)

Pull request description:

  ## Issue being fixed or feature implemented
  A few simple backports

  ## What was done?

  ## How Has This Been Tested?
  Built locally; see CI

  ## Breaking Changes
  None

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK ba7e500
  kwvg:
    utACK ba7e500

Tree-SHA512: 42f41cb04945af15bfa4581ae53354eec88d4cdec44ab7c0100edcfe2fa71a95e05073e76b8d3935bd05cd5ee1f5cd5b34d2ed99e7eb551475abdc77f1ffff7c

src/addrman.cpp

@@ -244,12 +244,18 @@ void AddrManImpl::Unserialize(Stream& s_)
 
     uint8_t compat;
     s >> compat;
+    if (compat < INCOMPATIBILITY_BASE) {
+        throw std::ios_base::failure(strprintf(
+            "Corrupted addrman database: The compat value (%u) "
+            "is lower than the expected minimum value %u.",
+            compat, INCOMPATIBILITY_BASE));
+    }
     const uint8_t lowest_compatible = compat - INCOMPATIBILITY_BASE;
     if (lowest_compatible > FILE_FORMAT) {
         throw InvalidAddrManVersionError(strprintf(
             "Unsupported format of addrman database: %u. It is compatible with formats >=%u, "
             "but the maximum supported by this version of %s is %u.",
-            uint8_t{format}, uint8_t{lowest_compatible}, PACKAGE_NAME, uint8_t{FILE_FORMAT}));
+            uint8_t{format}, lowest_compatible, PACKAGE_NAME, uint8_t{FILE_FORMAT}));
     }
 
     s >> nKey;

src/bench/bench_bitcoin.cpp

@@ -111,8 +111,8 @@ int main(int argc, char** argv)
     args.asymptote = parseAsymptote(argsman.GetArg("-asymptote", ""));
     args.is_list_only = argsman.GetBoolArg("-list", false);
     args.min_time = std::chrono::milliseconds(argsman.GetArg("-min_time", DEFAULT_MIN_TIME_MS));
-    args.output_csv = fs::PathFromString(argsman.GetArg("-output_csv", ""));
-    args.output_json = fs::PathFromString(argsman.GetArg("-output_json", ""));
+    args.output_csv = argsman.GetPathArg("-output_csv");
+    args.output_json = argsman.GetPathArg("-output_json");
     args.regex_filter = argsman.GetArg("-filter", DEFAULT_BENCH_FILTER);
 
     benchmark::BenchRunner::RunAll(args);

src/init.cpp

@@ -160,7 +160,7 @@ static const char* BITCOIN_PID_FILENAME = "dashd.pid";
 
 static fs::path GetPidFile(const ArgsManager& args)
 {
-    return AbsPathForConfigVal(fs::PathFromString(args.GetArg("-pid", BITCOIN_PID_FILENAME)));
+    return AbsPathForConfigVal(args.GetPathArg("-pid", BITCOIN_PID_FILENAME));
 }
 
 [[nodiscard]] static bool CreatePidFile(const ArgsManager& args)
@@ -1566,10 +1566,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         // Read asmap file if configured
         std::vector<bool> asmap;
         if (args.IsArgSet("-asmap")) {
-            fs::path asmap_path = fs::PathFromString(args.GetArg("-asmap", ""));
-            if (asmap_path.empty()) {
-                asmap_path = fs::PathFromString(DEFAULT_ASMAP_FILENAME);
-            }
+            fs::path asmap_path = args.GetPathArg("-asmap", DEFAULT_ASMAP_FILENAME);
             if (!asmap_path.is_absolute()) {
                 asmap_path = gArgs.GetDataDirNet() / asmap_path;
             }

src/init/common.cpp

@@ -77,7 +77,7 @@ void AddLoggingArgs(ArgsManager& argsman)
 void SetLoggingOptions(const ArgsManager& args)
 {
     LogInstance().m_print_to_file = !args.IsArgNegated("-debuglogfile");
-    LogInstance().m_file_path = AbsPathForConfigVal(fs::PathFromString(args.GetArg("-debuglogfile", DEFAULT_DEBUGLOGFILE)));
+    LogInstance().m_file_path = AbsPathForConfigVal(args.GetPathArg("-debuglogfile", DEFAULT_DEBUGLOGFILE));
     LogInstance().m_print_to_console = args.GetBoolArg("-printtoconsole", !args.GetBoolArg("-daemon", false));
     LogInstance().m_log_timestamps = args.GetBoolArg("-logtimestamps", DEFAULT_LOGTIMESTAMPS);
     LogInstance().m_log_time_micros = args.GetBoolArg("-logtimemicros", DEFAULT_LOGTIMEMICROS);

src/qt/bitcoin.cpp

@@ -680,7 +680,7 @@ int GuiMain(int argc, char* argv[])
     }
     // Validate/set custom css directory
     if (gArgs.IsArgSet("-custom-css-dir")) {
-        fs::path customDir = fs::PathFromString(gArgs.GetArg("-custom-css-dir", ""));
+        fs::path customDir = gArgs.GetPathArg("-custom-css-dir");
         QString strCustomDir = GUIUtil::PathToQString(customDir);
         std::vector<QString> vecRequiredFiles = GUIUtil::listStyleSheets();
         QString strFile;

src/test/getarg_tests.cpp

@@ -243,6 +243,24 @@ BOOST_AUTO_TEST_CASE(patharg)
 
     ResetArgs("-dir=user/.bitcoin/.//");
     BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), relative_path);
+
+    // Check negated and default argument handling. Specifying an empty argument
+    // is the same as not specifying the argument. This is convenient for
+    // scripting so later command line arguments can override earlier command
+    // line arguments or bitcoin.conf values. Currently the -dir= case cannot be
+    // distinguished from -dir case with no assignment, but #16545 would add the
+    // ability to distinguish these in the future (and treat the no-assign case
+    // like an imperative command or an error).
+    ResetArgs("");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir", "default"), fs::path{"default"});
+    ResetArgs("-dir=override");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir", "default"), fs::path{"override"});
+    ResetArgs("-dir=");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir", "default"), fs::path{"default"});
+    ResetArgs("-dir");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir", "default"), fs::path{"default"});
+    ResetArgs("-nodir");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir", "default"), fs::path{""});
 }
 
 BOOST_AUTO_TEST_CASE(doubledash)

src/test/util_tests.cpp

@@ -18,13 +18,15 @@
 #include <util/moneystr.h>
 #include <util/overflow.h>
 #include <util/ranges_set.h>
+#include <util/readwritefile.h>
 #include <util/spanparsing.h>
 #include <util/strencodings.h>
 #include <util/string.h>
 #include <util/time.h>
 #include <util/vector.h>
 
 #include <array>
+#include <fstream>
 #include <deque>
 #include <optional>
 #include <random>
@@ -2747,6 +2749,52 @@ 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/util/readwritefile.cpp

@@ -20,15 +20,15 @@ std::pair<bool,std::string> ReadBinaryFile(const fs::path &filename, size_t maxs
     std::string retval;
     char buffer[128];
     do {
-        const size_t n = fread(buffer, 1, sizeof(buffer), f);
+        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);
+    } while (!feof(f) && retval.size() < maxsize);
     fclose(f);
     return std::make_pair(true,retval);
 }

src/util/system.cpp

@@ -407,9 +407,12 @@ std::optional<unsigned int> ArgsManager::GetArgFlags(const std::string& name) co
     return std::nullopt;
 }
 
-fs::path ArgsManager::GetPathArg(std::string pathlike_arg) const
+fs::path ArgsManager::GetPathArg(std::string arg, const fs::path& default_value) const
 {
-    auto result = fs::PathFromString(GetArg(pathlike_arg, "")).lexically_normal();
+    if (IsArgNegated(arg)) return fs::path{};
+    std::string path_str = GetArg(arg, "");
+    if (path_str.empty()) return default_value;
+    fs::path result = fs::PathFromString(path_str).lexically_normal();
     // Remove trailing slash, if present.
     return result.has_filename() ? result : result.parent_path();
 }
@@ -478,7 +481,7 @@ fs::path ArgsManager::GetBackupsDirPath()
     if (!IsArgSet("-walletbackupsdir"))
         return GetDataDirNet() / "backups";
 
-    return fs::absolute(fs::PathFromString(GetArg("-walletbackupsdir", "")));
+    return fs::absolute(GetPathArg("-walletbackupsdir"));
 }
 
 void ArgsManager::ClearPathCache()
@@ -544,12 +547,12 @@ bool ArgsManager::InitSettings(std::string& error)
 
 bool ArgsManager::GetSettingsPath(fs::path* filepath, bool temp) const
 {
-    if (IsArgNegated("-settings")) {
+    fs::path settings = GetPathArg("-settings", fs::path{BITCOIN_SETTINGS_FILENAME});
+    if (settings.empty()) {
         return false;
     }
     if (filepath) {
-        std::string settings = GetArg("-settings", BITCOIN_SETTINGS_FILENAME);
-        *filepath = fsbridge::AbsPathJoin(GetDataDirNet(), fs::PathFromString(temp ? settings + ".tmp" : settings));
+        *filepath = fsbridge::AbsPathJoin(GetDataDirNet(), temp ? settings + ".tmp" : settings);
     }
     return true;
 }

src/util/system.h

@@ -283,16 +283,6 @@ class ArgsManager
      */
     const std::map<std::string, std::vector<util::SettingsValue>> GetCommandLineArgs() const;
 
-    /**
-     * Get a normalized path from a specified pathlike argument
-     *
-     * It is guaranteed that the returned path has no trailing slashes.
-     *
-     * @param pathlike_arg Pathlike argument to get a path from (e.g., "-datadir", "-blocksdir" or "-walletdir")
-     * @return Normalized path which is get from a specified pathlike argument
-     */
-    fs::path GetPathArg(std::string pathlike_arg) const;
-
     /**
      * Get blocks directory path
      *
@@ -357,6 +347,18 @@ class ArgsManager
      */
     std::string GetArg(const std::string& strArg, const std::string& strDefault) const;
 
+    /**
+     * Return path argument or default value
+     *
+     * @param arg Argument to get a path from (e.g., "-datadir", "-blocksdir" or "-walletdir")
+     * @param default_value Optional default value to return instead of the empty path.
+     * @return normalized path if argument is set, with redundant "." and ".."
+     * path components and trailing separators removed (see patharg unit test
+     * for examples or implementation for details). If argument is empty or not
+     * set, default_value is returned unchanged.
+     */
+    fs::path GetPathArg(std::string arg, const fs::path& default_value = {}) const;
+
     /**
      * Return integer argument or default value
      *

test/functional/feature_addrman.py

@@ -67,6 +67,17 @@ def run_test(self):
             self.start_node(0, extra_args=["-checkaddrman=1"])
         assert_equal(self.nodes[0].getnodeaddresses(), [])
 
+        self.log.info("Check that addrman with negative lowest_compatible cannot be read")
+        self.stop_node(0)
+        write_addrman(peers_dat, lowest_compatible=-32)
+        self.nodes[0].assert_start_raises_init_error(
+            expected_msg=init_error(
+                "Corrupted addrman database: The compat value \\(0\\) is lower "
+                "than the expected minimum value 32.: (.+)"
+            ),
+            match=ErrorMatch.FULL_REGEX,
+        )
+
         self.log.info("Check that addrman from future is overwritten with new addrman")
         self.stop_node(0)
         write_addrman(peers_dat, lowest_compatible=111)

test/sanitizer_suppressions/ubsan

@@ -78,7 +78,6 @@ implicit-integer-sign-change:util/strencodings.h
 implicit-integer-sign-change:validation.cpp
 implicit-signed-integer-truncation,implicit-integer-sign-change:chain.h
 implicit-signed-integer-truncation,implicit-integer-sign-change:test/skiplist_tests.cpp
-implicit-signed-integer-truncation:addrman.cpp
 implicit-signed-integer-truncation:addrman.h
 implicit-signed-integer-truncation:chain.h
 implicit-signed-integer-truncation:crypto/