refactor: Use our own implementation of urlDecode

[fjahr]

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 we were using in urlDecode could be easily emulated in fewer LOC. This also ports the applicable test vectors over from libevent.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	theStack, maflcko, stickies-v, achow101
Concept NACK	luke-jr
Concept ACK	TheCharlatan
Approach ACK	hebasto
Stale ACK	Sjors, laanwj

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

[laanwj]

Concept ACK
yes please... the hooking we do for URL_DECODE and UrlDecodeFn made me kind of scared, to be honest (in that context), i think can we get rid of that at the same time?

This will also fix @luke-jr's #29654.

[TheCharlatan]

Concept ACK

[stickies-v]

Concept ACK

[hebasto]

Concept ACK.

[stickies-v]

Approach ACK

Since urlDecode no longer needs libevent, we can clean up the workaround introduced in #18504 too (edit, oh that's basically what @laanwj said a few comments up):

git diff on 2447bc7

diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index 129deeec60..830171f63a 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 d5dfbbec27..fe90958a5f 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 4f0a816388..54796c5abb 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.h b/src/common/url.h
index b16b8241af..0283f471bc 100644
--- a/src/common/url.h
+++ b/src/common/url.h
@@ -7,8 +7,6 @@
 
 #include <string>
 
-using UrlDecodeFn = std::string(const std::string& url_encoded);
-UrlDecodeFn urlDecode;
-extern UrlDecodeFn* const URL_DECODE;
+std::string urlDecode(const std::string& urlEncoded);
 
 #endif // BITCOIN_COMMON_URL_H
diff --git a/src/qt/main.cpp b/src/qt/main.cpp
index ded057dbfa..16befd99e8 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/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index 2c18184261..38350b33cc 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;
diff --git a/src/wallet/rpc/util.cpp b/src/wallet/rpc/util.cpp
index 06ec7db1bc..4e93b0eac5 100644
--- a/src/wallet/rpc/util.cpp
+++ b/src/wallet/rpc/util.cpp
@@ -61,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.substr(0, WALLET_ENDPOINT_BASE.size()) == WALLET_ENDPOINT_BASE) {
         // wallet endpoint was used
-        wallet_name = URL_DECODE(request.URI.substr(WALLET_ENDPOINT_BASE.size()));
+        wallet_name = urlDecode(request.URI.substr(WALLET_ENDPOINT_BASE.size()));
         return true;
     }
     return false;

nit: Would then update the fn signature to std::string UrlDecode(const std::string& url_encoded)

[stickies-v]

What's the rationale for continuing here? Should we accept invalid URLs? At first sight, I think I'd prefer having this return a util::Result and have the function fail in case of missing hex digits or invalid hex?

[fjahr]

I also found this a bit strange but it's the behavior of libevent and they even have an explicit test case for this (which failed when I wrote my initial implementation). I didn't investigate this further yet but I think the safest option is to keep this a pure refactor for now.

[laanwj]

For better or worse, this silent accepting behavior for invalid hex values in URL decoding is really common. See for example Python:

>>> import urllib.parse
>>> urllib.parse.unquote('%12%%xx%0a')
'\x12%%xx\n'

It's the web philopsophy of "in case of doubt, just do something!" 😅 . Anyhow, in this refactor it doesn't make sense imo to add error handling here.

[fjahr]

Addressed @laanwj 's comment and removed the hooking.

FWIW, I wondered for a moment why this is in common and not util but that was a conscious decision, see #26294.

yes please... the hooking we do for URL_DECODE and UrlDecodeFn made me kind of scared, to be honest (in that context), i think can we get rid of that at the same time?

Absolutely, I hadn't even seen that yet. For reviewers that wonder as well, the point of this was to not have the wallet depend on libevent, see #18504.

This will also fix @luke-jr's #29654.

Nice, I also missed that one!

[fjahr]

Addressed @stickies-v 's feedback as well, thanks a lot!

[Sjors]

ACK 68bf6c8

Link to the code as of v2.2.12 which we use in depends:

• evhttp_uridecode: https://github.com/libevent/libevent/blob/release-2.1.12-stable/http.c#L3225-L3246
• evhttp_decode_uri_internal: https://github.com/libevent/libevent/blob/release-2.1.12-stable/http.c#L3169-L3205

I was also happy to see that test/fuzz/string.cpp already covers this function.

[Sjors]

Was this doing anything, or did we forget to remove it earlier?

[fjahr]

Not sure if I understand your question, do you mean this line in particular or the whole hooking thing? This line was added the with the rest of hooking code in #18504 and I don't think anything was changed about that until now, so I don't know where we might have forgotten to remove it.

[fjahr]

Addressed feedback by @paplorinc and @Sjors , thanks a lot!

I also reorganized the commits a bit because they were doing more than one thing and it was hard to address feedback cleanly. The tests are now split from the implementation, which makes it easier to cherry-pick the unit test commit and run it against master. And I split off the renaming into a scripted diff.

[fjahr]

CI failure can be ignored, it was introduced by #29827 and should hopefully be fixed by #29926

[luke-jr]

Concept NACK. Embedding copies of shared code just makes things more fragile, not safer. It's far more likely there's a bugfix that we miss picking up on, than that there's malicious code inserted later.

[laanwj]

It's far more likely there's a bugfix that we miss picking up on, than that there's malicious code inserted later.

Sometimes i'd agree with that reasoning. But let's face it, urlDecode is not more complex than say, leftpad, it's unlikely to need maintainance or bug fixes, and thus seems unnecessary to carry a dependency for.

(Also this isn't a direct copy of the C code! It's a C++ reimplementation that does std::string to std::string, so should handle embedded NULL bytes without truncation)

[fjahr]

Embedding copies of shared code just makes things more fragile, not safer. It's far more likely there's a bugfix that we miss picking up on, than that there's malicious code inserted later.

In addition to what @laanwj said, it's also a much simpler re-implementation. The libevent function provided further options that we didn't care about and hardcoded, that's how we could simplify. And as was discussed in the last IRC meeting, there is also interest in removing libevent in general, at which point this would be a necessary change anyway. We are also removing that hooking code which is a big improvement in itself. If you subtract the test code, we are almost removing as much code as we are adding.

It's a C++ reimplementation that does std::string to std::string, so should handle embedded NULL bytes without truncation

So, to be clear, your opinion is for this part I should remove the patch and introduce a change of behavior, right?

[fjahr]

unsigned may be better

No idea what this means ;) but I addressed the feedback, thanks a lot!

[theStack]

Code-review ACK 992c714

[maflcko]

Did some differential fuzzing, as this is an ideal fit for it.

ACK 992c714 👈

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 992c714451676cee33d3dff49f36329423270c1c 👈
uoUAes8qa1RR+FiK2t8YFFjOnn0fR1ewcF3JZcxlbFYRLH099vwrEWYq5uCUNGpAtidxMgd/deCwvPSTZbvgDQ==

[maflcko]

Some more tests:

%%ffg
%fg

[fjahr]

thanks, I will add them when I have to retouch, if not I will make a small follow-up

[maflcko]

https://github.com/bitcoin/bitcoin/pull/29967/files

[paplorinc]

I think we should document at least that we thought of the %00 corner case, e.g. via a test case or a change. (already added)

I have verified that the behavior is otherwise the same before and after via:

std::string GenerateRandomString() {
    std::random_device rd;
    std::mt19937 gen(rd());
    std::uniform_int_distribution<> length_dis(1, 10);
    std::uniform_int_distribution<> char_dis(1, 255);

    size_t length = length_dis(gen);
    std::string res;
    res.reserve(length);

    for (size_t i = 0; i < length; ++i) {
        char c = static_cast<char>(char_dis(gen));
        res.push_back(c);
    }

    return res;
}

std::string urlDecode_old(const std::string &urlEncoded) {
    std::string res;
    if (!urlEncoded.empty()) {
        char *decoded = evhttp_uridecode(urlEncoded.c_str(), false, nullptr);
        if (decoded) {
            res = std::string(decoded);
            free(decoded);
        }
    }
    return res;
}

BOOST_AUTO_TEST_CASE(randomized_url_decode_length_test) {
    for (int i = 0; i < 100'000'000; ++i) {
        std::string random_encoded = GenerateRandomString();

        std::string result1 = urlDecode_old(random_encoded);
        std::string result2 = urlDecode(random_encoded);

        if (result1 != result2) {
            std::cerr
                << "urlDecode discrepancy found, random_encoded = `" + random_encoded + "`, urlDecode_old = `" +
                   result1 + "`, urlDecode (new) = `" + result2 + "`" << std::endl;
        }

        BOOST_CHECK_LE(result1.size(), random_encoded.size());
    }
}

[paplorinc]

There's still one sneaky difference that I've found: %00.
Previously this prematurely terminated the string, now it's kept:

BOOST_CHECK_EQUAL(urlDecode("a%00b"), std::string("a\u0000b", 3));

this failed previously, passes now.

[stickies-v]

ACK 992c714

nit: definitely not worth more back-and-forth, but I think (untested) maflcko's suggestion should work with MSVC when explicitly converting iterators to pointers:

git diff on 992c714

diff --git a/src/common/url.cpp b/src/common/url.cpp
index ecf88d07ea..c901ecb5dc 100644
--- a/src/common/url.cpp
+++ b/src/common/url.cpp
@@ -14,25 +14,24 @@ std::string UrlDecode(std::string_view url_encoded)
     std::string res;
     res.reserve(url_encoded.size());
 
-    for (size_t i = 0; i < url_encoded.size(); ++i) {
-        char c = url_encoded[i];
+    for (auto it = url_encoded.begin(); it < url_encoded.end(); ++it) {
         // 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()) {
+        if (*it == '%' && std::distance(it, url_encoded.end()) > 2) {
             unsigned int decoded_value{0};
-            auto [p, ec] = std::from_chars(url_encoded.data() + i + 1, url_encoded.data() + i + 3, decoded_value, 16);
+            auto [p, ec] = std::from_chars(&*(it + 1), &*(it + 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) {
+            if (ec == std::errc{} && p == &*(it + 3)) {
                 res += static_cast<char>(decoded_value);
                 // Next two characters are part of the percent encoding
-                i += 2;
+                it += 2;
                 continue;
             }
             // In case of invalid percent encoding, add the '%' and continue
         }
-        res += c;
+        res += *it;
     }
 
     return res;

[fjahr]

I think (untested) maflcko's suggestion should work with MSVC when explicitly converting iterators to pointers

Yes, it's certainly possible and I briefly explored it myself, but it didn't seem like much of an improvement over the current version anymore. And the current version had already received some review at that point, so I decided it would be better to just revert since it was a style-nit anyway.

[achow101]

ACK 992c714

[ryanofsky]

I went ahead and merged this, despite NACK from luke #29904 (comment) about costs of forking shared code. It seems like the NACK was adequately responded to, with responses acknowledging the tradeoff and giving reasons why it seemed justified in this case (even fixing a bug luke reported #29654)

[hebasto]

Ported to the CMake-based build system in hebasto#181.