From d96983a3279f5427bbad2b168a84cd14db74dc73 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Mon, 7 Mar 2022 10:00:38 +0100 Subject: [PATCH 1/4] Merge bitcoin/bitcoin#24306: util: Make ArgsManager::GetPathArg more widely usable 60aa179d8f9a675efa2d78eaadc09e3ba450f50f Use GetPathArg where possible (Pavol Rusnak) 5b946edd73640c6ecdfb4cbac1d4351e634678dc util, refactor: Use GetPathArg to read "-settings" value (Ryan Ofsky) 687e655ae2970f2f13aca0267c7de86dc69be763 util: Add GetPathArg default path argument (Ryan Ofsky) Pull request description: Improve `ArgsManager::GetPathArg` method added in recent PR #24265, so it is usable more places. This PR starts to use it for the `-settings` option. This can also be helpful for #24274 which is parsing more path options. - Add `GetPathArg` default argument so it is less awkward to use to parse options that have default values. - Fix `GetPathArg` negated argument handling. Return path{} not path{"0"} when path argument is negated. - Add unit tests for default and negated cases - Move `GetPathArg` method declaration next to `GetArg` declaration. The two methods are close substitutes for each, so this should help keep them consistent and make them more discoverable. ACKs for top commit: w0xlt: Tested ACK 60aa179 on Ubuntu 21.10 hebasto: re-ACK 60aa179d8f9a675efa2d78eaadc09e3ba450f50f Tree-SHA512: 3d24b885d8bbeef39ea5d0556e2f09b9e5f4a21179cef11cbbbc1b84da29c8fb66ba698889054ce28d80bc25926687654c8532ed46054bf5b2dd1837866bd1cd --- src/bench/bench_bitcoin.cpp | 4 ++-- src/init.cpp | 7 ++----- src/init/common.cpp | 2 +- src/test/getarg_tests.cpp | 18 ++++++++++++++++++ src/util/system.cpp | 13 ++++++++----- src/util/system.h | 22 ++++++++++++---------- 6 files changed, 43 insertions(+), 23 deletions(-) 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/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/util/system.cpp b/src/util/system.cpp index 6834189aa6b96..1125a5f5637c1 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(); } @@ -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 * From 3383b790493cc9822d070dec84a4d05672f02915 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Tue, 8 Mar 2022 16:48:18 +0100 Subject: [PATCH 2/4] Merge bitcoin/bitcoin#24312: addrman: Log too low compat value fa097d074bc1afcc2a52976796bb618f7c6a68b3 addrman: Log too low compat value (MarcoFalke) Pull request description: Before this patch, when writing a negative `lowest_compatible` value, it would be read as a positive value. For example `-32` will be read as `224`. There is generally nothing wrong with that. Though, similarly there shouldn't be anything wrong with refusing to read a negative value. I find the code after this patch more logical than before. Also, this allows dropping a file-wide sanitizer suppression. In practice none of this should ever happen. Bitcoin Core would never write a negative `lowest_compatible` in normal operation, unless the file storage is later corrupted by external influence. ACKs for top commit: mzumsande: re-ACK fa097d074bc1afcc2a52976796bb618f7c6a68b3 Tree-SHA512: 9aae7b8fe666f52f667f149667025e0160cef1a793cc4d392e36608f65c2bee8096da429235118f40a3368f327aabe30f3732ae78c5874648ea6f423f2687b65 --- src/addrman.cpp | 8 +++++++- test/functional/feature_addrman.py | 11 +++++++++++ test/sanitizer_suppressions/ubsan | 1 - 3 files changed, 18 insertions(+), 2 deletions(-) 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/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/ From 562e3f7b18c7af9076ded6c1ad829aa180515fec Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Thu, 10 Mar 2022 10:08:43 +0100 Subject: [PATCH 3/4] Merge bitcoin/bitcoin#24371: util: Fix ReadBinaryFile reading beyond maxsize --- src/test/util_tests.cpp | 48 ++++++++++++++++++++++++++++++++++++++ src/util/readwritefile.cpp | 4 ++-- 2 files changed, 50 insertions(+), 2 deletions(-) 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); } From ba7e500062148455cb0157a47d4859984eba49f5 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Mon, 18 Nov 2024 13:24:51 +0300 Subject: [PATCH 4/4] refactor: more of 24306 (GetPathArg in Dash-specific code) --- src/qt/bitcoin.cpp | 2 +- src/util/system.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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/util/system.cpp b/src/util/system.cpp index 1125a5f5637c1..a0261103c42a3 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -481,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()