diff --git a/src/addrman.cpp b/src/addrman.cpp index 835b9aa6fe463..52d36cd1459a3 100644 --- a/src/addrman.cpp +++ b/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; diff --git a/src/bench/bench_bitcoin.cpp b/src/bench/bench_bitcoin.cpp index b12707dbf19e8..aded08c8a5735 100644 --- a/src/bench/bench_bitcoin.cpp +++ b/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); diff --git a/src/init.cpp b/src/init.cpp index efb29eae5dd0d..618cf3a4788a0 100644 --- a/src/init.cpp +++ b/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 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; } diff --git a/src/init/common.cpp b/src/init/common.cpp index 8be70f7cafeca..366f1f91181ae 100644 --- a/src/init/common.cpp +++ b/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); diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp index d550e13136ac9..debab9e2a24f1 100644 --- a/src/qt/bitcoin.cpp +++ b/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 vecRequiredFiles = GUIUtil::listStyleSheets(); QString strFile; diff --git a/src/test/getarg_tests.cpp b/src/test/getarg_tests.cpp index 7d7687e9090df..a2c3e1876a81b 100644 --- a/src/test/getarg_tests.cpp +++ b/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) diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index 68b6d1519af3a..ea9c73517846b 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -25,6 +26,7 @@ #include #include +#include #include #include #include @@ -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) { { diff --git a/src/util/readwritefile.cpp b/src/util/readwritefile.cpp index 2493028f95430..7bf0c7dd8a072 100644 --- a/src/util/readwritefile.cpp +++ b/src/util/readwritefile.cpp @@ -20,7 +20,7 @@ std::pair 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)) { @@ -28,7 +28,7 @@ std::pair ReadBinaryFile(const fs::path &filename, size_t maxs 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); } diff --git a/src/util/system.cpp b/src/util/system.cpp index 6834189aa6b96..a0261103c42a3 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -407,9 +407,12 @@ std::optional 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; } diff --git a/src/util/system.h b/src/util/system.h index b99a5f9a1e8c4..624306989451b 100644 --- a/src/util/system.h +++ b/src/util/system.h @@ -283,16 +283,6 @@ class ArgsManager */ const std::map> 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 * diff --git a/test/functional/feature_addrman.py b/test/functional/feature_addrman.py index 2d00e269665f5..51203f8d49106 100755 --- a/test/functional/feature_addrman.py +++ b/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) diff --git a/test/sanitizer_suppressions/ubsan b/test/sanitizer_suppressions/ubsan index c5fb0d34f490a..f0828c01cd0a5 100644 --- a/test/sanitizer_suppressions/ubsan +++ b/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/