Check the size of PROTMESSID_ACKN messages - #302
Merged
Conversation
The Evaluate* functions that parse other types of messages all check the message size before starting to read data, but the special code for acknowledgements didn't do this, so an ACKN message on an existing connection that had a valid checksum but no data would result in an out-of-bounds read. Found by fuzzing with afl-fuzz.
Contributor
|
Very good that you found it. Thank you! |
dtinth-claw Bot
added a commit
to dtinth/jamulus
that referenced
this pull request
Jul 23, 2026
Introduces src/test/, a QtTest-based unit test target (jamulus-test)
covering CProtocol's on-wire framing contract:
- golden-frame tests pin the exact bytes production emits today for a
fixed-length and a variable-length message body, so any accidental
wire format change fails loudly instead of silently -- the expected
hex string is built field by field (TAG, message ID, sequence
counter, data length, data, CRC), each `+=` line commented with what
that field is, against a fresh CProtocolTester's SentFrames()
- a frame acceptance/rejection contract test trio (AcceptValidFrame,
RejectInvalidFrame with bad CRC/length/truncation/junk rows, and
IgnoreAcknWithEmptyBody -- the regression test for
jamulussoftware#302, fixed in
024ebb4: an ACKN message with a valid checksum but no data caused
an out-of-bounds read; the crafted frame is still well-formed so
it's accepted at the frame level, but must be silently dropped with
no signal fired, and the ASan/UBSan matrix job is what gives the
"no OOB" part of that its teeth) checks how many frames the
receiving side actually parsed and accepted, building each frame
imperatively from a real sent frame (LastSentFrame()) plus a small
set of named mutation helpers
- one round-trip test through a connected sender/receiver pair
(CProtocolTester) sends a message on one side and asserts, against
an event log of everything the other side received, that exactly
the expected signal fired with the expected arguments
CProtocolTester (src/test/protocoltester.h) is the one public type this
header exposes: a struct-like pair of CProtocol instances (Sender,
Receiver) wired together in both directions -- the same wiring CChannel
uses for two peers, including routing acknowledgements back so the
sender's queue advances -- plus:
- a sent frame log: every frame Sender hands to MessReadyForSending is
recorded as both a hex string (SentFrames(), for golden frame
comparisons) and raw bytes (LastSentFrame(), for tests that mutate a
real frame); a fresh instance's first send has sequence counter 0,
which is what makes golden frames byte-for-byte reproducible
- a receiver-side acceptance count: ReceivedAndAcceptedMessageCount()
counts frames that passed frame parsing on the receiver side and
were handed to ParseMessageBody(); a failed parse -- whether from a
real send or a malformed frame injected via SendRawBytes() -- is a
silently dropped, countable non-event rather than an assertion
failure, and is tracked per direction so an ACK flowing back to the
sender never affects the receiver's own count
- a received signal log: every non-CLM "receiving" signal CProtocol
emits from ParseMessageBody (protocol.h's
ChangeJittBufSize..RecorderStateReceived block, 21 signals) is wired
to append one formatted "SignalName(arg1, arg2)" line to
ReceivedLog(), so a test can QCOMPARE the whole log against what it
expects -- which also proves, for free, that no other wired signal
fired
- ToByteArray()/FromByteArray(), the frame mutators
TruncateBy()/CorruptCRC()/SetDeclaredLength(), and ReplaceIdAndBody()
(rebuilds a frame with a different ID/body, recomputing length and
CRC) for crafting ID/body combinations a real Create*Mes() call
can't produce
Tests hold their own frames/expectations and call these helpers
directly, with no builder chain, lambda-taking capture function, or
shared error-message plumbing in the public surface to look through.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HGdLtGxMrbExBFG3aReNAH
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Evaluate* functions that parse other types of messages all check the body size before starting to read data, but the special code for acknowledgements didn't do this, so an ACKN message on an existing connection that had a valid checksum but no body would result in an out-of-bounds read.
Found by fuzzing the protocol parser with afl-fuzz, using messages from CTestbench as an initial corpus and disabling the CRC check. This was the only problem it found in a 24h fuzzing run.