PROTON-2957: fuzz-message-decode: exercise the codec on decoded content - #450
PROTON-2957: fuzz-message-decode: exercise the codec on decoded content#450jiridanek wants to merge 1 commit into
Conversation
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,annotations,properties,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.
Call the four accessors and force a full read-side traversal of each
resulting pn_data_t via pn_data_format(), then round-trip 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.
ide-developer
left a comment
ide-developer
left a comment
There was a problem hiding this comment.
Reviewed the diff itself (not pre-existing codec/message.c code, which is out of scope here). The change does what it says: it forces the lazily-decoded sections through the codec and re-encodes, which is a genuinely useful improvement for this harness's coverage. A few observations on the new code specifically, inline below.
| 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); | ||
| } |
There was a problem hiding this 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.
| * 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) { |
There was a problem hiding this 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.
| // 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); |
There was a problem hiding this 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.
| // 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); |
There was a problem hiding this 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.
2 participants
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 thepn_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 thepn_message_instructions(),pn_message_annotations(),pn_message_properties(), orpn_message_body()accessors.The
fuzz-message-decodeharness only ever calledpn_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_tviapn_data_format(), then round-trips the message back to bytes viapn_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 Datacomment.Test plan
ctest -R fuzz-message-decode)-DENABLE_FUZZ_TESTING=ON