Merge bitcoin/bitcoin#34230: fuzz: Reject too large descriptor leaf sizes in scriptpubkeyman target

fa8d56f9f0 fuzz: Reject too large descriptor leaf sizes in scriptpubkeyman target (MarcoFalke)
fabac1b395 fuzz: Reject some more "expensive" descriptors in the scriptpubkeyman target (MarcoFalke)
333333356f fuzz: [refactor] Use std::span over FuzzBufferType in descriptor utils (MarcoFalke)

Pull request description:

  Accepting "expensive" fuzz inputs which have no real use-case is problematic, because it prevents the fuzz engine from spending time on the next useful fuzz input.

  Also, this may lead to problems, where the fuzz target can not be run at all on some platforms. See https://github.com/bitcoin/bitcoin/issues/34110.

  Fixes https://github.com/bitcoin/bitcoin/issues/34110 by rejecting those useless and expensive inputs (via the third commit)

  Can be tested by running the input and checking the time before and after the changes here:

  ```
  curl -fLO '1cf91e0c6b'
  FUZZ=scriptpubkeyman time ./bld-cmake/bin/fuzz ./1cf91e0c6bfff9dafcd4db5b0ba36b1e906f4cf5
  ```

  Also, the second commit fixes https://github.com/bitcoin/bitcoin/issues/31066.

ACKs for top commit:
  brunoerg:
    code review ACK fa8d56f9f0
  marcofleon:
    ACK fa8d56f9f0
  sipa:
    ACK fa8d56f9f0

Tree-SHA512: e683cb89c3047358add438508c173f1cf647827bcadc3564ad42c757e4c99b8e9b777213fd38ebeb46f4c89a72363e0642f47435e20df3960eaeb5b8257dbd32

src/test/fuzz/util/descriptor.cpp

@@ -74,7 +74,7 @@ std::optional<std::string> MockedDescriptorConverter::GetDescriptor(std::string_
     return desc;
 }
 
-bool HasDeepDerivPath(const FuzzBufferType& buff, const int max_depth)
+bool HasDeepDerivPath(std::span<const uint8_t> buff, const int max_depth)
 {
     auto depth{0};
     for (const auto& ch: buff) {
@@ -88,7 +88,7 @@ bool HasDeepDerivPath(const FuzzBufferType& buff, const int max_depth)
     return false;
 }
 
-bool HasTooManySubFrag(const FuzzBufferType& buff, const int max_subs, const size_t max_nested_subs)
+bool HasTooManySubFrag(std::span<const uint8_t> buff, const int max_subs, const size_t max_nested_subs)
 {
     // We use a stack because there may be many nested sub-frags.
     std::stack<int> counts;
@@ -112,7 +112,7 @@ bool HasTooManySubFrag(const FuzzBufferType& buff, const int max_subs, const siz
     return false;
 }
 
-bool HasTooManyWrappers(const FuzzBufferType& buff, const int max_wrappers)
+bool HasTooManyWrappers(std::span<const uint8_t> buff, const int max_wrappers)
 {
     // The number of nested wrappers. Nested wrappers are always characters which follow each other so we don't have to
     // use a stack as we do above when counting the number of sub-fragments.
@@ -143,3 +143,23 @@ bool HasTooManyWrappers(const FuzzBufferType& buff, const int max_wrappers)
 
     return false;
 }
+
+bool HasTooLargeLeafSize(std::span<const uint8_t> buff, const uint32_t max_leaf_size)
+{
+    uint32_t leaf_len{0};
+    for (auto c : buff) {
+        if (c == '(' || c == ')' || c == ',' || c == '{' || c == '}') {
+            // Possibly start a fresh leaf, or a fresh function name (with
+            // wrappers), or terminate a prior leaf.
+            leaf_len = 0;
+        } else {
+            // Just treat everything else as a leaf. This will also reject long
+            // function names, but this should be fine if the max_leaf_size is
+            // set large enough.
+            if (++leaf_len > max_leaf_size) {
+                return true;
+            }
+        }
+    }
+    return false;
+}

src/test/fuzz/util/descriptor.h

@@ -53,7 +53,7 @@ constexpr int MAX_DEPTH{2};
  * Whether the buffer, if it represents a valid descriptor, contains a derivation path deeper than
  * a given maximum depth. Note this may also be hit for deriv paths in origins.
  */
-bool HasDeepDerivPath(const FuzzBufferType& buff, const int max_depth = MAX_DEPTH);
+bool HasDeepDerivPath(std::span<const uint8_t> buff, int max_depth = MAX_DEPTH);
 
 //! Default maximum number of sub-fragments.
 constexpr int MAX_SUBS{1'000};
@@ -64,8 +64,8 @@ constexpr size_t MAX_NESTED_SUBS{10'000};
  * Whether the buffer, if it represents a valid descriptor, contains a fragment with more
  * sub-fragments than the given maximum.
  */
-bool HasTooManySubFrag(const FuzzBufferType& buff, const int max_subs = MAX_SUBS,
-                       const size_t max_nested_subs = MAX_NESTED_SUBS);
+bool HasTooManySubFrag(std::span<const uint8_t> buff, int max_subs = MAX_SUBS,
+                       size_t max_nested_subs = MAX_NESTED_SUBS);
 
 //! Default maximum number of wrappers per fragment.
 constexpr int MAX_WRAPPERS{100};
@@ -74,6 +74,14 @@ constexpr int MAX_WRAPPERS{100};
  * Whether the buffer, if it represents a valid descriptor, contains a fragment with more
  * wrappers than the given maximum.
  */
-bool HasTooManyWrappers(const FuzzBufferType& buff, const int max_wrappers = MAX_WRAPPERS);
+bool HasTooManyWrappers(std::span<const uint8_t> buff, int max_wrappers = MAX_WRAPPERS);
+
+/// Default maximum leaf size. This should be large enough to cover an extended
+/// key, including paths "/", inside and outside of "[]".
+constexpr uint32_t MAX_LEAF_SIZE{200};
+
+/// Whether the expanded buffer (after calling GetDescriptor() in
+/// MockedDescriptorConverter) has a leaf size too large.
+bool HasTooLargeLeafSize(std::span<const uint8_t> buff, uint32_t max_leaf_size = MAX_LEAF_SIZE);
 
 #endif // BITCOIN_TEST_FUZZ_UTIL_DESCRIPTOR_H

src/wallet/test/fuzz/scriptpubkeyman.cpp

@@ -51,22 +51,22 @@ void initialize_spkm()
 }
 
 /**
- * Key derivation is expensive. Deriving deep derivation paths take a lot of compute and we'd rather spend time
- * elsewhere in this target, like on actually fuzzing the DescriptorScriptPubKeyMan. So rule out strings which could
- * correspond to a descriptor containing a too large derivation path.
+ * Deriving "expensive" descriptors will consume useful fuzz compute. The
+ * compute is better spent on a smaller subset of descriptors, which still
+ * covers all real end-user settings.
  */
-static bool TooDeepDerivPath(std::string_view desc)
+static bool IsTooExpensive(std::span<const uint8_t> desc)
 {
-    const FuzzBufferType desc_buf{reinterpret_cast<const unsigned char *>(desc.data()), desc.size()};
-    return HasDeepDerivPath(desc_buf);
+    return HasDeepDerivPath(desc) || HasTooManySubFrag(desc) || HasTooManyWrappers(desc);
 }
 
 static std::optional<std::pair<WalletDescriptor, FlatSigningProvider>> CreateWalletDescriptor(FuzzedDataProvider& fuzzed_data_provider)
 {
     const std::string mocked_descriptor{fuzzed_data_provider.ConsumeRandomLengthString()};
-    if (TooDeepDerivPath(mocked_descriptor)) return {};
+    if (IsTooExpensive(MakeUCharSpan(mocked_descriptor))) return {};
     const auto desc_str{MOCKED_DESC_CONVERTER.GetDescriptor(mocked_descriptor)};
     if (!desc_str.has_value()) return std::nullopt;
+    if (HasTooLargeLeafSize(MakeUCharSpan(*desc_str))) return {};
 
     FlatSigningProvider keys;
     std::string error;