Skip to content

PROTON-2957: fuzz-message-decode: exercise the codec on decoded content #450

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jiridanek wants to merge 1 commit into
base: main
Choose a base branch
from
+46 −1
Open

PROTON-2957: fuzz-message-decode: exercise the codec on decoded content #450

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 46 additions & 1 deletion c/tests/fuzz/fuzz-message-decode.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {

@ide-developer ide-developer Aug 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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);
}
Comment on lines +54 to +61

@ide-developer ide-developer Aug 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


int LLVMFuzzerTestOneInput(const uint8_t *Data, size_t Size) {
if (Size < 1) {
// pn_message_decode would die on assert
Expand All @@ -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);

@ide-developer ide-developer Aug 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

free(buf.start);
Comment on lines +77 to +81

@ide-developer ide-developer Aug 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
if (msg != NULL) {
pn_message_free(msg);
Expand Down
Loading