Repository: bitcoin/bitcoin (language: cpp)

Issue:
util-{util,wallet}: undefined reference to `evhttp_uridecode'
```
...
  CXXLD    libbitcoinconsensus.la
  AR       libbitcoin_util.a
  CXXLD    bitcoin-cli
  CXXLD    bitcoin-tx
  CXXLD    bitcoin-util
/usr/bin/ld: libbitcoin_common.a(libbitcoin_common_a-url.o): in function `urlDecode(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)':
/home/hassen/Workspace/OpenSource/bitcoin/src/common/url.cpp:15:(.text+0x61): undefined reference to `evhttp_uridecode'
/usr/bin/ld: libbitcoin_common.a(libbitcoin_common_a-url.o): in function `urlDecode(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)':
/home/hassen/Workspace/OpenSource/bitcoin/src/common/url.cpp:15:(.text+0x61): undefined reference to `evhttp_uridecode'
collect2: error: ld returned 1 exit status
make[2]: *** [Makefile:7407: bitcoin-util] Error 1
make[2]: *** Waiting for unfinished jobs....
collect2: error: ld returned 1 exit status
make[2]: *** [Makefile:7403: bitcoin-tx] Error 1
make[2]: Leaving directory '/home/hassen/Workspace/OpenSource/bitcoin/src'
make[1]: *** [Makefile:20138: all-recursive] Error 1
make[1]: Leaving directory '/home/hassen/Workspace/OpenSource/bitcoin/src'
make: *** [Makefile:814: all-recursive] Error 1
```

https://bitcoin.stackexchange.com/questions/121257/build-phase-fails-with-undefined-reference-to-evhttp-uridecode/122293#122293



Discussion / hints:
What are the exact steps to reproduce on a fresh install? I tried on mantic with the steps provided and it passed:

```
export DEBIAN_FRONTEND=noninteractive && apt update && apt install git vim ccache -y && git clone https://github.com/bitcoin/bitcoin.git --branch=26.x  --depth=1 ./bitcoin-core && cd bitcoin-core && apt install build-essential libtool autotools-dev automake pkg-config bsdmainutils python3-zmq systemtap-sdt-dev     libevent-dev libboost-dev   -y   &&  ./autogen.sh && ./configure --disable-bench --disable-fuzz-binary --enable-debug --without-gui             --enable-suppress-external-warnings --disable-wallet   && make -j $(nproc)
> https://bitcoin.stackexchange.com/questions/121257/build-phase-fails-with-undefined-reference-to-evhttp-uridecode/122293#122293

I can't reproduce the issue either.

> What are the exact steps to reproduce on a fresh install?

+1
`bitcoin-util` doesn't seem to be linked against libevent. It also doesn't actually use that function. Normally, the linker would drop unused object files, but somehow it doesn't in this user's case. It shouldn't be linking in that object (`common/url.o`) in the first place.

Fix patch:
diff --git a/configure.ac b/configure.ac
index febb352cdbe55..494cb3ce50d87 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1706,7 +1706,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_SSE42], [test "$enable_sse42" = "yes"])
 AM_CONDITIONAL([ENABLE_SSE41], [test "$enable_sse41" = "yes"])
diff --git a/src/Makefile.am b/src/Makefile.am
index 7673d2f54511c..7387eb1eaa39b 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -679,6 +679,7 @@ libbitcoin_common_a_SOURCES = \
   common/run_command.cpp \
   common/settings.cpp \
   common/system.cpp \
+  common/url.cpp \
   compressor.cpp \
   core_read.cpp \
   core_write.cpp \
@@ -711,11 +712,6 @@ libbitcoin_common_a_SOURCES = \
   script/solver.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/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index 129deeec600ea..830171f63a669 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -11,7 +11,6 @@
 #include <clientversion.h>
 #include <common/args.h>
 #include <common/system.h>
-#include <common/url.h>
 #include <compat/compat.h>
 #include <compat/stdin.h>
 #include <policy/feerate.h>
@@ -51,7 +50,6 @@
 using CliClock = std::chrono::system_clock;
 
 const std::function<std::string(const char*)> 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 d5dfbbec2713a..fe90958a5f5a2 100644
--- a/src/bitcoin-wallet.cpp
+++ b/src/bitcoin-wallet.cpp
@@ -11,7 +11,6 @@
 #include <clientversion.h>
 #include <common/args.h>
 #include <common/system.h>
-#include <common/url.h>
 #include <compat/compat.h>
 #include <interfaces/init.h>
 #include <key.h>
@@ -28,7 +27,6 @@
 #include <tuple>
 
 const std::function<std::string(const char*)> 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 4f0a816388e7f..54796c5abbfc2 100644
--- a/src/bitcoind.cpp
+++ b/src/bitcoind.cpp
@@ -12,7 +12,6 @@
 #include <common/args.h>
 #include <common/init.h>
 #include <common/system.h>
-#include <common/url.h>
 #include <compat/compat.h>
 #include <init.h>
 #include <interfaces/chain.h>
@@ -35,7 +34,6 @@
 using node::NodeContext;
 
 const std::function<std::string(const char*)> 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 053e1a825c55b..ecf88d07ea548 100644
--- a/src/common/url.cpp
+++ b/src/common/url.cpp
@@ -4,19 +4,36 @@
 
 #include <common/url.h>
 
-#include <event2/http.h>
-
-#include <cstdlib>
+#include <charconv>
 #include <string>
+#include <string_view>
+#include <system_error>
 
-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<char>(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 b16b8241af569..203f41c70f2d2 100644
--- a/src/common/url.h
+++ b/src/common/url.h
@@ -6,9 +6,12 @@
 #define BITCOIN_COMMON_URL_H
 
 #include <string>
+#include <string_view>
 
-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 ded057dbfafae..16befd99e8942 100644
--- a/src/qt/main.cpp
+++ b/src/qt/main.cpp
@@ -4,7 +4,6 @@
 
 #include <qt/bitcoin.h>
 
-#include <common/url.h>
 #include <compat/compat.h>
 #include <util/translation.h>
 
@@ -17,7 +16,6 @@
 extern const std::function<std::string(const char*)> G_TRANSLATION_FUN = [](const char* psz) {
     return QCoreApplication::translate("bitcoin-core", psz).toStdString();
 };
-UrlDecodeFn* const URL_DECODE = urlDecode;
 
 const std::function<std::string()> G_TEST_GET_FULL_NAME{};
 
diff --git a/src/wallet/rpc/util.cpp b/src/wallet/rpc/util.cpp
index 06ec7db1bc219..1252843e9d0c3 100644
--- a/src/wallet/rpc/util.cpp
+++ b/src/wallet/rpc/util.cpp
@@ -11,6 +11,7 @@
 #include <wallet/context.h>
 #include <wallet/wallet.h>
 
+#include <string_view>
 #include <univalue.h>
 
 #include <boost/date_time/posix_time/posix_time.hpp>
@@ -61,9 +62,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;


Test patch:
diff --git a/src/Makefile.test.include b/src/Makefile.test.include
index cf88a02b95f06..942e0bf69b34c 100644
--- a/src/Makefile.test.include
+++ b/src/Makefile.test.include
@@ -85,6 +85,7 @@ BITCOIN_TESTS =\
   test/checkqueue_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/test/common_url_tests.cpp b/src/test/common_url_tests.cpp
new file mode 100644
index 0000000000000..cc893cbed7245
--- /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 <common/url.h>
+
+#include <string>
+
+#include <boost/test/unit_test.hpp>
+
+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 e81efac6e0d7f..631da13803abd 100644
--- a/src/test/fuzz/string.cpp
+++ b/src/test/fuzz/string.cpp
@@ -90,7 +90,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 2c18184261e48..38350b33cc5e0 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -14,7 +14,6 @@
 #include <banman.h>
 #include <chainparams.h>
 #include <common/system.h>
-#include <common/url.h>
 #include <consensus/consensus.h>
 #include <consensus/params.h>
 #include <consensus/validation.h>
@@ -81,7 +80,6 @@ using node::RegenerateCommitments;
 using node::VerifyLoadedChainstate;
 
 const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
-UrlDecodeFn* const URL_DECODE = nullptr;
 
 /** Random context to get unique temp data dirs. Separate from g_insecure_rand_ctx, which can be seeded from a const env var */
 static FastRandomContext g_insecure_rand_ctx_temp_path;