From e2d5b4343bd0fb792dcc4bfc10c8c09f9462e6d3 Mon Sep 17 00:00:00 2001 From: Chris Lange Date: Mon, 11 May 2026 22:16:56 -0600 Subject: [PATCH 1/4] fix(communication): tighten ProtobufMessageParser gap-gate + wire-type validation (closes #189) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- .../Consumers/ProtobufMessageParserTests.cs | 96 +++++++++++++++++++ .../Consumers/ProtobufMessageParser.cs | 50 ++++++---- 2 files changed, 129 insertions(+), 17 deletions(-) diff --git a/src/Daqifi.Core.Tests/Communication/Consumers/ProtobufMessageParserTests.cs b/src/Daqifi.Core.Tests/Communication/Consumers/ProtobufMessageParserTests.cs index e262cea..9325fa4 100644 --- a/src/Daqifi.Core.Tests/Communication/Consumers/ProtobufMessageParserTests.cs +++ b/src/Daqifi.Core.Tests/Communication/Consumers/ProtobufMessageParserTests.cs @@ -230,6 +230,102 @@ public void ProtobufMessageParser_ParseMessages_PreservesPartialFrameAfterGarbag Assert.Equal(junk.Length, consumedBytes); } + [Fact] + public void ProtobufMessageParser_ParseMessages_LegitimateMultiKbFrame_NotCorruptedByGapGate() + { + // Closes #189 Bug 1. The gap gate previously fired whenever + // missingPayload > MaxPartialFrameGapBytes regardless of how many + // body bytes had already arrived. A legitimate multi-KB frame + // arriving across multiple reads (e.g. a fat initial-status frame + // on a transport that returns smaller chunks) would be misclassified + // as garbage mid-frame: the parser would advance into the real body + // and corrupt it. The fix gates the suspicion check on + // availableBodyBytes <= 1 — once 2+ body bytes are buffered, the + // field-tag check has already structurally validated the start. + // + // Construct a real frame whose declared length leaves a gap larger + // than MaxPartialFrameGapBytes (1024) and feed only the prefix + + // a couple of body bytes. The parser must NOT advance into the + // body — it must wait for the rest. + + // Arrange + var parser = new ProtobufMessageParser(); + + // Build a real frame ~2KB with HostFirmwareRev padded to push the + // body past the gap threshold. WriteDelimitedTo prepends the varint + // length prefix. + var msg = new DaqifiOutMessage + { + MsgTimeStamp = 99, + DeviceFwRev = new string('x', 2000), + }; + using var stream = new MemoryStream(); + msg.WriteDelimitedTo(stream); + var fullFrame = stream.ToArray(); + Assert.True(fullFrame.Length > 1100, + "Test precondition: frame must exceed MaxPartialFrameGapBytes (1024) so the gap gate would fire on a one-shot read."); + + // Feed only prefix + first 4 body bytes — body is partially present + // (more than 1 byte, so gap gate must NOT fire), but not complete. + var prefixPlusFour = fullFrame.Take(GetVarintLength(fullFrame) + 4).ToArray(); + + // Act + var messages = parser.ParseMessages(prefixPlusFour, out var consumedBytes).ToList(); + + // Assert: parser waits — no message parsed, no bytes advanced. + // Callers retain the buffer for the next read; corrupting it here + // (advancing into the body) would lose the frame forever. + Assert.Empty(messages); + Assert.Equal(0, consumedBytes); + } + + [Theory] + [InlineData((byte)0x83)] // continuation bit set, wire type 3 (deprecated SGROUP) + [InlineData((byte)0x84)] // continuation bit set, wire type 4 (deprecated EGROUP) + [InlineData((byte)0x86)] // continuation bit set, wire type 6 (undefined) + [InlineData((byte)0x87)] // continuation bit set, wire type 7 (undefined) + public void ProtobufMessageParser_ParseMessages_GarbageWithContinuationBit_StillRejected(byte garbageBodyByte) + { + // Closes #189 Bug 2. IsPlausibleFieldTagByte previously early- + // returned true for any byte with the continuation bit (0x80) set, + // bypassing wire-type validation. 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) MUST be rejected even on continuation bytes. + // + // Construct a frame with: a plausible-but-bogus length prefix + + // a body byte whose continuation bit is set AND whose low 3 bits + // form an impossible wire type. Without the fix, the parser + // accepts the body byte and stalls waiting for the bogus declared + // length to arrive. With the fix, the field-tag gate fires and + // the parser advances past the garbage. + + // Arrange + var parser = new ProtobufMessageParser(); + + // Length prefix: 0x10 = 16 bytes (well under MaxPartialFrameGapBytes + // so the gap gate doesn't fire on its own — we want to isolate the + // wire-type check). + var data = new byte[] { 0x10, garbageBodyByte }; + + // Act + var messages = parser.ParseMessages(data, out var consumedBytes).ToList(); + + // Assert: parser advances past the bogus prefix instead of waiting + // for 16 never-coming bytes. + Assert.Empty(messages); + Assert.True(consumedBytes >= 1, + $"Expected parser to advance past garbage prefix, got consumedBytes={consumedBytes}"); + } + + private static int GetVarintLength(byte[] buffer) + { + for (var i = 0; i < buffer.Length && i < 5; i++) + { + if ((buffer[i] & 0x80) == 0) return i + 1; + } + throw new InvalidOperationException("Malformed varint in test fixture"); + } + [Fact] public void ProtobufMessageParser_ParseMessages_ReturnsCorrectMessageType() { diff --git a/src/Daqifi.Core/Communication/Consumers/ProtobufMessageParser.cs b/src/Daqifi.Core/Communication/Consumers/ProtobufMessageParser.cs index 1d6e4ce..7ea1496 100644 --- a/src/Daqifi.Core/Communication/Consumers/ProtobufMessageParser.cs +++ b/src/Daqifi.Core/Communication/Consumers/ProtobufMessageParser.cs @@ -108,11 +108,24 @@ public IEnumerable> ParseMessages(byte[] data, // Neither gate advances into a real frame's prefix or payload: only // into bytes already identified as implausible. Callers that trim // consumedBytes from their buffer never lose recoverable data. - var missingPayload = messageLength - (remainingData.Length - prefixBytes); - var firstBodyByteIsGarbage = remainingData.Length > prefixBytes + var availableBodyBytes = remainingData.Length - prefixBytes; + var missingPayload = messageLength - availableBodyBytes; + var firstBodyByteIsGarbage = availableBodyBytes > 0 && !IsPlausibleFieldTagByte(remainingData[prefixBytes]); - if (missingPayload > MaxPartialFrameGapBytes || firstBodyByteIsGarbage) + // Gap gate ONLY applies in the pure-prefix-no-body case + // (≤1 body byte arrived). Once 2+ body bytes are buffered, + // the field-tag gate above has already structurally validated + // the start of the frame; a large declared length at that + // point is just a multi-chunk read of a legitimate frame + // (e.g. a multi-KB initial-status frame on a transport that + // returns smaller reads). Without this guard, the parser + // would treat the partial-but-real frame as garbage and + // skip into its body, corrupting it. + var gapIsSuspicious = availableBodyBytes <= 1 + && missingPayload > MaxPartialFrameGapBytes; + + if (gapIsSuspicious || firstBodyByteIsGarbage) { currentIndex++; consumedBytes = currentIndex; @@ -146,29 +159,32 @@ public IEnumerable> ParseMessages(byte[] data, /// Returns true if could plausibly be the first byte of a /// real DaqifiOutMessage body (a protobuf field tag). Used by the partial-frame /// wait path to reject garbage whose varint prefix happens to decode to a - /// plausible-looking length. Multi-byte tags (continuation bit set) can't be - /// fully validated from a single byte, so they pass this check. + /// plausible-looking length. Multi-byte tags (continuation bit set) can't have + /// their full field number validated from a single byte, but the wire type + /// always lives in the low 3 bits of the first byte regardless — so impossible + /// wire types are rejected even on continuation bytes. /// private static bool IsPlausibleFieldTagByte(byte b) { - // Continuation bit set → multi-byte tag. We can't fully validate without - // more bytes (which we may not have yet); don't reject on that alone. - if ((b & 0x80) != 0) - { - return true; - } - - // Single-byte tag: low 3 bits are wire type, next 4 bits (within the low 7) - // are the field number. Valid wire types are VARINT(0), I64(1), LEN(2), - // I32(5). Wire types 3 (SGROUP) and 4 (EGROUP) are deprecated group types - // that DaqifiOutMessage does not use; 6/7 are undefined. Field number 0 - // is reserved and never appears in a real message. + // Wire type lives in the low 3 bits regardless of whether the field + // number spans multiple bytes. Valid wire types are VARINT(0), I64(1), + // LEN(2), I32(5). Wire types 3 (SGROUP) and 4 (EGROUP) are deprecated + // group types that DaqifiOutMessage does not use; 6/7 are undefined. var wireType = b & 0x07; if (wireType is 3 or 4 or 6 or 7) { return false; } + // Continuation bit set → multi-byte tag. The field number spans more + // bytes which we may not have yet; accept once wire type passed. + if ((b & 0x80) != 0) + { + return true; + } + + // Single-byte tag: bits 6..3 are the field number. Field number 0 is + // reserved and never appears in a real message. var fieldNumber = (b >> 3) & 0x0F; return fieldNumber != 0; } From 75bea5f5f16c3fc49c739e74b3f49c31e7b47f82 Mon Sep 17 00:00:00 2001 From: Chris Lange Date: Mon, 11 May 2026 22:18:00 -0600 Subject: [PATCH 2/4] Code review: clean up before PR Fix 1 stale comment: test fixture references DeviceFwRev (actual field) instead of HostFirmwareRev (issue text used pre-rename name). --- .../Communication/Consumers/ProtobufMessageParserTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Daqifi.Core.Tests/Communication/Consumers/ProtobufMessageParserTests.cs b/src/Daqifi.Core.Tests/Communication/Consumers/ProtobufMessageParserTests.cs index 9325fa4..86d60e5 100644 --- a/src/Daqifi.Core.Tests/Communication/Consumers/ProtobufMessageParserTests.cs +++ b/src/Daqifi.Core.Tests/Communication/Consumers/ProtobufMessageParserTests.cs @@ -251,8 +251,8 @@ public void ProtobufMessageParser_ParseMessages_LegitimateMultiKbFrame_NotCorrup // Arrange var parser = new ProtobufMessageParser(); - // Build a real frame ~2KB with HostFirmwareRev padded to push the - // body past the gap threshold. WriteDelimitedTo prepends the varint + // Build a real frame ~2KB with DeviceFwRev padded to push the body + // past the gap threshold. WriteDelimitedTo prepends the varint // length prefix. var msg = new DaqifiOutMessage { From 32cfe23e3c708a0d07f3513c1d8b3b5b6ce2351c Mon Sep 17 00:00:00 2001 From: Chris Lange Date: Mon, 11 May 2026 22:23:08 -0600 Subject: [PATCH 3/4] fix: Apply Qodo /agentic_review pass 1 on PR #197: tighten gap gate to availableBodyBytes == 0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../Consumers/ProtobufMessageParserTests.cs | 37 ++++++++++++++++++- .../Consumers/ProtobufMessageParser.cs | 20 +++++----- 2 files changed, 47 insertions(+), 10 deletions(-) diff --git a/src/Daqifi.Core.Tests/Communication/Consumers/ProtobufMessageParserTests.cs b/src/Daqifi.Core.Tests/Communication/Consumers/ProtobufMessageParserTests.cs index 86d60e5..e8b21ea 100644 --- a/src/Daqifi.Core.Tests/Communication/Consumers/ProtobufMessageParserTests.cs +++ b/src/Daqifi.Core.Tests/Communication/Consumers/ProtobufMessageParserTests.cs @@ -266,7 +266,7 @@ public void ProtobufMessageParser_ParseMessages_LegitimateMultiKbFrame_NotCorrup "Test precondition: frame must exceed MaxPartialFrameGapBytes (1024) so the gap gate would fire on a one-shot read."); // Feed only prefix + first 4 body bytes — body is partially present - // (more than 1 byte, so gap gate must NOT fire), but not complete. + // but not complete. Parser must wait, not advance into the real body. var prefixPlusFour = fullFrame.Take(GetVarintLength(fullFrame) + 4).ToArray(); // Act @@ -279,6 +279,41 @@ public void ProtobufMessageParser_ParseMessages_LegitimateMultiKbFrame_NotCorrup Assert.Equal(0, consumedBytes); } + [Fact] + public void ProtobufMessageParser_ParseMessages_OneBodyByteOfMultiKbFrame_NotCorrupted() + { + // Edge case of #189 Bug 1: when EXACTLY one body byte of a real + // multi-KB frame has arrived (common with small chunked reads), + // the gap gate must not fire. The single body byte's field-tag + // check provides positive evidence the prefix is real; advancing + // even one byte here would unrecoverably corrupt the frame + // because StreamMessageConsumer trims consumedBytes from its + // buffer between reads. + + // Arrange + var parser = new ProtobufMessageParser(); + + var msg = new DaqifiOutMessage + { + MsgTimeStamp = 99, + DeviceFwRev = new string('x', 2000), + }; + using var stream = new MemoryStream(); + msg.WriteDelimitedTo(stream); + var fullFrame = stream.ToArray(); + + // Feed prefix + exactly 1 body byte. The first body byte of a real + // DaqifiOutMessage is a valid field tag (passes IsPlausibleFieldTagByte). + var prefixPlusOne = fullFrame.Take(GetVarintLength(fullFrame) + 1).ToArray(); + + // Act + var messages = parser.ParseMessages(prefixPlusOne, out var consumedBytes).ToList(); + + // Assert: parser waits, does not advance into the legitimate body. + Assert.Empty(messages); + Assert.Equal(0, consumedBytes); + } + [Theory] [InlineData((byte)0x83)] // continuation bit set, wire type 3 (deprecated SGROUP) [InlineData((byte)0x84)] // continuation bit set, wire type 4 (deprecated EGROUP) diff --git a/src/Daqifi.Core/Communication/Consumers/ProtobufMessageParser.cs b/src/Daqifi.Core/Communication/Consumers/ProtobufMessageParser.cs index 7ea1496..fa05913 100644 --- a/src/Daqifi.Core/Communication/Consumers/ProtobufMessageParser.cs +++ b/src/Daqifi.Core/Communication/Consumers/ProtobufMessageParser.cs @@ -114,15 +114,17 @@ public IEnumerable> ParseMessages(byte[] data, && !IsPlausibleFieldTagByte(remainingData[prefixBytes]); // Gap gate ONLY applies in the pure-prefix-no-body case - // (≤1 body byte arrived). Once 2+ body bytes are buffered, - // the field-tag gate above has already structurally validated - // the start of the frame; a large declared length at that - // point is just a multi-chunk read of a legitimate frame - // (e.g. a multi-KB initial-status frame on a transport that - // returns smaller reads). Without this guard, the parser - // would treat the partial-but-real frame as garbage and - // skip into its body, corrupting it. - var gapIsSuspicious = availableBodyBytes <= 1 + // (no body bytes arrived). Once any body byte is buffered, + // the field-tag gate above provides positive evidence that + // the prefix really is a frame start; a large declared + // length at that point is just a multi-chunk read of a + // legitimate frame (e.g. a multi-KB initial-status frame + // on a transport that returns smaller reads). Including + // availableBodyBytes == 1 here would corrupt real frames + // whose first body byte arrives alone — a single-byte + // mistaken advance is unrecoverable because callers trim + // consumedBytes from their buffer. + var gapIsSuspicious = availableBodyBytes == 0 && missingPayload > MaxPartialFrameGapBytes; if (gapIsSuspicious || firstBodyByteIsGarbage) From 69e9a792fc4ba8f10864442242ee01a08fce1c43 Mon Sep 17 00:00:00 2001 From: Chris Lange Date: Tue, 12 May 2026 12:40:43 -0600 Subject: [PATCH 4/4] Apply Qodo /agentic_review pass 2: assert frame size precondition 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. --- .../Communication/Consumers/ProtobufMessageParserTests.cs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/Daqifi.Core.Tests/Communication/Consumers/ProtobufMessageParserTests.cs b/src/Daqifi.Core.Tests/Communication/Consumers/ProtobufMessageParserTests.cs index e8b21ea..2b0f2d1 100644 --- a/src/Daqifi.Core.Tests/Communication/Consumers/ProtobufMessageParserTests.cs +++ b/src/Daqifi.Core.Tests/Communication/Consumers/ProtobufMessageParserTests.cs @@ -302,6 +302,14 @@ public void ProtobufMessageParser_ParseMessages_OneBodyByteOfMultiKbFrame_NotCor msg.WriteDelimitedTo(stream); var fullFrame = stream.ToArray(); + // Sanity-check the regression precondition: this test is meaningless + // unless the frame body would have triggered the old gap gate. The + // sibling LegitimateMultiKbFrame test uses the same threshold; if the + // fixture message ever shrinks below it, the regression silently + // de-scopes — fail loudly here instead. + Assert.True(fullFrame.Length > 1100, + $"Test fixture too small to exercise the gap gate (got {fullFrame.Length}, expected > 1100). Increase DeviceFwRev length."); + // Feed prefix + exactly 1 body byte. The first body byte of a real // DaqifiOutMessage is a valid field tag (passes IsPlausibleFieldTagByte). var prefixPlusOne = fullFrame.Take(GetVarintLength(fullFrame) + 1).ToArray();