fix(communication): tighten ProtobufMessageParser gap-gate + wire-type validation (closes #189)#197
Conversation
…e validation (closes #189) Two bugs surfaced by Qodo /agentic_review on the daqifi-python-core port (PR #100); both present in the C# implementation. Bug 1 — gap gate corrupted multi-chunk frames. The partial-frame wait path advanced currentIndex whenever missingPayload exceeded MaxPartialFrameGapBytes, regardless of how many body bytes had arrived. A legitimate multi-KB frame split across reads (e.g. an initial-status frame on a transport returning smaller chunks) was misclassified as garbage mid-frame, causing the parser to skip into the real body and corrupt it. Fixed: gate the gap heuristic on availableBodyBytes <= 1. Once 2+ body bytes are buffered, the field-tag gate above has already structurally validated the frame start and a large declared length is just a multi-chunk read. Bug 2 — IsPlausibleFieldTagByte skipped wire-type validation on multi-byte tags. The continuation-bit early return bypassed the wire-type check, but wire type lives in the low 3 bits of the first byte regardless of multi-byte field numbers. Impossible wire types (3,4,6,7) MUST be rejected even on continuation bytes. Fixed: check wire type before the continuation-bit return. Added two regression tests: one asserting a real >1KB frame fed in prefix+partial-body form is preserved (gap gate doesn't fire), and a parametrized theory covering all four impossible wire types with the continuation bit set (field-tag gate fires). 15/15 parser tests pass; 895/897 in the full suite (2 skipped require live hardware).
Fix 1 stale comment: test fixture references DeviceFwRev (actual field) instead of HostFirmwareRev (issue text used pre-rename name).
|
/improve |
|
/agentic_review |
Code Review by Qodo
1.
|
Review Summary by QodoTighten ProtobufMessageParser gap-gate and wire-type validation
WalkthroughsDescription• Fix gap-gate corruption of legitimate multi-KB frames split across reads - Gate suspicion check on availableBodyBytes <= 1 to prevent false positives • Fix wire-type validation bypass on multi-byte field tags with continuation bit - Check wire type before early-return for continuation bytes • Add two regression tests covering both bugs - Multi-KB frame preservation test and parametrized impossible wire-type tests Diagramflowchart LR
A["Parser receives<br/>partial frame"] --> B{"availableBodyBytes<br/><= 1?"}
B -->|Yes| C{"missingPayload ><br/>MaxPartialFrameGapBytes?"}
B -->|No| D["Wait for more<br/>body bytes"]
C -->|Yes| E["Advance past<br/>garbage"]
C -->|No| D
F["Check wire type<br/>in low 3 bits"] --> G{"Wire type<br/>3,4,6,7?"}
G -->|Yes| E
G -->|No| H{"Continuation<br/>bit set?"}
H -->|Yes| D
H -->|No| I{"Field number<br/>!= 0?"}
I -->|Yes| D
I -->|No| E
File Changes1. src/Daqifi.Core/Communication/Consumers/ProtobufMessageParser.cs
|
Code Review by Qodo
1.
|
PR Code Suggestions ✨Warning
|
|||||||||
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…o availableBodyBytes == 0 Qodo flagged that the previous fix's `availableBodyBytes <= 1` still corrupts real frames whose first body byte arrives alone (a common chunked-read scenario). With 1 body byte buffered, the gap gate would fire even though the field-tag check on that single byte already provides positive evidence the prefix is a real frame start; advancing 1 byte then unrecoverably corrupts the frame because StreamMessageConsumer trims consumedBytes from its buffer. Tightened the gap gate to availableBodyBytes == 0 — once any body byte is observable, the field-tag gate handles validation. Added a regression test feeding prefix + exactly 1 body byte of a real multi-KB frame; parser must wait, not advance.
|
/improve |
|
/agentic_review |
|
Persistent review updated to latest commit 32cfe23 |
PR Code Suggestions ✨Warning
|
|||||||||
Add explicit fullFrame.Length > 1100 assertion in ProtobufMessageParser_ParseMessages_OneBodyByteOfMultiKbFrame_NotCorrupted to mirror the sibling LegitimateMultiKbFrame test. Prevents silent regression de-scoping if the fixture message ever shrinks below the gap-gate threshold.
|
/improve |
|
/agentic_review |
|
Persistent review updated to latest commit 69e9a79 |
PR Code Suggestions ✨Warning
|
|||||||||
Hardware-in-the-loop test (DAQiFi device on
|
| Test | This PR | main |
|---|---|---|
| 1ch @ 100 Hz, 3 s | 299 samples | 299 samples |
| 16ch @ 200 Hz, 5 s | 998 samples | 998 samples |
| Median Δt (heavy) | 250112 | 250112 |
| Max Δt (heavy) | 378111 | 479093 |
Both branches connect, complete the initial-status handshake (streaming setup wouldn't finish otherwise), and deliver matching sample counts. Timestamp deltas are clean — no gaps that would suggest a swallowed frame.
What this proves / doesn't
- ✅ Non-regressive on real hardware — identical sample counts and median Δt vs
main, all 16 analog channels deserialize correctly across 998 frames. ⚠️ Bug 1 didn't fire organically on either branch. This device's initial-status frame fits under the 1 KB gap threshold, so the buggy gate onmainnever tripped during the runs I did. The unit test in this PR (LegitimateMultiKbFrame_NotCorruptedByGapGate) constructs a synthetic 2 KB frame to force it — that's where the regression coverage lives.⚠️ Bug 2 didn't fire either (non-deterministic — needs boot-time garbage to happen to have the continuation bit + a deprecated wire type).
daqifi-desktop compatibility
Spot-checked the desktop repo: it pins Daqifi.Core 0.21.0 and has zero direct references to ProtobufMessageParser, IMessageParser, StreamMessageConsumer, ParseMessages, or IsPlausibleFieldTagByte — it consumes the parser only transitively through DaqifiStreamingDevice. No method signatures, types, or public constants changed in this PR, so desktop picks up both fixes automatically on its next Daqifi.Core bump with no code changes.
Minor note on the <= 1 vs == 0 choice
Issue #189 suggested availableBodyBytes <= 1 for the gap gate; this PR uses == 0 (stricter) with a well-justified comment and a dedicated OneBodyByteOfMultiKbFrame_NotCorrupted test. If the Python port settled on <= 1, worth a quick note tracking the intentional divergence — otherwise no concern.
Smoke test says: ship it.
2 participants
Summary
Two parser-hardening bugs surfaced by Qodo on the daqifi-python-core port (PR #100); both present in the C# implementation.
availableBodyBytes <= 1so the field-tag gate handles all other cases.IsPlausibleFieldTagByteskipped wire-type validation on continuation bytes. Wire type lives in the low 3 bits of the first byte regardless of multi-byte tags, so impossible wire types (3, 4, 6, 7) are now rejected even on continuation.Both fixes match the resolution shipped on the Python port and validated by Qodo there.
Test plan
LegitimateMultiKbFrame_NotCorruptedByGapGateFact +GarbageWithContinuationBit_StillRejectedTheory × 4 wire types)Closes #189