test: add metadata/type/row fuzz targets (layer 2/4) - #424
Open
Saurabh Singh (saurabh500) wants to merge 3 commits into
Open
Saurabh Singh (saurabh500) wants to merge 3 commits into
Saurabh Singh (saurabh500) wants to merge 3 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## saurabh500-bound-tds-response-allocations #424 +/- ##
=============================================================================
+ Coverage 82.25% 82.48% +0.22%
=============================================================================
Files 35 35
Lines 7096 7096
=============================================================================
+ Hits 5837 5853 +16
+ Misses 992 978 -14
+ Partials 267 265 -2 🚀 New features to boost your workflow:
|
David Levy (dlevy-msft-sql)
left a comment
David Levy (dlevy-msft-sql)
left a comment
There was a problem hiding this comment.
Review
One blocker, and it is the same blocker for #425, #426, #428 and #429. This branch was cut 41 commits ago and never rebased, so its test file calls a version of the fuzz harness that no longer exists. go build passes — the production code is fine — but the test binary does not compile, which means none of the five PRs above this point can run their tests.
The good news is that the design is sound and I verified it end to end: once rebased, the targets this PR adds find two real out-of-bounds panics, and #425 fixes both. The fix is small and I have tested it.
What I verified locally
Merge refs, Go 1.27.0, GOTOOLCHAIN=local semantics matched to CI:
| PR | go build ./... |
go vet ./... |
|---|---|---|
| #419 | PASS | PASS |
| #421 | PASS | PASS |
| #424 | PASS | FAIL |
| #425 | PASS | FAIL (inherited) |
| #426 | PASS | FAIL (inherited) |
| #428 | PASS | FAIL (inherited) |
| #429 | PASS | FAIL (inherited) |
| #410 | PASS | PASS |
Blocker: the harness moved, this branch did not
drainSingleResponse and frameReplyPackets were rewritten on #419 — commit 326d4a8, "widen fuzz fragmentation arg to uint16 for full seam coverage", plus a collect flag and a dbState return. 19 commits on #419/#421 touch token_fuzz_test.go. This branch still calls the original shape:
| Branch | Signature |
|---|---|
| #419, #421 | drainSingleResponse(stream []byte, chunk int, collect bool) (tokens, dbState, sawError, framed) |
| #424 → #429 | drainSingleResponse(stream []byte, frag byte) (tokenTypes, sawError, framed) |
GitHub's merge ref takes the newer harness from #419 and the older callers from here, so:
types_fuzz_test.go:67: cannot use frag (variable of type byte) as int value in frameReplyPackets
types_fuzz_test.go:141: ucs2 redeclared in this block
token_fuzz_test.go:326: other declaration of ucs2
types_fuzz_test.go:397: assignment mismatch: 3 variables but drainSingleResponse returns 4 values
types_fuzz_test.go:451: (same)
Ancestry, measured:
#421 on #419: 3 commits behind
#424 on #421: 41 commits behind <== the break
#425 on #424: 0 behind
#426 on #425: 0 behind
#428 on #426: 0 behind
#429 on #428: 0 behind
Everything from #425 up is stacked perfectly. This one joint is the entire problem.
The fix, tested
The rebase itself is clean — this PR only adds types_fuzz_test.go, so there is no textual conflict. The breakage is purely semantic, and it is 7 insertions / 17 deletions in one file:
fuzzTypeReadBuffer(payload []byte, frag byte)→chunk int, and pass it straight toframeReplyPackets.- Delete the local
ucs2— #419 now provides it intoken_fuzz_test.go. - Both
drainSingleResponse(stream, frag)→drainSingleResponse(stream, int(frag), false)with four return values. - Three
fuzzTypeReadBuffer(payload, frag)call sites →int(frag).
After that, go vet . is clean, and cherry-picking #425 on top applies cleanly and stays vet-clean.
The targets work, and they find real bugs
Run on the rebased tree, 20s each, -parallel 4:
FuzzColMetadataAndRow PASS 31208 execs (10346/sec), new interesting: 7
FuzzNbcRow PASS 40928 execs (13639/sec), new interesting: 20
FuzzPLPValue PASS 3628 execs (1207/sec), new interesting: 8
FuzzTypeInfoAndValue FAIL panic: slice bounds out of range [:49] with capacity 48
FuzzVariantValue FAIL panic: slice bounds out of range [:4] with capacity 3
Both crashers appeared within seconds. I then cascaded #425 on top and re-ran the two saved inputs:
FuzzTypeInfoAndValue 0548d93a14ea821c -> FIXED (passes)
FuzzVariantValue 0d5d5b66a69d72c6 -> FIXED (passes)
So the layering does exactly what it claims. That is a good result and worth stating plainly: the targets are not decorative, they caught two genuine out-of-bounds reads in the type decode and SQL_VARIANT paths, and the fix layer closes them.
Suggestion: prefer widening the fuzz input over casting it
int(frag) compiles, but it keeps the fragmentation input a byte, so the targets here only ever explore seams at offsets 0-255. #419's 326d4a8 widened exactly this for "full seam coverage". Changing frag byte to chunk uint16 in the f.Fuzz signatures would match that intent rather than adapting around it. Slightly larger diff, but it is the difference between the seam coverage the harness was upgraded to provide and the coverage this file had before.
Nit: why CI never caught this
Not a change request, just so it is not mistaken for a CI gap you need to fix here. pr-validation.yml triggers on pull_request: branches: [main], and every PR in this stack targets another stack branch, so it never runs. AppVeyor is the only check that builds these merge refs — which is why #429 showed a red appveyor/pr while everything else looked green.
Verdict
- Blockers: the rebase. Everything from here up is untestable until it lands.
- Suggestions: widen the fragmentation input to
uint16rather than casting. - Nits: none in the code itself.
Rebase order once you start: #421 onto #419, then this onto #421 with the four edits, then #425 → #426 → #428 → #429 replay cleanly (each is a single commit sitting exactly on its parent's tip). Happy to push the rebase if you would rather not hand-run it.
Add types_fuzz_test.go with five structured TDS parser fuzz targets seeded with valid wire-format COLMETADATA/TYPE_INFO bytes: - FuzzColMetadataAndRow: full COLMETADATA + ROW + DONE across every type family (fixed, byte-len, date/time, short-len, PLP, long-len, sql_variant). - FuzzNbcRow: NBCROW null-bitmap decoding around the 7/8/9 and 15/16/17 column byte boundaries. - FuzzTypeInfoAndValue: readTypeInfo + value Reader for each valid typeId. - FuzzVariantValue: readVariantTypeWithEncoding across supported subtypes. - FuzzPLPValue: readPLPType with NULL/unknown-length/single/multi-chunk/ empty/missing-terminator streams for each PLP-backed type. Direct-read targets recover expected parser rejections (StreamError / badStreamPanicf) but let runtime panics (slice bounds, nil deref) escape so the engine records them as crashers. Two committed crashers guard real production slice-bounds bugs found while fuzzing (kept as regression seeds until fixed): - readByteLenTypeWithEncoding slices ti.Buffer[:size] with a per-value length byte that can exceed the metadata-declared column size. - readVariantTypeWithEncoding passes an undersized data buffer to decodeDateTimeOffset/decodeDateTime2, yielding a negative slice bound. Refs #418 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Remove the two committed crashers so the Layer 2 branch is green on its own base. The minimized inputs are recorded in the crash report; the nested fix session (stacked on this branch) will re-add them as regression seeds once readByteLenTypeWithEncoding and readShortLenType are bounded. Refs #418 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The layer 1 harness changed on #419 after this branch was cut: the fragmentation argument widened from byte to int, drainSingleResponse gained a collect flag and a dbState return, and ucs2 moved to token_fuzz_test.go. Update the layer 2 call sites to match. The f.Fuzz signatures keep frag as a byte so the committed corpus files stay valid; only the calls into the harness are converted.
David Levy (dlevy-msft-sql)
force-pushed
the
saurabh500-fuzz-layer2-types
branch
from
August 23, 2026 05:58
7141a8e to
7a4e109
Compare
Copilot started reviewing on behalf of
David Levy (dlevy-msft-sql)
August 23, 2026 05:59
View session
There was a problem hiding this comment.
Pull request overview
This PR adds Layer 2 structured fuzz targets (metadata/type-info/row, sql_variant, and PLP value decoding) to expand coverage of the TDS query-response parser hardening effort tracked in #418. It builds valid wire-format seeds using helpers from the Layer 1 fuzz harness and then lets the Go fuzz engine mutate those structured inputs.
Changes:
- Add
FuzzColMetadataAndRowandFuzzNbcRowto exerciseCOLMETADATA+ROW/NBCROWdecoding via the full response parser. - Add direct fuzz drivers for type-info/value decoding,
sql_variant, and PLP-backed values using realtdsBufferreads (including cross-packet seams). - Add a small internal test-only framework for classifying “expected parser rejection” panics vs. crash-worthy runtime/string panics.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+312
to
+327
| func variantSubtypeValues() [][]byte { | ||
| return [][]byte{ | ||
| variantValue(typeInt4, nil, []byte{4, 3, 2, 1}), | ||
| variantValue(typeBit, nil, []byte{1}), | ||
| variantValue(typeInt8, nil, []byte{8, 7, 6, 5, 4, 3, 2, 1}), | ||
| variantValue(typeFlt8, nil, make([]byte, 8)), | ||
| variantValue(typeMoney, nil, make([]byte, 8)), | ||
| variantValue(typeDateTime, nil, make([]byte, 8)), | ||
| variantValue(typeGuid, nil, make([]byte, 16)), | ||
| variantValue(typeDecimalN, []byte{18, 0}, []byte{0x01, 0x2a, 0, 0, 0}), | ||
| variantValue(typeBigVarChar, append(collationBytes(), 0, 0), []byte("abcd")), | ||
| variantValue(typeNVarChar, append(collationBytes(), 0, 0), ucs2("ab")), | ||
| variantValue(typeTimeN, []byte{7}, make([]byte, 5)), | ||
| variantValue(typeBigVarBin, []byte{0, 0}, []byte{1, 2, 3, 4}), | ||
| } | ||
| } |
Comment on lines
+32
to
+36
| // isExpectedParserPanic reports whether a recovered panic value represents a | ||
| // deliberate parser rejection of a malformed stream (acceptable) rather than a | ||
| // real bug. StreamError and the plain fmt.Errorf values raised by | ||
| // badStreamPanicf both satisfy the error interface but are NOT runtime.Error; | ||
| // runtime panics (index out of range, slice bounds, nil deref) implement |
Comment on lines
+529
to
+533
| for sel := range plpFuzzTypeIds { | ||
| for _, s := range seeds { | ||
| f.Add(byte(sel), s, byte(0)) | ||
| } | ||
| } |
Rebase pushed — and a correction to my earlier reviewI pushed the rebase ( CorrectionMy earlier review said the test files "don't compile" without qualifying which ref. That was
This was staleness, not a code defect. Apologies for the framing. What I changedTwo of your commits cherry-picked onto #421 unchanged, keeping your authorship, plus one
I withdrew my own suggestionI had suggested widening the Widening the type would invalidate all four. Keeping Verified after the rebaseAnd with #425 applied, 15s each: One thing worth flagging: the quarantine commit
Recommend #424 and #425 land together, or #425 immediately after. Verdict
I had to cascade the same fix through #425–#429 — #426 had the identical drift in |
This was referenced Aug 23, 2026
Benchmark Results (main vs PR)Click to expand benchstat outputGenerated by CI — commit 207db9a |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Layer 2/4: metadata / type-info / row / variant / PLP fuzz targets
Part of the proactive TDS parser fuzz-hardening effort tracked by #418. Stacks on #421 (
saurabh500-bound-tds-response-allocations), which itself stacks on the Layer 1 harness PR #419. Base branch:saurabh500-bound-tds-response-allocations.Adds
types_fuzz_test.go(packagemssql) with five structured fuzz targets. Because raw random bytes almost never form a validCOLMETADATA/TYPE_INFOstream, every target is seeded with real wire-format bytes assembled by builders and reuses the Layer 1 harness helpers (frameReplyPackets,newFuzzSession,drainSingleResponse).Targets
COLMETADATA+ROW+DONEseeded with one column of every type family: fixed, byte-len (incl. decimal/numeric with prec+scale, guid, legacy char/binary, date/time N at valid scales), short-len (with collation), PLP/(max), long-len (text/ntext/image), andsql_variantcarrying many subtypes.NBCROWnull-bitmap decoding with column counts of 7/8/9 and 15/16/17 to hit null-bitmap byte boundaries, across several null masks.readTypeInfodirectly with a fuzzedtypeId+ metadata, then runs the installed valueReader, seeded with every validtypeId.readVariantTypeWithEncodingwith fuzzed size/vartype/propbytes/payload, seeded with each supported subtype.readPLPTypewith NULL / unknown-length / single-chunk / multi-chunk / empty / missing-terminator streams for each PLP-backed type.Invariants enforced
Never hang, never OOM; the only acceptable failure is a recovered parser rejection (
StreamErrororbadStreamPanicf). Runtime panics (index/slice OOB, nil deref) are not swallowed, so the engine records them as crashers. Inputs > 64 KiB are skipped.Fuzzing results (180 s each,
-parallel=6, 32-core/128 GB)Real bugs found (crashers quarantined, not committed here)
Both reproduce deterministically and are not covered by #421. They are handed to a dedicated nested fix session stacked on this branch, which will add the bounds checks + regression tests + the minimized seeds (so seed and fix land green together). The crasher corpus entries are intentionally not committed on this branch so
go test .stays green on its own base; the minimized inputs live in the crash report.readByteLenTypeWithEncodingslice OOB (types.go:397). The per-value length byte is used to sliceti.Buffer[:size]without checking it against the metadata-declared column size, so a value-length byte larger than the column size panics withslice bounds out of range. Sibling issue confirmed atreadShortLenType(types.go:522). Repro:typeId=0x26, meta20 30, value30.sql_variantdate/time slice OOB (types.go:1063,decodeDateTimeOffset).readVariantTypeWithEncodingsizes the data buffer assize-2-propbytesand hands an undersized buffer todecodeDateTimeOffset/decodeDateTime2, producing a negative slice bound. Repro payload hex:0d0000002b073030303030303030303030.go build ./...,go vet ., gofmt, andgo test -run 'Fuzz|Test' .are all green on this branch.Refs #418