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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
