rpc: Sanitize label name in various RPCs with tests #26186

Given that all the rpc methods are at the same indentation and thus easily readable/enumerable, I don't think we need the comments here.

Even though for the current implementation it doesn't reduce LoC that much, I think this alternative would be significantly more compact if in the future we'd also need to check for multiple invalid labels (not just '*'). But even in the current format, I think it's a bit more DRY?

        address = node.getnewaddress()
        pubkey = node.getaddressinfo(address)['pubkey']
        rpc_calls = [
            [node.getnewaddress],
            [node.setlabel, address],
            [node.getaddressesbylabel],
            [node.importpubkey, pubkey],
            [node.addmultisigaddress, 1, [pubkey]],
            [node.getreceivedbylabel],
        ]
        for rpc_call in rpc_calls:
            assert_raises_rpc_error(-11, "Invalid label name", *rpc_call, "*")

I'd just move this out of the if/else loop and remove the duplicated lines in the if statement?

            strLabel = LabelFromValue(request.params[1]);

        strLabel = LabelFromValue(request.params[1]);

I don't think this line adds much, it's not the expected type of r and it's not necessary to declare it before the if/else. I'd prefer to remove, not really pythonic.

nit: if you move the entire if/else loop to before the execution of the rpc calls, you could replace this with rpc_calls.append(...)

nit: PEP8 suggests a single whiteline

nit: would avoid using one-letter variables, the brevity isn't necessary here imo and it's just slightly more confusing

            response = node.importdescriptors([{

nit: f-string

                'desc': f'pkh({address})',

What about moving the null check into LabelFromValue (if needed with an optional arg to set the default value to something other than "")?

What do you think of 5b98f5a2aeb0d49ab5c72732acb4ae0d00f907c7?

Looks like listsinceblock ought to be added to this test if #25934 is merged. Edit: should the wildcard functionality be added either in that PR or this one?

answered here: #25934 (review)

in b0fdad3436083aa8b965db374b7e97c94e6e246c: LabelFromValue() returns by value (as opposed to get_str() indeed return a ref to the data object, so having label be a const std::string& is quite risky from a dangling reference perspective. I would update label to be a std::string here (and similar cases further down) - even though it's changed to an auto in later commits.

Since AddrBookFilter takes a std::optional<std::string>, I think there's no need for the CHECK_NONFATAL here? Wouldn't be any behaviour change either I think, because the existing code would throw if params[0] were to be null? (not 100% sure on that)

        addresses = wallet.ListAddrBookAddresses(CWallet::AddrBookFilter{LabelFromValue(params[0])});

Missing #include <util/check.h> here and in other places.

Since the asterisk is added in the for-loop below, there is no need to add it here. I'd suggest to also add test coverage for importaddress here:

            rpc_calls.extend([
                [node.importprivkey, node.dumpprivkey(address)],
                [node.importaddress, address],
            ])

Thanks done

Conceptually there isn't a default label, the record label is just empty.

What if you return the empty string from LabelFromValue instead? It will also remove the need for all the .value_or(DEFAULT_WALLET_LABEL);.

Some RPCs require the label to be optional for filtering. getaddressesbylabel will return all addresses if the label is nullopt for example. I could hard-code the "" in .value_or calls if you prefer though.

Maybe you are referring to listsinceblock? (as far as I can see, it's the only place where the label is used as optional), getaddressesbylabel uses a string not an optional.

Check this out: https://github.com/furszy/bitcoin-core/commit/8ce2d8965537a889fe88ca1a891763b35aba32ad. (If you like it, feel free to squash it on the first commit).

The CI doesn't pass on your commit because of listsinceblock. A label can be empty in this RPC.

https://cirrus-ci.com/task/6056876765347840?logs=ci#L3626

will check it tonight 👍🏼. I coded a quick example. Shouldn't be hard to correct that single case.

Ready, fix was simple: https://github.com/furszy/bitcoin-core/commit/f8da21cb6eec0d62b2d3ed766201aca182b47342.

nit: list initialization (in many other places too)

    auto label{LabelFromValue(request.params[0]).value_or(DEFAULT_WALLET_LABEL)};

nit: missing optional and string includes

nit: could avoid multiline ternary this way:

    std::optional<std::string> filter_label;
    if (!request.params[5].isNull()) filter_label.emplace(LabelFromValue(request.params[5]));

This refactor introduces quite a few string copy operations, where previously references were passed. I think this can be quite easily avoided with the below patch. There are other callsites of LabelFromValue that would benefit from just changing their type to const std::string&, but to keep LoC change minimal I've not changed them in this patch.

<details> <summary>git diff on ba1e78a2d58059f439f17e821eb2d1bab4db86cb</summary>

diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp
index e819c09c5..5d04d3113 100644
--- a/src/wallet/rpc/addresses.cpp
+++ b/src/wallet/rpc/addresses.cpp
@@ -43,7 +43,7 @@ RPCHelpMan getnewaddress()
     }
 
     // Parse the label first so we don't generate a key if there's an error
-    std::string label{LabelFromValue(request.params[0])};
+    const std::string& label{LabelFromValue(request.params[0])};
 
     OutputType output_type = pwallet->m_default_address_type;
     if (!request.params[1].isNull()) {
@@ -256,7 +256,7 @@ RPCHelpMan addmultisigaddress()
 
     LOCK2(pwallet->cs_wallet, spk_man.cs_KeyStore);
 
-    std::string label{LabelFromValue(request.params[2])};
+    const std::string& label{LabelFromValue(request.params[2])};
 
     int required = request.params[0].getInt<int>();
 
diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
index 1abb2efda..7e5fbfae0 100644
--- a/src/wallet/rpc/backup.cpp
+++ b/src/wallet/rpc/backup.cpp
@@ -140,7 +140,7 @@ RPCHelpMan importprivkey()
         EnsureWalletIsUnlocked(*pwallet);
 
         std::string strSecret = request.params[0].get_str();
-        std::string strLabel{LabelFromValue(request.params[1])};
+        const std::string& strLabel{LabelFromValue(request.params[1])};
 
         // Whether to perform rescan after import
         if (!request.params[2].isNull())
@@ -232,7 +232,7 @@ RPCHelpMan importaddress()
     EnsureLegacyScriptPubKeyMan(*pwallet, true);
 
     // Parse label
-    std::string strLabel{LabelFromValue(request.params[1])};
+    const std::string& strLabel{LabelFromValue(request.params[1])};
 
     // Whether to perform rescan after import
     bool fRescan = true;
@@ -423,7 +423,7 @@ RPCHelpMan importpubkey()
 
     EnsureLegacyScriptPubKeyMan(*pwallet, true);
 
-    std::string strLabel{LabelFromValue(request.params[1])};
+    const std::string& strLabel{LabelFromValue(request.params[1])};
 
     // Whether to perform rescan after import
     bool fRescan = true;
@@ -1158,7 +1158,7 @@ static UniValue ProcessImport(CWallet& wallet, const UniValue& data, const int64
         if (internal && data.exists("label")) {
             throw JSONRPCError(RPC_INVALID_PARAMETER, "Internal addresses should not have a label");
         }
-        std::string label{LabelFromValue(data["label"])};
+        const std::string& label{LabelFromValue(data["label"])};
         const bool add_keypool = data.exists("keypool") ? data["keypool"].get_bool() : false;
 
         // Add to keypool only works with privkeys disabled
@@ -1452,7 +1452,7 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c
         const std::string& descriptor = data["desc"].get_str();
         const bool active = data.exists("active") ? data["active"].get_bool() : false;
         const bool internal = data.exists("internal") ? data["internal"].get_bool() : false;
-        std::string label{LabelFromValue(data["label"])};
+        const std::string& label{LabelFromValue(data["label"])};
 
         // Parse descriptor string
         FlatSigningProvider keys;
diff --git a/src/wallet/rpc/transactions.cpp b/src/wallet/rpc/transactions.cpp
index 7088ff4ab..9b03782d4 100644
--- a/src/wallet/rpc/transactions.cpp
+++ b/src/wallet/rpc/transactions.cpp
@@ -636,8 +636,8 @@ RPCHelpMan listsinceblock()
     bool include_change = (!request.params[4].isNull() && request.params[4].get_bool());
 
     // Only set it if 'label' was provided.
-    std::optional<std::string> filter_label = request.params[5].isNull() ? std::nullopt :
-                                              std::optional(LabelFromValue(request.params[5]));
+    const std::optional<std::string>& filter_label = request.params[5].isNull() ? std::nullopt :
+                                                     std::optional{LabelFromValue(request.params[5])};
 
     int depth = height ? wallet.GetLastBlockHeight() + 1 - *height : -1;
 
diff --git a/src/wallet/rpc/util.cpp b/src/wallet/rpc/util.cpp
index 1fdb80630..bc0c284dc 100644
--- a/src/wallet/rpc/util.cpp
+++ b/src/wallet/rpc/util.cpp
@@ -130,11 +130,12 @@ const LegacyScriptPubKeyMan& EnsureConstLegacyScriptPubKeyMan(const CWallet& wal
     return *spk_man;
 }
 
-std::string LabelFromValue(const UniValue& value)
+const std::string& LabelFromValue(const UniValue& value)
 {
-    if (value.isNull()) return ""; // empty
+    static std::string empty_string;
+    if (value.isNull()) return empty_string;
 
-    std::string label = value.get_str();
+    const std::string& label{value.get_str()};
     if (label == "*")
         throw JSONRPCError(RPC_WALLET_INVALID_LABEL_NAME, "Invalid label name");
     return label;
diff --git a/src/wallet/rpc/util.h b/src/wallet/rpc/util.h
index 87d34f7c1..9dd101167 100644
--- a/src/wallet/rpc/util.h
+++ b/src/wallet/rpc/util.h
@@ -5,6 +5,7 @@
 #ifndef BITCOIN_WALLET_RPC_UTIL_H
 #define BITCOIN_WALLET_RPC_UTIL_H
 
+#include <attributes.h>
 #include <script/script.h>
 
 #include <any>
@@ -40,7 +41,7 @@ const LegacyScriptPubKeyMan& EnsureConstLegacyScriptPubKeyMan(const CWallet& wal
 
 bool GetAvoidReuseFlag(const CWallet& wallet, const UniValue& param);
 bool ParseIncludeWatchonly(const UniValue& include_watchonly, const CWallet& wallet);
-std::string LabelFromValue(const UniValue& value);
+const std::string& LabelFromValue(const UniValue& value LIFETIMEBOUND);
 //! Fetch parent descriptors of this scriptPubKey.
 void PushParentDescriptors(const CWallet& wallet, const CScript& script_pubkey, UniValue& entry);

</details>

Guess that you wanted to set LIFETIMEBOUND for the return type instead of the function's argument right?.

Still, would return the plain string and not a reference. There is no need to overcomplicate code for address book labels.

Thanks, removed the reference.

Guess that you wanted to set LIFETIMEBOUND for the return type instead of the function's argument right?

The return value of LabelFromValue was bound to the lifetime of const UniValue& value so the attribute was correct I think? See https://clang.llvm.org/docs/AttributeReference.html#lifetimebound

There is no need to overcomplicate code for address book labels.

I'm honestly not sure why this got reverted. It introduces almost no additional complexity in the code, and callsites can now choose if they wish to have LabelFromValue return by value or by reference. Even though wallet labels aren't expected to be huge strings, I don't see the downside of doing it this way? There's no point bikeshedding over this, we won't be able to measure the performance impact, but it just seems like the more proper way of doing things, imo. Up to you, @aureleoules.

I'm honestly not sure why this got reverted. It introduces almost no additional complexity in the code, and callsites can now choose if they wish to have LabelFromValue return by value or by reference. Even though wallet labels aren't expected to be huge strings, I don't see the downside of doing it this way? There's no point bikeshedding over this, we won't be able to measure the performance impact, but it just seems like the more proper way of doing things, imo. Up to you, @aureleoules.

I'm actually not really happy with the revert neither but for another reason.

But first, my point above was to take advantage of the compiler's rvo, which is designed for this kind of stuff.

Now, the reason why I'm not particularly happy with the latest changes is the use of the auto keyword alone (which involves a string copy). Instead, should receive the function's output into a const ref string. @aureleoules.

Thoughts @stickies-v?

Is this ok with you? https://github.com/bitcoin/bitcoin/compare/cce2e872e1a8b7c5b80036f48f1f35467b9328ff..b119bb84250f5b97053314f730aefb8c10b4867a

Have spoken with @stickies-v further and he made me notice that the string is not initialized inside LabelFromValue, the univalue get_str method returns a const reference.. so the compiler's rvo argument does not apply.

So all good for me now, I'm aligned on returning the const ref string from LabelFromValue and adding the lifetime bound attribute once more. just need to re-add the following line:

const std::string& LabelFromValue(const UniValue& value LIFETIMEBOUND);

Can be marked as resolved, as furszy mentioned we're all in agreement here and I'm okay with the current implementation 👍

Changing this to return a reference doesn't seem very safe -- in that case calling string& label = LabelFromValue(data["label"]) will mean label can become invalid if data gets changed (eg, if additional calls to data.pushKV() are made, the data.values vector may be reallocated, invalidating the reference), which is not something LIFETIMEBOUND can check.

As far as I can see we're talking one copy of each fairly short label per RPC call here, which isn't something that needs to be aggressively optimised. So seems much better to me to just keep the implicit copy of the string and have a simple API that minimises the risk of errors creeping in.

That's a great point I hadn't considered. Even though the data["label"] UniValue::VSTR object that we bind to doesn't use a vector to store its string value and would seem safe at first, data["label"] returns a reference to the data.values vector - so any changes to that parent UniValue::VOBJ that we can't control for here are potentially dangerous.

I agree that this could lead to nasty issues and isn't worth the overhead. Sorry for causing the back and forth on this @aureleoules. The UniValue type can be a bit tricky to reason about, it seems.

Done https://github.com/bitcoin/bitcoin/compare/408e375d15e327b2f5886f395dbc23a4834460e5..632202c5b7bb5fb975f8bde74b6085772b142c5b

    static const std::string empty_string;

nit

        filter_label.emplace(LabelFromValue(request.params[0]));

Done

In 7d2d0c4d: is this include intended?

Removed thanks

nit: not needed anymore now that we're returning a string copy

    if (value.isNull()) return "";

Or return {};

In 65e78bda7cd23ad7b6ede63c6bf5ffe6f552c71d "test: Invalid label name coverage"

importpubkey is a legacy wallet only RPC. The reason this test still passes with --descriptors is because the test framework has this (and some of the other import RPCs) aliased to importdescriptors.

Good to know. But considering this PR has 4 ACKs, is it worth the change? The test is executed with both --legacy-wallet and --descriptors anyway.