fuzz: miniscript: test the node is satisfiable before dereferencing `GetOps` #28114

After #27997. Mostly a self-reminder that the fuzz target could crash at any moment on master on OSS fuzz. I'll do it tomorrow, hopefully Marco won't come up with an OSS fuzz bug report before then.

Heh, if there's a known bug that should be found by fuzzers, but isn't, then the tests should be fixed to find it :)

Yeah i need to investigate why it didn't trigger in the PR. Maybe i'm just wrong, i wrote this on a rush to not forget about it.

-------- Original Message -------- On Jul 20, 2023, 5:36 PM, MacrabFalke wrote:

Heh, if there's a known bug that should be found by fuzzers, but isn't, then the tests should be fixed to find it :)

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

Okay, TIL. Or rather today i read the doc. Dereferencing an optional is undefined behaviour (won't necessarily crash) whereas calling value() would throw std::bad_optional_access.

Simply changing one for the other will trigger the expected crash when running over the qa-assets corpus:

diff --git a/src/test/fuzz/miniscript.cpp b/src/test/fuzz/miniscript.cpp
index 56327b9665..c3955cad7e 100644
--- a/src/test/fuzz/miniscript.cpp
+++ b/src/test/fuzz/miniscript.cpp
@@ -943,8 +943,8 @@ void TestNode(const NodeRef& node, FuzzedDataProvider& provider)
     assert(decoded->ToScript(PARSER_CTX) == script);
     assert(decoded->GetType() == node->GetType());
 
-    const auto node_ops{node->GetOps()};
-    if (provider.ConsumeBool() && node_ops && *node_ops < MAX_OPS_PER_SCRIPT && node->ScriptSize() < MAX_STANDARD_P2WSH_SCRIPT_SIZE) {
+    const auto node_ops{node->GetOps().value()};
+    if (provider.ConsumeBool() && node_ops && node_ops < MAX_OPS_PER_SCRIPT && node->ScriptSize() < MAX_STANDARD_P2WSH_SCRIPT_SIZE) {
         // Optionally pad the script with OP_NOPs to max op the ops limit of the constructed script.
         // This makes the script obviously not actually miniscript-compatible anymore, but the
         // signatures constructed in this test don't commit to the script anyway, so the same
@@ -955,7 +955,7 @@ void TestNode(const NodeRef& node, FuzzedDataProvider& provider)
         // Do not pad more than what would cause MAX_STANDARD_P2WSH_SCRIPT_SIZE to be reached, however,
         // as that also invalidates scripts.
         int add = std::min<int>(
-            MAX_OPS_PER_SCRIPT - *node_ops,
+            MAX_OPS_PER_SCRIPT - node_ops,
             MAX_STANDARD_P2WSH_SCRIPT_SIZE - node->ScriptSize());
         for (int i = 0; i < add; ++i) script.push_back(OP_NOP);
     }

However @MarcoFalke shouldn't this UB be caught by the sanitizer?

I think there is a check for both dereferences of the optional?

if (provider.ConsumeBool() && node_ops && *node_ops < MAX...
                              ^^check     ^^deref

:facepalm:

Next time i'll make sure to have coffee before commenting on anything. Sorry for the noise (again).