From 21efd265174cd887ec90bb9ee363597ee9ad5e2f Mon Sep 17 00:00:00 2001 From: merge-script Date: Thu, 11 Apr 2024 16:18:39 +0200 Subject: [PATCH 1/8] Merge bitcoin/bitcoin#29849: Fix typos in `subprocess.hpp` 13f5391bbb45cd8aebc6ae70cad08aff632ebd55 Fix typos in `subprocess.hpp` (Hennadii Stepanov) Pull request description: Resolves one item in the https://github.com/bitcoin/bitcoin/pull/28981#pullrequestreview-1991272752: > - Remove linter exclusions and fix all issues. Based on upstream https://github.com/arun11299/cpp-subprocess/pull/101. ACKs for top commit: fanquake: ACK 13f5391bbb45cd8aebc6ae70cad08aff632ebd55 Tree-SHA512: 2ee27a5b7d1ba6f47a5148add155c918eadaaffb94a4b5dd3edea00e63440b87291c559361bf25a8db1567debff78cf7e9466dc34f14331ca1d426994837df93 --- src/util/subprocess.hpp | 32 +++++++++++++++++--------------- test/lint/lint-spelling.py | 2 +- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/src/util/subprocess.hpp b/src/util/subprocess.hpp index dd53a8fbb4fb..0fcc9397ea7a 100644 --- a/src/util/subprocess.hpp +++ b/src/util/subprocess.hpp @@ -1,6 +1,8 @@ +// Based on the https://github.com/arun11299/cpp-subprocess project. + /*! -Documentation for C++ subprocessing libraray. +Documentation for C++ subprocessing library. @copyright The code is licensed under the [MIT License](http://opensource.org/licenses/MIT): @@ -106,7 +108,7 @@ namespace subprocess { // from pipe static const size_t SP_MAX_ERR_BUF_SIZ = 1024; -// Default buffer capcity for OutBuffer and ErrBuffer. +// Default buffer capacity for OutBuffer and ErrBuffer. // If the data exceeds this capacity, the buffer size is grown // by 1.5 times its previous capacity static const size_t DEFAULT_BUF_CAP_BYTES = 8192; @@ -312,8 +314,8 @@ namespace util inline env_map_t MapFromWindowsEnvironment(){ wchar_t *variable_strings_ptr; wchar_t *environment_strings_ptr; - std::wstring delimeter(L"="); - int del_len = delimeter.length(); + std::wstring delimiter(L"="); + int del_len = delimiter.length(); env_map_t mapped_environment; // Get a pointer to the environment block. @@ -335,7 +337,7 @@ namespace util // Create a string from Variable String env_string_t current_line(variable_strings_ptr); // Find the first "equals" sign. - auto pos = current_line.find(delimeter); + auto pos = current_line.find(delimiter); // Assuming it's not missing ... if(pos!=std::wstring::npos){ // ... parse the key and value. @@ -433,8 +435,8 @@ namespace util * Function: join * Parameters: * [in] vec : Vector of strings which needs to be joined to form - * a single string with words seperated by a seperator char. - * [in] sep : String used to seperate 2 words in the joined string. + * a single string with words separated by a separator char. + * [in] sep : String used to separate 2 words in the joined string. * Default constructed to ' ' (space). * [out] string: Joined string. */ @@ -655,7 +657,7 @@ namespace util * Default value is 0. */ struct bufsize { - explicit bufsize(int siz): bufsiz(siz) {} + explicit bufsize(int sz): bufsiz(sz) {} int bufsiz = 0; }; @@ -711,7 +713,7 @@ struct string_arg }; /*! - * Option to specify the executable name seperately + * Option to specify the executable name separately * from the args sequence. * In this case the cmd args must only contain the * options required for this executable. @@ -728,7 +730,7 @@ struct executable: string_arg * Option to set the current working directory * of the spawned process. * - * Eg: cwd{"/som/path/x"} + * Eg: cwd{"/some/path/x"} */ struct cwd: string_arg { @@ -852,7 +854,7 @@ struct error wr_ch_ = fd; } explicit error(IOTYPE typ) { - assert ((typ == PIPE || typ == STDOUT) && "STDERR not aloowed"); + assert ((typ == PIPE || typ == STDOUT) && "STDERR not allowed"); if (typ == PIPE) { #ifndef __USING_WINDOWS__ std::tie(rd_ch_, wr_ch_) = util::pipe_cloexec(); @@ -874,7 +876,7 @@ struct error // ATTN: Can be used only to execute functions with no // arguments and returning void. // Could have used more efficient methods, ofcourse, but -// that wont yield me the consistent syntax which I am +// that won't yield me the consistent syntax which I am // aiming for. If you know, then please do let me know. class preexec_func @@ -1211,10 +1213,10 @@ class Streams * 9. communicate(...) - Get the output/error from the child and close the channels * from the parent side. *10. input() - Get the input channel/File pointer. Can be used for - * cutomizing the way of sending input to child. + * customizing the way of sending input to child. *11. output() - Get the output channel/File pointer. Usually used in case of redirection. See piping examples. - *12. error() - Get the error channel/File poiner. Usually used + *12. error() - Get the error channel/File pointer. Usually used in case of redirection. *13. start_process() - Start the child process. Only to be used when * `defer_spawn` option was provided in Popen constructor. @@ -1358,7 +1360,7 @@ class Popen // Command in string format std::string args_; - // Comamnd provided as sequence + // Command provided as sequence std::vector vargs_; std::vector cargv_; diff --git a/test/lint/lint-spelling.py b/test/lint/lint-spelling.py index fb4d2495c691..79c4533bdf55 100755 --- a/test/lint/lint-spelling.py +++ b/test/lint/lint-spelling.py @@ -12,7 +12,7 @@ from subprocess import check_output, STDOUT, CalledProcessError IGNORE_WORDS_FILE = 'test/lint/spelling.ignore-words.txt' -FILES_ARGS = ['git', 'ls-files', '--', ":(exclude)build-aux/m4/", ":(exclude)contrib/seeds/*.txt", ":(exclude)depends/", ":(exclude)doc/release-notes/", ":(exclude)src/dashbls/", ":(exclude)src/crc32c/", ":(exclude)src/crypto/", ":(exclude)src/ctpl_stl.h", ":(exclude)src/cxxtimer.hpp", ":(exclude)src/immer/", ":(exclude)src/leveldb/", ":(exclude)src/qt/locale/", ":(exclude)src/qt/*.qrc", ":(exclude)src/secp256k1/", ":(exclude)src/minisketch/", ":(exclude)contrib/builder-keys/", ":(exclude)contrib/guix/patches", ":(exclude)src/util/subprocess.hpp", ":(exclude)src/wallet/bip39_english.h"] +FILES_ARGS = ['git', 'ls-files', '--', ":(exclude)build-aux/m4/", ":(exclude)contrib/seeds/*.txt", ":(exclude)depends/", ":(exclude)doc/release-notes/", ":(exclude)src/dashbls/", ":(exclude)src/crc32c/", ":(exclude)src/crypto/", ":(exclude)src/ctpl_stl.h", ":(exclude)src/cxxtimer.hpp", ":(exclude)src/immer/", ":(exclude)src/leveldb/", ":(exclude)src/qt/locale/", ":(exclude)src/qt/*.qrc", ":(exclude)src/secp256k1/", ":(exclude)src/minisketch/", ":(exclude)contrib/builder-keys/", ":(exclude)contrib/guix/patches", ":(exclude)src/wallet/bip39_english.h"] def check_codespell_install(): From 86f25a85538f84907bf5c5caf2228c99457fedfb Mon Sep 17 00:00:00 2001 From: merge-script Date: Wed, 24 Apr 2024 22:57:50 +0800 Subject: [PATCH 2/8] Merge bitcoin/bitcoin#29910: refactor: Rename `subprocess.hpp` to follow our header name conventions 08f756bd370c3e100b186c7e24bef6a033575b29 Replace locale-dependent `std::strerror` with `SysErrorString` (Hennadii Stepanov) d8e4ba4d0568769782b8c19c82e955c4aee73477 refactor: Rename `subprocess.hpp` to follow our header name conventions (Hennadii Stepanov) Pull request description: This PR renames the header `*.hpp` --> `*.h` and adjusts the header guard name, which makes it available for processing by linters. Fixed the following linter warning: ``` The locale dependent function strerror(...) appears to be used: src/util/subprocess.h: std::runtime_error( err_msg + ": " + std::strerror(err_code) ) Unnecessary locale dependence can cause bugs that are very tricky to isolate and fix. Please avoid using locale-dependent functions if possible. Advice not applicable in this specific case? Add an exception by updating the ignore list in /bitcoin/test/lint/lint-locale-dependence.py ^---- failure generated from lint-locale-dependence.py ``` ACKs for top commit: TheCharlatan: ACK 08f756bd370c3e100b186c7e24bef6a033575b29 Tree-SHA512: 57a2f01c20eb9552481e428a4969bd59e9ada9f784fe1a45cb62aa9c9152c8e950d336854f45af0e2e5dc7c7b2a1fb216c8f832e3d6ccfb457ad71b6e423231e --- src/Makefile.am | 2 +- src/common/run_command.cpp | 2 +- src/test/system_tests.cpp | 2 +- src/util/{subprocess.hpp => subprocess.h} | 10 ++++++---- 4 files changed, 9 insertions(+), 7 deletions(-) rename src/util/{subprocess.hpp => subprocess.h} (99%) diff --git a/src/Makefile.am b/src/Makefile.am index 4f40714c7677..9de99c307476 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -424,7 +424,7 @@ BITCOIN_CORE_H = \ util/sock.h \ util/string.h \ util/spanparsing.h \ - util/subprocess.hpp \ + util/subprocess.h \ util/syserror.h \ util/system.h \ util/time.h \ diff --git a/src/common/run_command.cpp b/src/common/run_command.cpp index e5356490ef7f..347b4860955f 100644 --- a/src/common/run_command.cpp +++ b/src/common/run_command.cpp @@ -12,7 +12,7 @@ #include #ifdef ENABLE_EXTERNAL_SIGNER -#include +#include #endif // ENABLE_EXTERNAL_SIGNER UniValue RunCommandParseJSON(const std::string& str_command, const std::string& str_std_in) diff --git a/src/test/system_tests.cpp b/src/test/system_tests.cpp index ce7ed3912c79..3a11152bf071 100644 --- a/src/test/system_tests.cpp +++ b/src/test/system_tests.cpp @@ -7,7 +7,7 @@ #include #ifdef ENABLE_EXTERNAL_SIGNER -#include +#include #endif // ENABLE_EXTERNAL_SIGNER #include diff --git a/src/util/subprocess.hpp b/src/util/subprocess.h similarity index 99% rename from src/util/subprocess.hpp rename to src/util/subprocess.h index 0fcc9397ea7a..501155dffdcd 100644 --- a/src/util/subprocess.hpp +++ b/src/util/subprocess.h @@ -33,8 +33,10 @@ Documentation for C++ subprocessing library. @version 1.0.0 */ -#ifndef SUBPROCESS_HPP -#define SUBPROCESS_HPP +#ifndef BITCOIN_UTIL_SUBPROCESS_H +#define BITCOIN_UTIL_SUBPROCESS_H + +#include #include #include @@ -150,7 +152,7 @@ class OSError: public std::runtime_error { public: OSError(const std::string& err_msg, int err_code): - std::runtime_error( err_msg + ": " + std::strerror(err_code) ) + std::runtime_error(err_msg + ": " + SysErrorString(err_code)) {} }; @@ -1985,4 +1987,4 @@ namespace detail { } -#endif // SUBPROCESS_HPP +#endif // BITCOIN_UTIL_SUBPROCESS_H From 4c48c1a080d820642d26115bbb7ac54870467217 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 25 Apr 2024 12:49:26 -0400 Subject: [PATCH 3/8] Merge bitcoin/bitcoin#29904: refactor: Use our own implementation of urlDecode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 992c714451676cee33d3dff49f36329423270c1c common: Don't terminate on null character in UrlDecode (Fabian Jahr) 099fa571511f113e0056d4bc27b3153a42f9dc65 scripted-diff: Modernize name of urlDecode function and param (Fabian Jahr) 8f39aaae417c33490e0e41fb97620eb23ced3d05 refactor: Remove hooking code for urlDecode (Fabian Jahr) 650d43ec15f7a3ae38126f65ef8fa0b1fd3ee936 refactor: Replace libevent use in urlDecode with our own code (Fabian Jahr) 46bc6c2aaa613eef526b21a06bf21e8edde31a88 test: Add unit tests for urlDecode (Fabian Jahr) Pull request description: Fixes #29654 (as a side-effect) Removing dependencies is a general goal of the project and the xz backdoor has been an additional wake up call recently. Libevent shows many of the same symptoms, few maintainers and slow releases. While libevent can not be removed completely over night we should start removing itโ€™s usage where it's possible, ideally with the end goal to removing it completely. This is a pretty easy win in that direction. The [`evhttp_uridecode` function from libevent](https://github.com/libevent/libevent/blob/e0a4574ba2cbcdb64bb2b593e72be7f7f4010746/http.c#L3542) we were using in `urlDecode` could be easily emulated in fewer LOC. This also ports the [applicable test vectors over from libevent](https://github.com/libevent/libevent/blob/master/test/regress_http.c#L3430). ACKs for top commit: achow101: ACK 992c714451676cee33d3dff49f36329423270c1c theStack: Code-review ACK 992c714451676cee33d3dff49f36329423270c1c maflcko: ACK 992c714451676cee33d3dff49f36329423270c1c ๐Ÿ‘ˆ stickies-v: ACK 992c714451676cee33d3dff49f36329423270c1c Tree-SHA512: 78f76ae7ab3b6710eab2aaac20f55eb0da7803e057eaa6220e865f328666a5399ef1a479702aaf630b2f974ad3aa15e2b6adac9c11bc8c3d4be21e8af1667fea --- configure.ac | 1 - src/Makefile.am | 6 +-- src/Makefile.test.include | 1 + src/bitcoin-cli.cpp | 2 - src/bitcoin-wallet.cpp | 2 - src/bitcoind.cpp | 2 - src/common/url.cpp | 35 ++++++++++++----- src/common/url.h | 9 +++-- src/qt/main.cpp | 2 - src/test/common_url_tests.cpp | 72 ++++++++++++++++++++++++++++++++++ src/test/fuzz/string.cpp | 2 +- src/test/util/setup_common.cpp | 2 - src/wallet/rpc/util.cpp | 5 ++- 13 files changed, 110 insertions(+), 31 deletions(-) create mode 100644 src/test/common_url_tests.cpp diff --git a/configure.ac b/configure.ac index 0c97af58e657..e8c922437a9d 100644 --- a/configure.ac +++ b/configure.ac @@ -1966,7 +1966,6 @@ AM_CONDITIONAL([ENABLE_QT_TESTS], [test "$BUILD_TEST_QT" = "yes"]) AM_CONDITIONAL([ENABLE_BENCH], [test "$use_bench" = "yes"]) AM_CONDITIONAL([USE_QRCODE], [test "$use_qr" = "yes"]) AM_CONDITIONAL([USE_LCOV], [test "$use_lcov" = "yes"]) -AM_CONDITIONAL([USE_LIBEVENT], [test "$use_libevent" = "yes"]) AM_CONDITIONAL([HARDEN], [test "$use_hardening" = "yes"]) AM_CONDITIONAL([ENABLE_SSSE3], [test "$enable_ssse3" = "yes"]) AM_CONDITIONAL([ENABLE_SSE42], [test "$enable_sse42" = "yes"]) diff --git a/src/Makefile.am b/src/Makefile.am index 9de99c307476..e8ec46b4a5c8 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -927,6 +927,7 @@ libbitcoin_common_a_SOURCES = \ coins.cpp \ common/bloom.cpp \ common/run_command.cpp \ + common/url.cpp \ compressor.cpp \ core_read.cpp \ core_write.cpp \ @@ -964,11 +965,6 @@ libbitcoin_common_a_SOURCES = \ script/standard.cpp \ warnings.cpp \ $(BITCOIN_CORE_H) - -if USE_LIBEVENT -libbitcoin_common_a_CPPFLAGS += $(EVENT_CFLAGS) -libbitcoin_common_a_SOURCES += common/url.cpp -endif # # util # diff --git a/src/Makefile.test.include b/src/Makefile.test.include index b7d7cc5cde14..78cd380bb958 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -100,6 +100,7 @@ BITCOIN_TESTS =\ test/cachemultimap_tests.cpp \ test/coins_tests.cpp \ test/coinstatsindex_tests.cpp \ + test/common_url_tests.cpp \ test/compilerbug_tests.cpp \ test/compress_tests.cpp \ test/crypto_tests.cpp \ diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp index 3b0873970e9f..6c94f27b87d5 100644 --- a/src/bitcoin-cli.cpp +++ b/src/bitcoin-cli.cpp @@ -10,7 +10,6 @@ #include #include -#include #include #include #include @@ -49,7 +48,6 @@ using CliClock = std::chrono::system_clock; const std::function G_TRANSLATION_FUN = nullptr; -UrlDecodeFn* const URL_DECODE = urlDecode; static const char DEFAULT_RPCCONNECT[] = "127.0.0.1"; static const int DEFAULT_HTTP_CLIENT_TIMEOUT=900; diff --git a/src/bitcoin-wallet.cpp b/src/bitcoin-wallet.cpp index be9ba957ec4b..3b79b75cc46c 100644 --- a/src/bitcoin-wallet.cpp +++ b/src/bitcoin-wallet.cpp @@ -8,7 +8,6 @@ #include #include -#include #include #include #include @@ -25,7 +24,6 @@ #include #include const std::function G_TRANSLATION_FUN = nullptr; -UrlDecodeFn* const URL_DECODE = nullptr; static void SetupWalletToolArgs(ArgsManager& argsman) { diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp index b364c7e5439a..b01859aaa30c 100644 --- a/src/bitcoind.cpp +++ b/src/bitcoind.cpp @@ -10,7 +10,6 @@ #include #include -#include #include #include #include @@ -34,7 +33,6 @@ using node::NodeContext; const std::function G_TRANSLATION_FUN = nullptr; -UrlDecodeFn* const URL_DECODE = urlDecode; #if HAVE_DECL_FORK diff --git a/src/common/url.cpp b/src/common/url.cpp index 5200d55096a7..283352a38e51 100644 --- a/src/common/url.cpp +++ b/src/common/url.cpp @@ -4,19 +4,36 @@ #include -#include - -#include +#include #include +#include +#include -std::string urlDecode(const std::string &urlEncoded) { +std::string UrlDecode(std::string_view url_encoded) +{ std::string res; - if (!urlEncoded.empty()) { - char *decoded = evhttp_uridecode(urlEncoded.c_str(), false, nullptr); - if (decoded) { - res = std::string(decoded); - free(decoded); + res.reserve(url_encoded.size()); + + for (size_t i = 0; i < url_encoded.size(); ++i) { + char c = url_encoded[i]; + // Special handling for percent which should be followed by two hex digits + // representing an octet values, see RFC 3986, Section 2.1 Percent-Encoding + if (c == '%' && i + 2 < url_encoded.size()) { + unsigned int decoded_value{0}; + auto [p, ec] = std::from_chars(url_encoded.data() + i + 1, url_encoded.data() + i + 3, decoded_value, 16); + + // Only if there is no error and the pointer is set to the end of + // the string, we can be sure both characters were valid hex + if (ec == std::errc{} && p == url_encoded.data() + i + 3) { + res += static_cast(decoded_value); + // Next two characters are part of the percent encoding + i += 2; + continue; + } + // In case of invalid percent encoding, add the '%' and continue } + res += c; } + return res; } diff --git a/src/common/url.h b/src/common/url.h index 7bbd8b60de3c..3213ef90f058 100644 --- a/src/common/url.h +++ b/src/common/url.h @@ -6,9 +6,12 @@ #define BITCOIN_COMMON_URL_H #include +#include -using UrlDecodeFn = std::string(const std::string& url_encoded); -UrlDecodeFn urlDecode; -extern UrlDecodeFn* const URL_DECODE; +/* Decode a URL. + * + * Notably this implementation does not decode a '+' to a ' '. + */ +std::string UrlDecode(std::string_view url_encoded); #endif // BITCOIN_COMMON_URL_H diff --git a/src/qt/main.cpp b/src/qt/main.cpp index 19a8b3685548..4ca0650dc40b 100644 --- a/src/qt/main.cpp +++ b/src/qt/main.cpp @@ -4,7 +4,6 @@ #include -#include #include #include @@ -17,7 +16,6 @@ extern const std::function G_TRANSLATION_FUN = [](const char* psz) { return QCoreApplication::translate("dash-core", psz).toStdString(); }; -UrlDecodeFn* const URL_DECODE = urlDecode; MAIN_FUNCTION { diff --git a/src/test/common_url_tests.cpp b/src/test/common_url_tests.cpp new file mode 100644 index 000000000000..cc893cbed724 --- /dev/null +++ b/src/test/common_url_tests.cpp @@ -0,0 +1,72 @@ +// Copyright (c) 2024-present The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or https://opensource.org/license/mit/. + +#include + +#include + +#include + +BOOST_AUTO_TEST_SUITE(common_url_tests) + +// These test vectors were ported from test/regress.c in the libevent library +// which used to be a dependency of the UrlDecode function. + +BOOST_AUTO_TEST_CASE(encode_decode_test) { + BOOST_CHECK_EQUAL(UrlDecode("Hello"), "Hello"); + BOOST_CHECK_EQUAL(UrlDecode("99"), "99"); + BOOST_CHECK_EQUAL(UrlDecode(""), ""); + BOOST_CHECK_EQUAL(UrlDecode("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ123456789-.~_"), + "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ123456789-.~_"); + BOOST_CHECK_EQUAL(UrlDecode("%20"), " "); + BOOST_CHECK_EQUAL(UrlDecode("%FF%F0%E0"), "\xff\xf0\xe0"); + BOOST_CHECK_EQUAL(UrlDecode("%01%19"), "\x01\x19"); + BOOST_CHECK_EQUAL(UrlDecode("http%3A%2F%2Fwww.ietf.org%2Frfc%2Frfc3986.txt"), + "http://www.ietf.org/rfc/rfc3986.txt"); + BOOST_CHECK_EQUAL(UrlDecode("1%2B2%3D3"), "1+2=3"); +} + +BOOST_AUTO_TEST_CASE(decode_malformed_test) { + BOOST_CHECK_EQUAL(UrlDecode("%%xhello th+ere \xff"), "%%xhello th+ere \xff"); + + BOOST_CHECK_EQUAL(UrlDecode("%"), "%"); + BOOST_CHECK_EQUAL(UrlDecode("%%"), "%%"); + BOOST_CHECK_EQUAL(UrlDecode("%%%"), "%%%"); + BOOST_CHECK_EQUAL(UrlDecode("%%%%"), "%%%%"); + + BOOST_CHECK_EQUAL(UrlDecode("+"), "+"); + BOOST_CHECK_EQUAL(UrlDecode("++"), "++"); + + BOOST_CHECK_EQUAL(UrlDecode("?"), "?"); + BOOST_CHECK_EQUAL(UrlDecode("??"), "??"); + + BOOST_CHECK_EQUAL(UrlDecode("%G1"), "%G1"); + BOOST_CHECK_EQUAL(UrlDecode("%2"), "%2"); + BOOST_CHECK_EQUAL(UrlDecode("%ZX"), "%ZX"); + + BOOST_CHECK_EQUAL(UrlDecode("valid%20string%G1"), "valid string%G1"); + BOOST_CHECK_EQUAL(UrlDecode("%20invalid%ZX"), " invalid%ZX"); + BOOST_CHECK_EQUAL(UrlDecode("%20%G1%ZX"), " %G1%ZX"); + + BOOST_CHECK_EQUAL(UrlDecode("%1 "), "%1 "); + BOOST_CHECK_EQUAL(UrlDecode("% 9"), "% 9"); + BOOST_CHECK_EQUAL(UrlDecode(" %Z "), " %Z "); + BOOST_CHECK_EQUAL(UrlDecode(" % X"), " % X"); + + BOOST_CHECK_EQUAL(UrlDecode("%-1"), "%-1"); + BOOST_CHECK_EQUAL(UrlDecode("%1-"), "%1-"); +} + +BOOST_AUTO_TEST_CASE(decode_lowercase_hex_test) { + BOOST_CHECK_EQUAL(UrlDecode("%f0%a0%b0"), "\xf0\xa0\xb0"); +} + +BOOST_AUTO_TEST_CASE(decode_internal_nulls_test) { + std::string result1{"\0\0x\0\0", 5}; + BOOST_CHECK_EQUAL(UrlDecode("%00%00x%00%00"), result1); + std::string result2{"abc\0\0", 5}; + BOOST_CHECK_EQUAL(UrlDecode("abc%00%00"), result2); +} + +BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/fuzz/string.cpp b/src/test/fuzz/string.cpp index 33490b0c1f28..0d15ec776ea7 100644 --- a/src/test/fuzz/string.cpp +++ b/src/test/fuzz/string.cpp @@ -180,7 +180,7 @@ FUZZ_TARGET(string) (void)ToUpper(random_string_1); (void)TrimString(random_string_1); (void)TrimString(random_string_1, random_string_2); - (void)urlDecode(random_string_1); + (void)UrlDecode(random_string_1); (void)ContainsNoNUL(random_string_1); (void)_(random_string_1.c_str()); try { diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 2616b5ae1bfe..21b5b83cfb40 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -9,7 +9,6 @@ #include #include #include -#include #include #include #include @@ -92,7 +91,6 @@ using node::fPruneMode; using node::fReindex; const std::function G_TRANSLATION_FUN = nullptr; -UrlDecodeFn* const URL_DECODE = nullptr; FastRandomContext g_insecure_rand_ctx; /** Random context to get unique temp data dirs. Separate from g_insecure_rand_ctx, which can be seeded from a const env var */ diff --git a/src/wallet/rpc/util.cpp b/src/wallet/rpc/util.cpp index e67be931af6d..75b369b45f2a 100644 --- a/src/wallet/rpc/util.cpp +++ b/src/wallet/rpc/util.cpp @@ -10,6 +10,7 @@ #include #include +#include #include #include @@ -60,9 +61,9 @@ bool ParseIncludeWatchonly(const UniValue& include_watchonly, const CWallet& wal bool GetWalletNameFromJSONRPCRequest(const JSONRPCRequest& request, std::string& wallet_name) { - if (URL_DECODE && request.URI.substr(0, WALLET_ENDPOINT_BASE.size()) == WALLET_ENDPOINT_BASE) { + if (request.URI.starts_with(WALLET_ENDPOINT_BASE)) { // wallet endpoint was used - wallet_name = URL_DECODE(request.URI.substr(WALLET_ENDPOINT_BASE.size())); + wallet_name = UrlDecode(std::string_view{request.URI}.substr(WALLET_ENDPOINT_BASE.size())); return true; } return false; From 26fae74374abb5c93e5822a9bf10320e1186dd10 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Thu, 2 May 2024 16:33:18 -0400 Subject: [PATCH 4/8] Merge bitcoin/bitcoin#29961: refactor: remove remaining unused code from cpp-subprocess 8b52e7f628304e83b0e36fd97e617de0f71c5a62 update comments in cpp-subprocess (check_output references) (Sebastian Falbesoner) 97f159776ec06f767df1d4990aa7d0859140f52f remove unused method `Popen::kill` from cpp-subprocess (Sebastian Falbesoner) 908c51fe4afeba0af500c6275027b1afa1b3bd19 remove commented out code in cpp-subprocess (Sebastian Falbesoner) ff79adbe056220202f7a56d67f788c38fc49ef9f remove unused templates from cpp-subprocess (Sebastian Falbesoner) Pull request description: This PR removes remaining code that is unused within the cpp-subprocess module (templates and commented out code). Happy to add more removals if anyone finds more unused parts. Note that there are some API functions of the `Popen` class that we don't use, e.g. `wait()`, `pid()`, `poll()`, `kill()`, but they sound IMHO common enough to be useful in the future, so not sure how deep we should go there. ACKs for top commit: fjahr: Code review ACK 8b52e7f628304e83b0e36fd97e617de0f71c5a62 achow101: ACK 8b52e7f628304e83b0e36fd97e617de0f71c5a62 hebasto: ACK 8b52e7f628304e83b0e36fd97e617de0f71c5a62. Tree-SHA512: 14c1cd2216185d941923f06fdc7acbeed66cd87e2691d9a352f7309b3e07fe4877b580f598a2e4106f9c48395ed6de00a0bfb5d3c3af9c4624d1956a0f543e99 --- src/util/subprocess.h | 84 ++++++++++--------------------------------- 1 file changed, 18 insertions(+), 66 deletions(-) diff --git a/src/util/subprocess.h b/src/util/subprocess.h index 501155dffdcd..a1be9a145f07 100644 --- a/src/util/subprocess.h +++ b/src/util/subprocess.h @@ -172,12 +172,6 @@ using env_vector_t = std::vector; //-------------------------------------------------------------------- namespace util { - template - inline bool is_ready(std::shared_future const &f) - { - return f.wait_for(std::chrono::seconds(0)) == std::future_status::ready; - } - inline void quote_argument(const std::wstring &argument, std::wstring &command_line, bool force) { @@ -919,8 +913,8 @@ class preexec_func * This is basically used to determine the length of the actual * data stored inside the dynamically resized vector. * - * This is what is returned as the output to communicate and check_output - * functions, so, users must know about this class. + * This is what is returned as the output to the communicate + * function, so, users must know about this class. * * OutBuffer and ErrBuffer are just different typedefs to this class. */ @@ -931,22 +925,6 @@ class Buffer explicit Buffer(size_t cap) { buf.resize(cap); } void add_cap(size_t cap) { buf.resize(cap); } -#if 0 - Buffer(const Buffer& other): - buf(other.buf), - length(other.length) - { - std::cout << "COPY" << std::endl; - } - - Buffer(Buffer&& other): - buf(std::move(other.buf)), - length(other.length) - { - std::cout << "MOVE" << std::endl; - } -#endif - public: std::vector buf; size_t length = 0; @@ -1202,25 +1180,24 @@ class Streams * interface to the client. * * API's provided by the class: - * 1. Popen({"cmd"}, output{..}, error{..}, cwd{..}, ....) + * Popen({"cmd"}, output{..}, error{..}, cwd{..}, ....) * Command provided as a sequence. - * 2. Popen("cmd arg1"m output{..}, error{..}, cwd{..}, ....) + * Popen("cmd arg1", output{..}, error{..}, cwd{..}, ....) * Command provided in a single string. - * 3. wait() - Wait for the child to exit. - * 4. retcode() - The return code of the exited child. - * 5. pid() - PID of the spawned child. - * 6. poll() - Check the status of the running child. - * 7. kill(sig_num) - Kill the child. SIGTERM used by default. - * 8. send(...) - Send input to the input channel of the child. - * 9. communicate(...) - Get the output/error from the child and close the channels - * from the parent side. - *10. input() - Get the input channel/File pointer. Can be used for - * customizing the way of sending input to child. - *11. output() - Get the output channel/File pointer. Usually used - in case of redirection. See piping examples. - *12. error() - Get the error channel/File pointer. Usually used - in case of redirection. - *13. start_process() - Start the child process. Only to be used when + * wait() - Wait for the child to exit. + * retcode() - The return code of the exited child. + * pid() - PID of the spawned child. + * poll() - Check the status of the running child. + * send(...) - Send input to the input channel of the child. + * communicate(...) - Get the output/error from the child and close the channels + * from the parent side. + * input() - Get the input channel/File pointer. Can be used for + * customizing the way of sending input to child. + * output() - Get the output channel/File pointer. Usually used + in case of redirection. See piping examples. + * error() - Get the error channel/File pointer. Usually used + in case of redirection. + * start_process() - Start the child process. Only to be used when * `defer_spawn` option was provided in Popen constructor. */ class Popen @@ -1265,14 +1242,6 @@ class Popen if (!defer_process_start_) execute_process(); } -/* - ~Popen() - { -#ifdef __USING_WINDOWS__ - CloseHandle(this->process_handle_); -#endif - } -*/ void start_process() noexcept(false); @@ -1284,10 +1253,6 @@ class Popen int poll() noexcept(false); - // Does not fail, Caller is expected to recheck the - // status with a call to poll() - void kill(int sig_num = 9); - void set_out_buf_cap(size_t cap) { stream_.set_out_buf_cap(cap); } void set_err_buf_cap(size_t cap) { stream_.set_err_buf_cap(cap); } @@ -1478,19 +1443,6 @@ inline int Popen::poll() noexcept(false) #endif } -inline void Popen::kill(int sig_num) -{ -#ifdef __USING_WINDOWS__ - if (!TerminateProcess(this->process_handle_, (UINT)sig_num)) { - throw OSError("TerminateProcess", 0); - } -#else - if (session_leader_) killpg(child_pid_, sig_num); - else ::kill(child_pid_, sig_num); -#endif -} - - inline void Popen::execute_process() noexcept(false) { #ifdef __USING_WINDOWS__ From 45cee73a63b33f6d865e2d1a212ca2bdc5b883ab Mon Sep 17 00:00:00 2001 From: merge-script Date: Fri, 3 May 2024 10:47:37 +0800 Subject: [PATCH 5/8] Merge bitcoin/bitcoin#30017: refactor, fuzz: Make 64-bit shift explicit b50d127a7710d790c2ba4a08f01b832c2a0b1203 refactor: Make 64-bit shift explicit (Hennadii Stepanov) Pull request description: This PR fixes MSVC warning [C4334](https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4334) in the fuzzing code. Similar to https://github.com/bitcoin/bitcoin/pull/26252. All `DisableSpecificWarnings` dropped from `fuzz.vcxproj` as all remained are inherited from `common.init.vcxproj`. Required to simplify warning suppression porting to the CMake-based build system. ACKs for top commit: maflcko: utACK b50d127a7710d790c2ba4a08f01b832c2a0b1203 sipsorcery: utACK b50d127a7710d790c2ba4a08f01b832c2a0b1203 Tree-SHA512: 18f6082b4234506ad2f9df54e577031b97cdf9f7ef64cad4162f275660716ab73587a97d3af0f778dfd48d2751d8676b5d3381d0aa837fcc60a09704473a9209 --- src/test/fuzz/poolresource.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/fuzz/poolresource.cpp b/src/test/fuzz/poolresource.cpp index ce64ef6472cf..f764d9f8dbe7 100644 --- a/src/test/fuzz/poolresource.cpp +++ b/src/test/fuzz/poolresource.cpp @@ -63,9 +63,9 @@ class PoolResourceFuzzer { if (m_total_allocated > 0x1000000) return; size_t alignment_bits = m_provider.ConsumeIntegralInRange(0, 7); - size_t alignment = 1 << alignment_bits; + size_t alignment = size_t{1} << alignment_bits; size_t size_bits = m_provider.ConsumeIntegralInRange(0, 16 - alignment_bits); - size_t size = m_provider.ConsumeIntegralInRange(1U << size_bits, (1U << (size_bits + 1)) - 1U) << alignment_bits; + size_t size = m_provider.ConsumeIntegralInRange(size_t{1} << size_bits, (size_t{1} << (size_bits + 1)) - 1U) << alignment_bits; Allocate(size, alignment); } From e95136399126b8f834faa558c358040bef514cd7 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Fri, 3 May 2024 09:44:00 -0400 Subject: [PATCH 6/8] Merge bitcoin/bitcoin#30026: refactor, test: Always initialize pointer bd2de7ac591d7704b79304089ad1fb57e085da8b refactor, test: Always initialize pointer (Hennadii Stepanov) Pull request description: This change fixes MSVC warning [C4703](https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4703). All `DisableSpecificWarnings` dropped from `test_bitcoin.vcxproj` as all remained are inherited from `common.init.vcxproj`. Required to simplify warning suppression porting to the CMake-based build system. ACKs for top commit: maflcko: utACK bd2de7ac591d7704b79304089ad1fb57e085da8b sipsorcery: utACK bd2de7ac591d7704b79304089ad1fb57e085da8b. ryanofsky: Code review ACK bd2de7ac591d7704b79304089ad1fb57e085da8b Tree-SHA512: 006db041d3c3697a77d9df14de86cf7c8a10804b45789df01268b2236cf6452e77dc57e89f5d5a6bc26d4b5cd483f0722d6035649c8a523b57954bb1fc810d0c --- src/test/validation_chainstate_tests.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/test/validation_chainstate_tests.cpp b/src/test/validation_chainstate_tests.cpp index 73920bda6743..3d7d9e50e368 100644 --- a/src/test/validation_chainstate_tests.cpp +++ b/src/test/validation_chainstate_tests.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include #include @@ -96,14 +97,14 @@ BOOST_FIXTURE_TEST_CASE(chainstate_update_tip, TestChain100Setup) BOOST_CHECK_EQUAL(chainman.GetAll().size(), 2); - CChainState& background_cs{*[&] { + CChainState& background_cs{*Assert([&]() -> CChainState* { for (CChainState* cs : chainman.GetAll()) { if (cs != &chainman.ActiveChainstate()) { return cs; } } - assert(false); - }()}; + return nullptr; + }())}; // Create a block to append to the validation chain. std::vector noTxns; From 05f1dd9a9fe25292eba7ebd8ab9c1eb59a60b532 Mon Sep 17 00:00:00 2001 From: merge-script Date: Wed, 29 May 2024 09:22:24 +0100 Subject: [PATCH 7/8] Merge bitcoin/bitcoin#30170: refactor: Use type-safe time in txorphanage fa6d4891c7815025ab1cd58d0906466af31bb648 refactor: Use type-safe time in txorphanage (MarcoFalke) Pull request description: This allows to remove manual conversions like multiplication by `60`, and uses a type-safe type instead of a raw `int64_t`. ACKs for top commit: epiccurious: utACK fa6d4891c7815025ab1cd58d0906466af31bb648. dergoegge: Code review ACK fa6d4891c7815025ab1cd58d0906466af31bb648 brunoerg: utACK fa6d4891c7815025ab1cd58d0906466af31bb648 Tree-SHA512: c187d0e579b1131afcef8c901f5662c18ab867fa2a99fbb13b67bb1e10b2047128194bfef8329cde0d51e1c35d6227ae292b823968f37ea9422975e46e01846a --- src/txorphanage.cpp | 17 +++++++++-------- src/txorphanage.h | 3 ++- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index d03924c2fe7d..50c0758f32f1 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -8,13 +8,14 @@ #include #include #include +#include #include -/** Expiration time for orphan transactions in seconds */ -static constexpr int64_t ORPHAN_TX_EXPIRE_TIME = 20 * 60; -/** Minimum time between orphan transactions expire time checks in seconds */ -static constexpr int64_t ORPHAN_TX_EXPIRE_INTERVAL = 5 * 60; +/** Expiration time for orphan transactions */ +static constexpr auto ORPHAN_TX_EXPIRE_TIME{20min}; +/** Minimum time between orphan transactions expire time checks */ +static constexpr auto ORPHAN_TX_EXPIRE_INTERVAL{5min}; bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer) @@ -39,7 +40,7 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer) return false; } - auto ret = m_orphans.emplace(hash, OrphanTx{tx, peer, GetTime() + ORPHAN_TX_EXPIRE_TIME, m_orphan_list.size(), sz}); + auto ret = m_orphans.emplace(hash, OrphanTx{tx, peer, Now() + ORPHAN_TX_EXPIRE_TIME, m_orphan_list.size(), sz}); assert(ret.second); m_orphan_list.push_back(ret.first); for (const CTxIn& txin : tx->vin) { @@ -121,12 +122,12 @@ void TxOrphanage::LimitOrphans(unsigned int max_orphans_size) LOCK(m_mutex); unsigned int nEvicted = 0; - static int64_t nNextSweep; - int64_t nNow = GetTime(); + static NodeSeconds nNextSweep; + auto nNow{Now()}; if (nNextSweep <= nNow) { // Sweep out expired orphan pool entries: int nErased = 0; - int64_t nMinExpTime = nNow + ORPHAN_TX_EXPIRE_TIME - ORPHAN_TX_EXPIRE_INTERVAL; + auto nMinExpTime = nNow + ORPHAN_TX_EXPIRE_TIME - ORPHAN_TX_EXPIRE_INTERVAL; std::map::iterator iter = m_orphans.begin(); while (iter != m_orphans.end()) { diff --git a/src/txorphanage.h b/src/txorphanage.h index bd68ea091627..0bd364d672f6 100644 --- a/src/txorphanage.h +++ b/src/txorphanage.h @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -70,7 +71,7 @@ class TxOrphanage { struct OrphanTx { CTransactionRef tx; NodeId fromPeer; - int64_t nTimeExpire; + NodeSeconds nTimeExpire; size_t list_pos; size_t nTxSize; }; From 63328b0639e76a8a59061fe11d85375f272abc03 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 23 Apr 2024 12:58:50 -0400 Subject: [PATCH 8/8] Merge bitcoin/bitcoin#29865: util: remove unused cpp-subprocess options 13adbf733f09c73c3cf0025d94c52f9cec5dba3b remove unneeded environment option from cpp-subprocess (Sebastian Falbesoner) 2088777ba0f9ad3f6d4ab8b0b6ff8aad71117307 remove unneeded cwd option from cpp-subprocess (Sebastian Falbesoner) 03ffb09c31aa04cc296c0ce10d07109e22a8dd75 remove unneeded bufsize option from cpp-subprocess (Sebastian Falbesoner) 79c30363733503a1fb7d4c98aa0d56ced0be6e32 remove unneeded close_fds option from cpp-subprocess (Sebastian Falbesoner) 62db8f8e5a6cfe19d905afc91731d6bc8a665f61 remove unneeded session_leader option from cpp-subprocess (Sebastian Falbesoner) 80d008c66d00d3496cd8549daee6775cf2c6b782 remove unneeded defer_spawn option from cpp-subprocess (Sebastian Falbesoner) cececad7b29e2ca3de1216db1c541dba6dc81bfa remove unneeded preexec function option from cpp-subprocess (Sebastian Falbesoner) 633e45b2e2728efcb0637afa94fcbd5756dfbe76 remove unneeded shell option from cpp-subprocess (Sebastian Falbesoner) Pull request description: The newly introduced cpp-subprocess library provides a good number of options for the `Popen` class: https://github.com/bitcoin/bitcoin/blob/0de63b8b46eff5cda85b4950062703324ba65a80/src/util/subprocess.hpp#L1009-L1020 Some of them are either not fully implemented (`shell`, missing an implementation on Windows), implemented in an ugly way (e.g. using "Impoverished, meager, needy, truly needy version of type erasure" for `preexec_func` according to the author's own words) or simply unlikely to be ever needed for our external signer use-case (`defer_spawn`). Instead of maintaining incomplete and/or unneeded code, I'd suggest to get rid of it and only keep support for options if there is a strong reason for it. ACKs for top commit: achow101: ACK 13adbf733f09c73c3cf0025d94c52f9cec5dba3b hebasto: re-ACK 13adbf733f09c73c3cf0025d94c52f9cec5dba3b. Tree-SHA512: 8270da27891cb659da2ef6062a23f4b86331859b15ac27b79ae7433b14f5bd7efaba621f2b3ba1953708d0f38377a8bd23ef1cc0f28b9c152ac8958dd9eec6b0 --- src/util/subprocess.h | 393 +----------------------------------------- 1 file changed, 9 insertions(+), 384 deletions(-) diff --git a/src/util/subprocess.h b/src/util/subprocess.h index a1be9a145f07..25ba2c438f93 100644 --- a/src/util/subprocess.h +++ b/src/util/subprocess.h @@ -156,19 +156,6 @@ class OSError: public std::runtime_error {} }; -//-------------------------------------------------------------------- - -//Environment Variable types -#ifndef _MSC_VER - using env_string_t = std::string; - using env_char_t = char; -#else - using env_string_t = std::wstring; - using env_char_t = wchar_t; -#endif -using env_map_t = std::map; -using env_vector_t = std::vector; - //-------------------------------------------------------------------- namespace util { @@ -301,100 +288,6 @@ namespace util if (!SetHandleInformation(*child_handle, HANDLE_FLAG_INHERIT, 0)) throw OSError("SetHandleInformation", 0); } - - // env_map_t MapFromWindowsEnvironment() - // * Imports current Environment in a C-string table - // * Parses the strings by splitting on the first "=" per line - // * Creates a map of the variables - // * Returns the map - inline env_map_t MapFromWindowsEnvironment(){ - wchar_t *variable_strings_ptr; - wchar_t *environment_strings_ptr; - std::wstring delimiter(L"="); - int del_len = delimiter.length(); - env_map_t mapped_environment; - - // Get a pointer to the environment block. - environment_strings_ptr = GetEnvironmentStringsW(); - // If the returned pointer is NULL, exit. - if (environment_strings_ptr == NULL) - { - throw OSError("GetEnvironmentStringsW", 0); - } - - // Variable strings are separated by NULL byte, and the block is - // terminated by a NULL byte. - - variable_strings_ptr = (wchar_t *) environment_strings_ptr; - - //Since the environment map ends with a null, we can loop until we find it. - while (*variable_strings_ptr) - { - // Create a string from Variable String - env_string_t current_line(variable_strings_ptr); - // Find the first "equals" sign. - auto pos = current_line.find(delimiter); - // Assuming it's not missing ... - if(pos!=std::wstring::npos){ - // ... parse the key and value. - env_string_t key = current_line.substr(0, pos); - env_string_t value = current_line.substr(pos + del_len); - // Map the entry. - mapped_environment[key] = value; - } - // Jump to next line in the environment map. - variable_strings_ptr += std::wcslen(variable_strings_ptr) + 1; - } - // We're done with the old environment map buffer. - FreeEnvironmentStringsW(environment_strings_ptr); - - // Return the map. - return mapped_environment; - } - - // env_vector_t WindowsEnvironmentVectorFromMap(const env_map_t &source_map) - // * Creates a vector buffer for the new environment string table - // * Copies in the mapped variables - // * Returns the vector - inline env_vector_t WindowsEnvironmentVectorFromMap(const env_map_t &source_map) - { - // Make a new environment map buffer. - env_vector_t environment_map_buffer; - // Give it some space. - environment_map_buffer.reserve(4096); - - // And fill'er up. - for(auto kv: source_map){ - // Create the line - env_string_t current_line(kv.first); current_line += L"="; current_line += kv.second; - // Add the line to the buffer. - std::copy(current_line.begin(), current_line.end(), std::back_inserter(environment_map_buffer)); - // Append a null - environment_map_buffer.push_back(0); - } - // Append one last null because of how Windows does it's environment maps. - environment_map_buffer.push_back(0); - - return environment_map_buffer; - } - - // env_vector_t CreateUpdatedWindowsEnvironmentVector(const env_map_t &changes_map) - // * Merges host environment with new mapped variables - // * Creates and returns string vector based on map - inline env_vector_t CreateUpdatedWindowsEnvironmentVector(const env_map_t &changes_map){ - // Import the environment map - env_map_t environment_map = MapFromWindowsEnvironment(); - // Merge in the changes with overwrite - for(auto& it: changes_map) - { - environment_map[it.first] = it.second; - } - // Create a Windows-usable Environment Map Buffer - env_vector_t environment_map_strings_vector = WindowsEnvironmentVectorFromMap(environment_map); - - return environment_map_strings_vector; - } - #endif /*! @@ -427,26 +320,6 @@ namespace util } - /*! - * Function: join - * Parameters: - * [in] vec : Vector of strings which needs to be joined to form - * a single string with words separated by a separator char. - * [in] sep : String used to separate 2 words in the joined string. - * Default constructed to ' ' (space). - * [out] string: Joined string. - */ - static inline - std::string join(const std::vector& vec, - const std::string& sep = " ") - { - std::string res; - for (auto& elem : vec) res.append(elem + sep); - res.erase(--res.end()); - return res; - } - - #ifndef __USING_WINDOWS__ /*! * Function: set_clo_on_exec @@ -647,56 +520,6 @@ namespace util * ------------------------------- */ -/*! - * The buffer size of the stdin/stdout/stderr - * streams of the child process. - * Default value is 0. - */ -struct bufsize { - explicit bufsize(int sz): bufsiz(sz) {} - int bufsiz = 0; -}; - -/*! - * Option to defer spawning of the child process - * till `Popen::start_process` API is called. - * Default value is false. - */ -struct defer_spawn { - explicit defer_spawn(bool d): defer(d) {} - bool defer = false; -}; - -/*! - * Option to close all file descriptors - * when the child process is spawned. - * The close fd list does not include - * input/output/error if they are explicitly - * set as part of the Popen arguments. - * - * Default value is false. - */ -struct close_fds { - explicit close_fds(bool c): close_all(c) {} - bool close_all = false; -}; - -/*! - * Option to make the child process as the - * session leader and thus the process - * group leader. - * Default value is false. - */ -struct session_leader { - explicit session_leader(bool sl): leader_(sl) {} - bool leader_ = false; -}; - -struct shell { - explicit shell(bool s): shell_(s) {} - bool shell_ = false; -}; - /*! * Base class for all arguments involving string value. */ @@ -722,34 +545,6 @@ struct executable: string_arg executable(T&& arg): string_arg(std::forward(arg)) {} }; -/*! - * Option to set the current working directory - * of the spawned process. - * - * Eg: cwd{"/some/path/x"} - */ -struct cwd: string_arg -{ - template - cwd(T&& arg): string_arg(std::forward(arg)) {} -}; - -/*! - * Option to specify environment variables required by - * the spawned process. - * - * Eg: environment{{ {"K1", "V1"}, {"K2", "V2"},... }} - */ -struct environment -{ - environment(env_map_t&& env): - env_(std::move(env)) {} - explicit environment(const env_map_t& env): - env_(env) {} - env_map_t env_; -}; - - /*! * Used for redirecting input/output/error */ @@ -866,44 +661,6 @@ struct error int wr_ch_ = -1; }; -// Impoverished, meager, needy, truly needy -// version of type erasure to store function pointers -// needed to provide the functionality of preexec_func -// ATTN: Can be used only to execute functions with no -// arguments and returning void. -// Could have used more efficient methods, ofcourse, but -// that won't yield me the consistent syntax which I am -// aiming for. If you know, then please do let me know. - -class preexec_func -{ -public: - preexec_func() {} - - template - explicit preexec_func(Func f): holder_(new FuncHolder(std::move(f))) - {} - - void operator()() { - (*holder_)(); - } - -private: - struct HolderBase { - virtual void operator()() const = 0; - virtual ~HolderBase(){}; - }; - template - struct FuncHolder: HolderBase { - FuncHolder(T func): func_(std::move(func)) {} - void operator()() const override { func_(); } - // The function pointer/reference - T func_; - }; - - std::unique_ptr holder_ = nullptr; -}; - // ~~~~ End Popen Args ~~~~ @@ -987,17 +744,9 @@ struct ArgumentDeducer ArgumentDeducer(Popen* p): popen_(p) {} void set_option(executable&& exe); - void set_option(cwd&& cwdir); - void set_option(bufsize&& bsiz); - void set_option(environment&& env); - void set_option(defer_spawn&& defer); - void set_option(shell&& sh); void set_option(input&& inp); void set_option(output&& out); void set_option(error&& err); - void set_option(close_fds&& cfds); - void set_option(preexec_func&& prefunc); - void set_option(session_leader&& sleader); private: Popen* popen_ = nullptr; @@ -1148,9 +897,6 @@ class Streams HANDLE g_hChildStd_ERR_Wr = nullptr; #endif - // Buffer size for the IO streams - int bufsiz_ = 0; - // Pipes for communicating with child // Emulates stdin @@ -1180,9 +926,9 @@ class Streams * interface to the client. * * API's provided by the class: - * Popen({"cmd"}, output{..}, error{..}, cwd{..}, ....) + * Popen({"cmd"}, output{..}, error{..}, ....) * Command provided as a sequence. - * Popen("cmd arg1", output{..}, error{..}, cwd{..}, ....) + * Popen("cmd arg1", output{..}, error{..}, ....) * Command provided in a single string. * wait() - Wait for the child to exit. * retcode() - The return code of the exited child. @@ -1197,8 +943,6 @@ class Streams in case of redirection. See piping examples. * error() - Get the error channel/File pointer. Usually used in case of redirection. - * start_process() - Start the child process. Only to be used when - * `defer_spawn` option was provided in Popen constructor. */ class Popen { @@ -1216,7 +960,7 @@ class Popen // Setup the communication channels of the Popen class stream_.setup_comm_channels(); - if (!defer_process_start_) execute_process(); + execute_process(); } template @@ -1228,7 +972,7 @@ class Popen // Setup the communication channels of the Popen class stream_.setup_comm_channels(); - if (!defer_process_start_) execute_process(); + execute_process(); } template @@ -1239,12 +983,10 @@ class Popen // Setup the communication channels of the Popen class stream_.setup_comm_channels(); - if (!defer_process_start_) execute_process(); + execute_process(); } - void start_process() noexcept(false); - int pid() const noexcept { return child_pid_; } int retcode() const noexcept { return retcode_; } @@ -1314,16 +1056,7 @@ class Popen std::future cleanup_future_; #endif - bool defer_process_start_ = false; - bool close_fds_ = false; - bool has_preexec_fn_ = false; - bool shell_ = false; - bool session_leader_ = false; - std::string exe_name_; - std::string cwd_; - env_map_t env_; - preexec_func preexec_fn_; // Command in string format std::string args_; @@ -1358,20 +1091,6 @@ inline void Popen::populate_c_argv() cargv_.push_back(nullptr); } -inline void Popen::start_process() noexcept(false) -{ - // The process was started/tried to be started - // in the constructor itself. - // For explicitly calling this API to start the - // process, 'defer_spawn' argument must be set to - // true in the constructor. - if (!defer_process_start_) { - assert (0); - return; - } - execute_process(); -} - inline int Popen::wait() noexcept(false) { #ifdef __USING_WINDOWS__ @@ -1446,17 +1165,6 @@ inline int Popen::poll() noexcept(false) inline void Popen::execute_process() noexcept(false) { #ifdef __USING_WINDOWS__ - if (this->shell_) { - throw OSError("shell not currently supported on windows", 0); - } - - void* environment_string_table_ptr = nullptr; - env_vector_t environment_string_vector; - if(this->env_.size()){ - environment_string_vector = util::CreateUpdatedWindowsEnvironmentVector(env_); - environment_string_table_ptr = (void*)environment_string_vector.data(); - } - if (exe_name_.length()) { this->vargs_.insert(this->vargs_.begin(), this->exe_name_); this->populate_c_argv(); @@ -1502,8 +1210,8 @@ inline void Popen::execute_process() noexcept(false) NULL, // process security attributes NULL, // primary thread security attributes TRUE, // handles are inherited - creation_flags, // creation flags - environment_string_table_ptr, // use provided environment + creation_flags, // creation flags + NULL, // use parent's environment NULL, // use parent's current directory &siStartInfo, // STARTUPINFOW pointer &piProcInfo); // receives PROCESS_INFORMATION @@ -1542,14 +1250,6 @@ inline void Popen::execute_process() noexcept(false) int err_rd_pipe, err_wr_pipe; std::tie(err_rd_pipe, err_wr_pipe) = util::pipe_cloexec(); - if (shell_) { - auto new_cmd = util::join(vargs_); - vargs_.clear(); - vargs_.insert(vargs_.begin(), {"/bin/sh", "-c"}); - vargs_.push_back(new_cmd); - populate_c_argv(); - } - if (exe_name_.length()) { vargs_.insert(vargs_.begin(), exe_name_); populate_c_argv(); @@ -1616,30 +1316,6 @@ namespace detail { popen_->exe_name_ = std::move(exe.arg_value); } - inline void ArgumentDeducer::set_option(cwd&& cwdir) { - popen_->cwd_ = std::move(cwdir.arg_value); - } - - inline void ArgumentDeducer::set_option(bufsize&& bsiz) { - popen_->stream_.bufsiz_ = bsiz.bufsiz; - } - - inline void ArgumentDeducer::set_option(environment&& env) { - popen_->env_ = std::move(env.env_); - } - - inline void ArgumentDeducer::set_option(defer_spawn&& defer) { - popen_->defer_process_start_ = defer.defer; - } - - inline void ArgumentDeducer::set_option(shell&& sh) { - popen_->shell_ = sh.shell_; - } - - inline void ArgumentDeducer::set_option(session_leader&& sleader) { - popen_->session_leader_ = sleader.leader_; - } - inline void ArgumentDeducer::set_option(input&& inp) { if (inp.rd_ch_ != -1) popen_->stream_.read_from_parent_ = inp.rd_ch_; if (inp.wr_ch_ != -1) popen_->stream_.write_to_child_ = inp.wr_ch_; @@ -1662,15 +1338,6 @@ namespace detail { if (err.rd_ch_ != -1) popen_->stream_.err_read_ = err.rd_ch_; } - inline void ArgumentDeducer::set_option(close_fds&& cfds) { - popen_->close_fds_ = cfds.close_all; - } - - inline void ArgumentDeducer::set_option(preexec_func&& prefunc) { - popen_->preexec_fn_ = std::move(prefunc); - popen_->has_preexec_fn_ = true; - } - inline void Child::execute_child() { #ifndef __USING_WINDOWS__ @@ -1717,41 +1384,8 @@ namespace detail { if (stream.err_write_ != -1 && stream.err_write_ > 2) close(stream.err_write_); - // Close all the inherited fd's except the error write pipe - if (parent_->close_fds_) { - int max_fd = sysconf(_SC_OPEN_MAX); - if (max_fd == -1) throw OSError("sysconf failed", errno); - - for (int i = 3; i < max_fd; i++) { - if (i == err_wr_pipe_) continue; - close(i); - } - } - - // Change the working directory if provided - if (parent_->cwd_.length()) { - sys_ret = chdir(parent_->cwd_.c_str()); - if (sys_ret == -1) throw OSError("chdir failed", errno); - } - - if (parent_->has_preexec_fn_) { - parent_->preexec_fn_(); - } - - if (parent_->session_leader_) { - sys_ret = setsid(); - if (sys_ret == -1) throw OSError("setsid failed", errno); - } - // Replace the current image with the executable - if (parent_->env_.size()) { - for (auto& kv : parent_->env_) { - setenv(kv.first.c_str(), kv.second.c_str(), 1); - } - sys_ret = execvp(parent_->exe_name_.c_str(), parent_->cargv_.data()); - } else { - sys_ret = execvp(parent_->exe_name_.c_str(), parent_->cargv_.data()); - } + sys_ret = execvp(parent_->exe_name_.c_str(), parent_->cargv_.data()); if (sys_ret == -1) throw OSError("execve failed", errno); @@ -1794,16 +1428,7 @@ namespace detail { for (auto& h : handles) { if (h == nullptr) continue; - switch (bufsiz_) { - case 0: - setvbuf(h, nullptr, _IONBF, BUFSIZ); - break; - case 1: - setvbuf(h, nullptr, _IONBF, BUFSIZ); - break; - default: - setvbuf(h, nullptr, _IOFBF, bufsiz_); - }; + setvbuf(h, nullptr, _IONBF, BUFSIZ); } #endif }