Skip to content

s9: harden the codec/client surface against malformed server input (+ perf, docs, CI) - #449

Open
MattJackson wants to merge 7 commits into
stack/s8from
stack/s9
Open

s9: harden the codec/client surface against malformed server input (+ perf, docs, CI)#449
MattJackson wants to merge 7 commits into
stack/s8from
stack/s9

Conversation

@MattJackson

@MattJackson MattJackson commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Stacked on top of s8 (#447). This is a defensive-hardening + foundation slice, not a feature — it makes the driver robust against a malicious or buggy server, tightens the security surface, and speeds up the per-row read path, without changing any happy-path behavior.

Verification

  • Unit test coverage: 687 → 800 (+113 server-free tests). Every fix ships with a test that reproduces the bad input and asserts the new behavior — e.g. a SmallDateTime minute field ≥ 1440 now returns a protocol error instead of panicking, an over-long B_VARCHAR name errors instead of wrapping its length prefix, ERROR/INFO tokens with a length/content mismatch fail cleanly instead of desyncing the stream. The decode-hardening tests assert against crafted byte buffers, so they run without a server (cargo test --lib).
  • Decode output proven unchanged. The perf and bulk-read changes are covered by the existing round-trip tests plus new packet-boundary tests, so LOB values decode byte-for-byte identically — the change is purely fewer async polls, not different bytes.
  • New state-machine + framing coverage that didn't exist before: the token-stream dispatch/precedence logic, the TLS pre-login write framing, and the bulk-load partial-row rollback are now unit-tested via an in-memory mock connection.
  • Server-verified on the full integration matrix (SQL Server 2019, every feature combo): 798 lib + 120 bulk + all integration/auth lanes, 0 failures — including new server-gated tests for identity/computed column skipping and ntext value round-trips.
  • Clean across 7 feature combinations with zero compiler warnings, clippy --all-targets, and fmt.

Happy-path behavior is unchanged — the changes only reject malformed input and batch the reads.

The 7 commits (the stack reads from the wire inward)

  1. perf(codec): read packet-spanning values in bulk — the LOB value decoders (PLP/TEXT/NTEXT/IMAGE) read one byte per poll_read, i.e. N async polls per N-byte value per row. A new packet-aware read_bytes_into makes it O(packets), keeping the MAX_PREALLOC cap so a lying length can't over-allocate.
  2. fix(codec): reject malformed column values instead of panicking/corrupting — closes a remote-triggerable SmallDateTime panic (and a silent wrong-time on the time backend); checked numeric/sql_variant magnitude arithmetic; scale/precision and DATETIME2/time length validation.
  3. fix(codec): length-bound the token stream and share the framing encoders — ERROR/INFO/LOGINACK now consume exactly their declared Length (a mismatch is a clean error, not a stream desync); replaces four hand-rolled B_VARCHAR u8 counters that wrapped past 255 and corrupted the wire with bound-checked shared helpers; applies the LOGINACK-negotiated TDS version; fails (doesn't downgrade) when Required encryption is declined.
  4. security: identifiers, credentials, connection integrity, TLS — one shared strict validator for the bulk table/column and TVP db_type identifiers (interpolated into a batch because T-SQL can't parameterize them); rejects statement-breakers and token-splicing while keeping multi-part/parameterized names working (documented defense-in-depth). AAD token in Zeroizing; bulk-load + sensitive-login guarded by the poisoned-connection check with partial-row rollback.
  5. fix(macros): spanned compile errors for malformed #[colname] — the derive returns syn::Error instead of panicking inside the proc-macro.
  6. docs(0.13): changelog / README / API docs / #[non_exhaustive] Error.
  7. ci(0.13): pin MSRV 1.88, cargo-deny license checks, a chrono+time cross-feature smoke lane.

Worth explicit attention

  • Error is now #[non_exhaustive] — a deliberate semver call so future variants aren't breaking. Happy to revert if you'd rather not commit to that yet.
  • The #[colname] macro now emits a compile error where it previously panicked — user-visible, strictly better diagnostic.
  • Connection-string encryption defaulting to Required is carried consistent with main (0.13.1) and noted in the changelog; no new decision here.

Reviewable commit-by-commit; happy to split further or drop any commit (e.g. the #[non_exhaustive] one) if you'd prefer to take it separately.

@MattJackson
perf(codec): read packet-spanning values in bulk instead of one byte …
2d85d92 
…per poll

The PLP / TEXT / NTEXT / IMAGE value decoders read their payload one byte
(or one u16) per `poll_read`, turning an N-byte column value into N async
state-machine polls on every row. Add a packet-aware `read_bytes_into`
primitive to `SqlReadBytes` that fills a buffer by copying the largest
available contiguous slice each iteration — O(packets) instead of O(bytes) —
while preserving the existing `MAX_PREALLOC` windowed-reservation cap so a
lying server length can't force a large up-front allocation. Route every LOB
value decoder through it; decode output is byte-for-byte identical, locked by
the existing round-trip tests plus new packet-boundary tests.

Also drop a duplicate packet-header decode in `PacketCodec::decode` (the
length is now peeked inline) and move the `length < HEADER_BYTES` guard ahead
of consuming the header so a malformed short packet is rejected without
draining bytes.
@MattJackson
fix(codec): reject malformed column values instead of panicking or co…
d709f4d 
…rrupting

Server-controlled value bytes could previously panic the connection task or
silently produce wrong data. Harden every value decode/encode path:

- numeric: reconstruct the 12/16-byte magnitude with checked arithmetic and
  reject magnitudes past i128::MAX; use `unsigned_abs` on encode (no i128::MIN
  panic); validate scale/precision (0..=38) at decode before constructing.
- time (chrono + time backends): a SmallDateTime minute field >= 1440 no
  longer panics (chrono) or silently wraps to the wrong wall-clock time (time)
  — both return a protocol error; reject DATETIME2 scale > 9 and out-of-range
  day/offset values; surface out-of-range chrono dates as errors, not asserts.
- sql_variant: checked 16-byte magnitude arithmetic; cross-checked prop/data
  lengths.
- add the length-validation guard the sibling decoders already have to the
  `time`/`guid` column paths; bound-check the interpolated numeric scale;
  guard the XML blob-length multiply against overflow.

`numeric`/`guid` magnitudes also switch to the bulk `read_bytes_into` reader.
Each fix ships with a red-before-green unit test.
@MattJackson
fix(codec): length-bound the token stream and share the framing encoders
3994741 
Make token decode desync-proof and token encode overflow-proof, and align a
few spots with MS-TDS:

- decode: ERROR/INFO/LOGINACK now read exactly their declared Length into a
  bounded buffer (a Length/content mismatch is a clean error, not a stream
  desync); reject odd-length ORDER, out-of-range ENVCHANGE packet sizes, and
  COLINFO entries that overrun their token; strict (non-lossy) UTF-16.
- encode: replace four hand-rolled B_VARCHAR length counters (which wrapped a
  u8 past 255 and corrupted the wire) and the truncating XML/UDT length
  prefixes with shared, bound-checked `encode_b_varchar`/`encode_us_varchar`
  helpers; de-duplicate the ALL_HEADERS block.
- negotiation: apply the LOGINACK-negotiated TDS version to the connection
  context so the version-dependent DONE/ERROR field widths are actually
  reached; fail (don't downgrade) when Required encryption is declined;
  exhaustive TokenType dispatch; preserve a trailing empty result set in the
  command stream.

Adds server-free coverage for the TokenStream state machine (via a test-only
mock connection) and the token length-mismatch paths.
@MattJackson
security: harden identifiers, credentials, connection integrity, and TLS
2be301c 
- SQL identifiers: one shared strict validator for the bulk table/column and
  TVP `db_type` paths (which are interpolated into a batch because T-SQL can't
  parameterize identifiers). It rejects statement-breakers (`;`, quotes, `--`,
  `=`), top-level spaces, and delimiter-adjacent token splicing (`[t]UNION(..)`,
  `foo(1)UNION(..)`) while keeping multi-part names and parameterized types
  working. Documented as defense-in-depth, not a substitute for trust.
- credentials: store the AAD token in `Zeroizing`; surface a GSSAPI error
  instead of unwrapping.
- connection integrity: guard the bulk-load write path and
  `send_sensitive_login` with the poisoned-connection check so a
  cancelled/dropped write can't be followed by a silently-desynced reuse; roll
  back a partial bulk row on encode failure (now tested).
- TLS: role-specific cert/key file-extension checks; named SSRP constants.
@MattJackson
fix(macros): emit spanned compile errors for malformed #[colname]
45c8a2b 
`FieldAttr::parse` panicked inside the proc-macro on a malformed or duplicate
`#[colname]` attribute, giving users a raw macro panic instead of a normal
compiler diagnostic. Return `syn::Error`s pointed at the offending span, like
the rest of the `TableValueRow` derive already does.
@MattJackson
docs(0.13): changelog, README, and API docs
09715f2 
- add a 0.13.0 CHANGELOG section and refresh the README feature table (drop the
  removed sql-browser-async-std, add winauth/sspi-rs/serde).
- add `# Errors`/`# Panics` sections to the public fallible/panicking APIs and
  correct the chrono `DateTime` type-mapping doc tables.
@MattJackson
ci(0.13): pin MSRV, add cargo-deny license checks and a cross-feature…
9870b07 
… lane

- declare `rust-version = 1.88` so the MSRV is enforced by cargo, not just CI.
- add a `[licenses]` allow-list and run `cargo deny check ... licenses`.
- add one Linux smoke lane exercising chrono + time + the decimal crates
  together, which per-feature-isolated lanes couldn't catch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@MattJackson