[PR] PROTON-2957: fuzz-message-decode: exercise the codec on decoded content [qpid-proton]

["jiridanek (via GitHub)" <gi...@apache.org>]

jiridanek opened a new pull request, #450:
URL: https://github.com/apache/qpid-proton/pull/450

   https://issues.apache.org/jira/browse/PROTON-2957
   
   `pn_message_decode()` only scans the wire-level section framing (header, properties, annotations, application-properties, body) and stores each section's raw, undecoded bytes on the `pn_message_t`. It never invokes the generic AMQP codec (`codec.c`, `decoder.c`) on any of that content -- that only happens lazily, the first time an application calls one of the `pn_message_instructions()`, `pn_message_annotations()`, `pn_message_properties()`, or `pn_message_body()` accessors.
   
   The `fuzz-message-decode` harness only ever called `pn_message_decode()` and discarded the result, so none of those accessors were ever invoked and the fuzzer's input never actually reached the codec.
   
   This calls the four accessors and forces a full read-side traversal of each resulting `pn_data_t` via `pn_data_format()`, then round-trips the message back to bytes via `pn_message_encode2()` to also exercise the encoder on the same content. This implements the harness's own long-standing `// FUTURE: do something like encode msg and compare again with Data` comment.
   
   ## Test plan
   - [x] Verified the existing regression corpus still passes unmodified (`ctest -R fuzz-message-decode`)
   - [x] Confirmed the harness builds and runs cleanly with `-DENABLE_FUZZ_TESTING=ON`


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

Re: [PR] PROTON-2957: fuzz-message-decode: exercise the codec on decoded content [qpid-proton]

["ide-developer (via GitHub)" <gi...@apache.org>]

ide-developer commented on code in PR #450:
URL: https://github.com/apache/qpid-proton/pull/450#discussion_r3804913655


##########
c/tests/fuzz/fuzz-message-decode.c:
##########
@@ -20,11 +20,46 @@
  */
 
 #include <stdint.h>
+#include <stdlib.h>
 
 #include "proton/message.h"
 
 #include "libFuzzingEngine.h"
 
+/*
+ * pn_message_decode() (c/src/core/message.c) only scans the wire-level
+ * section framing (header / properties / delivery-annotations /
+ * message-annotations / application-properties / body) and stashes each
+ * section's *raw*, undecoded bytes on the pn_message_t. It never calls into
+ * the generic AMQP codec (c/src/core/codec.c, decoder.c) on any of those
+ * byte ranges. That only happens lazily -- the first time something calls
+ * one of the pn_message_{instructions,annotations,properties,body}()
+ * accessors, which route through pni_switch_to_data() (c/src/core/util.h)
+ * -> pn_data_decode() -> the real recursive decoder in decoder.c.
+ *
+ * Previously this harness only ever called pn_message_decode() and threw
+ * the result away, so none of those accessors were ever invoked and the
+ * fuzzer's input bytes never actually reached codec.c/decoder.c/encoder.c.
+ *
+ * Force that decode here so the fuzzer's own input bytes actually drive the
+ * codec, then force a full read-side traversal of each resulting pn_data_t
+ * via pn_data_format() -- which recursively walks the decoded tree with
+ * pn_data_next()/pn_data_enter()/pn_data_exit() and the type-specific
+ * pn_data_get_*() accessors, so nested lists/maps/arrays/described values
+ * are actually visited and not just the outermost node. Finally re-encode
+ * the message (pn_message_encode2()) to drive the corresponding encoder.c
+ * paths on the same decoded content -- this is the harness's own
+ * long-standing "FUTURE" comment, now implemented.
+ */
+static void pni_force_data_traversal(pn_data_t *data) {
+  if (!data) return;
+  pn_data_rewind(data);
+  char buf[4096];
+  size_t size = sizeof(buf);
+  pn_data_format(data, buf, &size);
+  pn_data_rewind(data);
+}

Review Comment:
   Both `pn_data_rewind(data)` calls here look redundant given the call sites: the data producer (`pn_message_instructions/annotations/properties/body()`, via `pni_switch_to_data()`) and the eventual raw-bytes consumer (`pni_switch_to_raw_bytes()`) already rewind at the points where it matters, and `pn_data_format()` itself doesn't require the caller to rewind first.
   
   Not a functional problem today — both calls are harmless no-ops here — but the extra defensive rewinds make it harder for a future reader to tell whether the pre/post position is actually load-bearing. If this helper is ever reused in a context or call order where the rewind does matter, the redundant pair could mask that. Worth dropping unless there's a reason to keep them explicit.



##########
c/tests/fuzz/fuzz-message-decode.c:
##########
@@ -20,11 +20,46 @@
  */
 
 #include <stdint.h>
+#include <stdlib.h>
 
 #include "proton/message.h"
 
 #include "libFuzzingEngine.h"
 
+/*
+ * pn_message_decode() (c/src/core/message.c) only scans the wire-level
+ * section framing (header / properties / delivery-annotations /
+ * message-annotations / application-properties / body) and stashes each
+ * section's *raw*, undecoded bytes on the pn_message_t. It never calls into
+ * the generic AMQP codec (c/src/core/codec.c, decoder.c) on any of those
+ * byte ranges. That only happens lazily -- the first time something calls
+ * one of the pn_message_{instructions,annotations,properties,body}()
+ * accessors, which route through pni_switch_to_data() (c/src/core/util.h)
+ * -> pn_data_decode() -> the real recursive decoder in decoder.c.
+ *
+ * Previously this harness only ever called pn_message_decode() and threw
+ * the result away, so none of those accessors were ever invoked and the
+ * fuzzer's input bytes never actually reached codec.c/decoder.c/encoder.c.
+ *
+ * Force that decode here so the fuzzer's own input bytes actually drive the
+ * codec, then force a full read-side traversal of each resulting pn_data_t
+ * via pn_data_format() -- which recursively walks the decoded tree with
+ * pn_data_next()/pn_data_enter()/pn_data_exit() and the type-specific
+ * pn_data_get_*() accessors, so nested lists/maps/arrays/described values
+ * are actually visited and not just the outermost node. Finally re-encode
+ * the message (pn_message_encode2()) to drive the corresponding encoder.c
+ * paths on the same decoded content -- this is the harness's own
+ * long-standing "FUTURE" comment, now implemented.
+ */
+static void pni_force_data_traversal(pn_data_t *data) {

Review Comment:
   Nit: `pni_` is used throughout `c/src/core` as the naming convention for that library's own private/internal symbols (e.g. `pni_switch_to_data`, `pni_switch_to_raw_bytes`, both referenced in the comment above). Reusing it for a `static` helper local to this fuzz harness is a little confusing when grepping for internal core symbols. A different prefix (or none, since it's already `static`) would avoid the naming collision in spirit.



##########
c/tests/fuzz/fuzz-message-decode.c:
##########
@@ -33,7 +68,17 @@ int LLVMFuzzerTestOneInput(const uint8_t *Data, size_t Size) {
   pn_message_t *msg = pn_message();
   int ret = pn_message_decode(msg, (const char *)Data, Size);
   if (ret == 0) {
-    // FUTURE: do something like encode msg and compare again with Data
+    // Force real decode + traversal of each lazily-decoded section.
+    pni_force_data_traversal(pn_message_instructions(msg));
+    pni_force_data_traversal(pn_message_annotations(msg));
+    pni_force_data_traversal(pn_message_properties(msg));
+    pni_force_data_traversal(pn_message_body(msg));
+
+    // Round-trip the decoded message back to bytes: exercises encoder.c on
+    // the same fuzzer-controlled content.
+    pn_rwbytes_t buf = {0, NULL};
+    pn_message_encode2(msg, &buf);

Review Comment:
   The return value of `pn_message_encode2()` isn't checked here, unlike `pn_message_decode()`'s `ret` above. For a fuzz harness that's often fine since we want to keep exploring even on expected failures, but it might be worth at least asserting it's one of the expected error codes, so a genuinely unexpected failure mode during fuzzing doesn't go silently unnoticed.



##########
c/tests/fuzz/fuzz-message-decode.c:
##########
@@ -33,7 +68,17 @@ int LLVMFuzzerTestOneInput(const uint8_t *Data, size_t Size) {
   pn_message_t *msg = pn_message();
   int ret = pn_message_decode(msg, (const char *)Data, Size);
   if (ret == 0) {
-    // FUTURE: do something like encode msg and compare again with Data
+    // Force real decode + traversal of each lazily-decoded section.
+    pni_force_data_traversal(pn_message_instructions(msg));
+    pni_force_data_traversal(pn_message_annotations(msg));
+    pni_force_data_traversal(pn_message_properties(msg));
+    pni_force_data_traversal(pn_message_body(msg));
+
+    // Round-trip the decoded message back to bytes: exercises encoder.c on
+    // the same fuzzer-controlled content.
+    pn_rwbytes_t buf = {0, NULL};
+    pn_message_encode2(msg, &buf);
+    free(buf.start);

Review Comment:
   This encodes the decoded message but discards the result (`free(buf.start)`) without decoding it again and diffing against the original. The comment this replaces — `// FUTURE: do something like encode msg and compare again with Data` — reads like the round-trip comparison itself was the intended end state; what's implemented here exercises the encoder paths (good, that's real new coverage) but doesn't yet catch encode-side correctness bugs, e.g. the encoder picking the wrong body-section descriptor or dropping/duplicating a header field on re-encode, since nothing inspects `buf` before it's freed.
   
   Might be worth a follow-up that decodes `buf` again into a second `pn_message_t` and compares the relevant fields against the original `msg`, so silent encode-content bugs actually produce a signal.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org