Skip to content

Extract PLP chunk validation into a sync, unit-testable validator #262

Description

Part of #250.

Problem statement

PlpChunkStreamReader::ensure_active_chunk (mssql-tds/src/datatypes/decoder.rs:242-307) performs six distinct integrity checks on PLP chunk headers, but they are interleaved with async reads. That means they can only be exercised through a full async reader, and no other decode path can reuse them.

The six checks:

  1. terminator handling, plus "chunk stream ended before declared length"
  2. chunks_seen > MAX_PLP_CHUNKS
  3. chunk_len > MAX_PLP_CHUNK_SIZE
  4. checked_add overflow on the running total
  5. next_total > MAX_PLP_SIZE
  6. next_total > declared

The PoC in #238 introduces a second, parallel implementation for its buffered path (validate_plp_chunk, pr238.diff:451) that performs only checks 2 and 3, and re-declares the limit constants locally. Two divergent validators for one wire format is both a correctness and a security hazard: the buffered path would accept streams the async path rejects, and the two would drift as limits change.

Checks 1 and 6 matter beyond hygiene. Together they are what makes a declared PLP length trustworthy — the stream is guaranteed neither to end early nor to overrun what it declared. The RowWriter sink API proposes that LenHint::Exact be trusted by implementors for pre-sizing a destination buffer. That is only sound if these two checks are guaranteed to run.

Proposed solution

Extract a sync, allocation-free validator that owns the counters and the limits:

struct PlpChunkValidator {
    declared: u64,
    total_read: u64,
    chunks_seen: usize,
}

impl PlpChunkValidator {
    fn new(declared: u64) -> Self;

    /// `Ok(true)` to continue reading, `Ok(false)` at a clean end of stream.
    fn accept_chunk(&mut self, chunk_len: u32) -> TdsResult<bool>;

    /// Enforces "ended before declared length".
    fn finish(&self) -> TdsResult<()>;
}

ensure_active_chunk becomes read-header-then-validate, and the buffered path can call the identical validator with no async involvement.

Limits stay centralized rather than being re-declared per call site:

Constant Normal cfg(fuzzing)
MAX_PLP_SIZE i32::MAX 64 KB
MAX_PLP_CHUNKS 100,000 1,000
MAX_PLP_CHUNK_SIZE 16 MiB 8 KB

Affected crate

mssql-tds

Alternatives considered

  • Leave validation inline and duplicate it in the buffered path. This is what the PoC does. Rejected: two validators for one wire format will drift, and the PoC's copy already dropped four of six checks.
  • Make the validator async. Unnecessary — validation is pure arithmetic over header bytes that have already been read. Keeping it sync is precisely what lets both the async and buffered paths share one implementation.
  • Assert rather than return errors. These are protocol-integrity checks against potentially hostile input; they must be recoverable errors, not panics.

Additional context

This is deliberately the smallest and lowest-risk item under #250: a pure refactor with no behaviour change on the async path.

Two things it unblocks:

  • The buffered two-pass decode work, which needs sync validation.
  • The RowWriter sink API's decision to trust LenHint::Exact.

It also makes all six checks directly unit-testable for the first time, without standing up an async reader — relevant to the repo's 85% diff-coverage gate, which is otherwise awkward to hit on this code.

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions