Skip to content

Check the size of PROTMESSID_ACKN messages - #302

Merged
corrados merged 1 commit into
jamulussoftware:masterfrom
atsampson:acknsize
May 28, 2020
Merged

Check the size of PROTMESSID_ACKN messages#302
corrados merged 1 commit into
jamulussoftware:masterfrom
atsampson:acknsize

Conversation

@atsampson

Copy link
Copy Markdown
Contributor

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.

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.
@corrados

Copy link
Copy Markdown
Contributor

Very good that you found it. Thank you!

@corrados
corrados merged commit 64c4eb4 into jamulussoftware:master May 28, 2020
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
@dtinth dtinth mentioned this pull request Jul 23, 2026
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants