Skip to content

fix(communication): tighten ProtobufMessageParser gap-gate + wire-type validation (Qodo findings on Python port) #189

@cptkoolbeenz

Description

@cptkoolbeenz

Surfaced from a Qodo /agentic_review on the Python port of PRs #169 + #173 (daqifi/daqifi-python-core PR #100). Both bugs are also present in the C# implementation.

Bug 1 — Gap gate corrupts legitimate multi-chunk frames

In `ProtobufMessageParser.ParseMessages`, the partial-frame wait path advances `currentIndex` whenever `missingPayload > MaxPartialFrameGapBytes`, regardless of how many body bytes have arrived. If a legitimate (large) frame is arriving across multiple reads — e.g. a multi-KB initial-status frame on a transport that returns smaller reads — this skips into the real prefix/payload, corrupting the frame because callers drop `consumedBytes` unconditionally.

The 4 KB cap explicitly leaves `headroom for any legitimate frame including initial-status frames a few KB at most`, so multi-KB frames are expected. Whether such a frame presents as "partial" depends on read chunking.

Fix (already shipped in Python port): only apply the gap-based heuristic when ≤1 body byte has arrived. The field-tag gate already covers the case where any body byte is observable, so the gap gate's job is just to cover the pure-prefix-no-body case where field-tag validation can't apply.

var availableBodyBytes = remainingData.Length - prefixBytes;
var missingPayload = messageLength - availableBodyBytes;
var firstBodyByteIsGarbage = availableBodyBytes > 0
    && !IsPlausibleFieldTagByte(remainingData[prefixBytes]);

var gapIsSuspicious = availableBodyBytes <= 1
    && missingPayload > MaxPartialFrameGapBytes;

if (gapIsSuspicious || firstBodyByteIsGarbage)
{
    currentIndex++;
    consumedBytes = currentIndex;
    continue;
}
break;

Bug 2 — IsPlausibleFieldTagByte skips wire-type validation on multi-byte tags

`IsPlausibleFieldTagByte` early-returns `true` for any byte with the continuation bit set (`b & 0x80 != 0`), bypassing wire-type validation. But wire type lives in the low 3 bits of the first byte regardless of whether the field number spans multiple bytes, so impossible wire types (3,4,6,7) should be rejected even on continuation bytes.

Fix (already shipped in Python port):

private static bool IsPlausibleFieldTagByte(byte b)
{
    // Wire type lives in the low 3 bits regardless of the continuation bit.
    var wireType = b & 0x07;
    if (wireType is 3 or 4 or 6 or 7)
    {
        return false;
    }

    // Continuation bit set → multi-byte tag. Field number spans more
    // bytes which we may not have yet; accept once wire type passed.
    if ((b & 0x80) != 0)
    {
        return true;
    }

    var fieldNumber = (b >> 3) & 0x0F;
    return fieldNumber != 0;
}

Refs

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions