Validate malformed input packets before decoding#2024
Open
cmuellner wants to merge 6 commits into
Open
Conversation
Origin: reported in PR mcallegari#1955. The regression test creates a fixture on universe 1, then calls `setfixture` with a runtime universe list that only contains universe 0. Before this fix, `handleSetFixture()` indexed `universes[1]` and could read past the list. Reject the command before requesting a fader when the fixture universe is not present in the runtime universe list. This keeps malformed or inconsistent projects from crashing script playback. Added a regression test in `script_test` for a fixture whose universe is missing from the runtime universe list. Signed-off-by: Christoph Müllner <christophm30@gmail.com>
Origin: reported in PR mcallegari#1955. The regression test covers protocol inputs rather than GUI steps: a 186-byte ArtPollReply, a short ArtDMX packet, an ArtDMX packet whose length field exceeds the datagram, and a TOD packet with `uidCount` 1 but no UID bytes. These inputs previously reached fixed-offset reads or accepted an incomplete UID list. Validate the minimum ArtPollReply size, the ArtDMX fixed header and advertised payload length, and the TOD UID list length before decoding. Extended `artnet_test` to cover short ArtPollReply, short ArtDMX, truncated ArtDMX payload, and truncated TOD UID list packets. Signed-off-by: Christoph Müllner <christophm30@gmail.com>
Origin: reported in PR mcallegari#1955. The regression test covers protocol inputs rather than GUI steps: a 125-byte packet missing the DMX start code, a zero property value count, and a packet whose count exceeds the remaining datagram. Before this fix, `checkPacket()` accepted the missing-start-code packet and `fillDMXdata()` could read byte 125 or underflow `length - 1`. Require the start-code byte before accepting an E1.31 DMX packet, then reject zero, oversized, or truncated property value counts before copying DMX slots. Added `e131packetizer_test` to cover a missing start code, zero property value count, truncated payload, and a valid packet. Signed-off-by: Christoph Müllner <christophm30@gmail.com>
Origin: reported in PR mcallegari#1955. The regression test covers protocol inputs rather than GUI steps: an OSC message whose type-tag list has no NUL terminator, a message whose string argument is missing its NUL terminator, and a bundle with a valid first message followed by a trailing 3-byte message-size field. Before this fix, parseMessage() could scan past the message buffer or accept a truncated argument, and the bundle parser could read bufPos + 3 past the packet. Stop type-tag and string-argument scanning at the buffer end. Reject fixed-size arguments whose bytes are not fully present, and require four bytes before reading each bundled message size. Added oscpacketizer_test to cover unterminated type tags, truncated string arguments, short bundle size fields, and a valid bundled message. Signed-off-by: Christoph Müllner <christophm30@gmail.com>
Origin: derived while validating PR mcallegari#1955. The regression test uses protocol inputs in artnet_test: a one-byte RDM start-code packet, a 22-byte fixed header, a packet whose PDL claims 21 bytes without carrying that payload, and a DEVICE_INFO response whose valid checksum covers a too-short PID payload. Before this fix, parsePacket() could read the sub-start code, fixed response fields, or PID-specific payload beyond the reply buffer or beyond the declared PDL. Validate the fixed response header before decoding fields, then require PDL bytes plus the checksum before entering PID-specific parsing. Reject PID payloads that are too short for fixed-field decoders, and decode the 19-byte DEVICE_INFO sensor-count field at its protocol offset. Extended artnet_test to cover short RDM headers, truncated PDL payloads, too-short PID payloads, and valid RDM packets. Signed-off-by: Christoph Müllner <christophm30@gmail.com>
Origin: derived while validating PR mcallegari#1955. The payload-length guard was found while reviewing the same Enttec input parsing path. Feed the Enttec DMX USB Pro input path a receive-DMX packet with a payload length of 0 or 1, or a MIDI input packet ending with an incomplete command. The DMX branch read the status and start-code bytes without proving they existed, and the MIDI branch read data bytes at i + 1 and i + 2 for a truncated final command. The DMX branch also derived its slot-data slice from packet.size(). That is equivalent for well-formed packets, but using the validated payload offset makes the relationship to the decoded header explicit and keeps the malformed-payload checks localized. Reject malformed DMX payloads before decoding fixed fields, copy DMX data from the validated payload offset, and ignore a trailing incomplete MIDI command. No automated regression test exists for this device input path. Signed-off-by: Christoph Müllner <christophm30@gmail.com>
cmuellner
force-pushed
the
pr-1955-fixes-p1
branch
from
71b6634 to
11dd00f
Compare
|
coverage: 35.646% (+0.6%) from 35.037% — cmuellner:pr-1955-fixes-p1 into mcallegari:master |
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
2 participants
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.
Description
Summary of Changes:
This PR fixes malformed-input reliability issues found while reviewing
stale PR #1955 and adjacent parser code.
The affected parsers now reject short or inconsistent input before
reading fixed fields or payload bytes. Well-formed input is handled as
before.
Commit Guide
script_testfor a fixture whose universe is absent from the runtime universe list.artnet_testwith short ArtPollReply, ArtDMX and ArtTOD packets.e131packetizer_testcoverage for missing start code, zero count, truncated payload and a valid packet.oscpacketizer_testcoverage for unterminated type tags, truncated string arguments, short bundle size fields and a valid bundle.artnet_testwith short RDM headers, oversized or truncated PDL values, too-short PID payloads, and valid ACK responses.Testing
Automated Coverage:
The parser paths with in-tree harnesses are covered by:
script_testartnet_teste131packetizer_testoscpacketizer_testResult:
The added and extended tests pass locally.
Manual Testing Gaps
The Enttec DMX USB Pro input-path change is not covered by an
automated regression test because the current tree does not provide a
device-level test harness for that plugin.
A reviewer with compatible hardware can help confirm there is no
behavioral regression by connecting the device, enabling the DMX USB
plugin, and verifying that normal DMX input and MIDI input still work.
Additional Notes
The fixes are split into one commit per issue so individual parser
changes remain easy to review and bisect.
Each commit body contains the full rationale and reproduction details.
Checklist
{on a new line for functions and class definitions.