ci: add scheduled fuzz workflow (layer 4/4) - #429
Saurabh Singh (saurabh500) wants to merge 1 commit into
Conversation
David Levy (dlevy-msft-sql)
left a comment
David Levy (dlevy-msft-sql)
left a comment
There was a problem hiding this comment.
AppVeyor failure is inherited from #424, not from this PR
continuous-integration/appveyor/pr failed on all 5 matrix jobs (build 1856) with a compile error, not a test failure:
.\types_fuzz_test.go:397:41: not enough arguments in call to drainSingleResponse
have ([]byte, byte)
want ([]byte, int, bool)
This PR adds one file (.github/workflows/fuzz.yml, 66 lines, zero Go), so it can't be the source. I compiled each merge ref in the stack locally:
| PR | branch | merge ref compiles |
|---|---|---|
| #419 | fuzz-tds-response-harness | yes |
| #421 | bound-tds-response-allocations | yes |
| #424 | fuzz-layer2-types | no — 3 errors, first break |
| #425 | fix-422-423-decoders | no |
| #426 | fuzz-layer3-complex-tokens | no — 4 errors |
| #428 | fix-427-loginack | no |
| #429 | fuzz-nightly-ci | no |
The stack was never rebased after #419 was revised during review. Three generations of drainSingleResponse are live at once:
| Branch | Signature |
|---|---|
saurabh500-fuzz-tds-response-harness (#419) |
(stream []byte, chunk int, collect bool) |
saurabh500-bound-tds-response-allocations (#421) |
(stream []byte, frag byte, collect bool) |
| #424 / #425 / #426 / #428 / #429 | (stream []byte, frag byte) |
Each branch is internally consistent, which is why appveyor/branch is green on all of them and only appveyor/pr fails. The merge ref takes the newer token_fuzz_test.go from #419 but keeps the stale call sites in types_fuzz_test.go (added by #424) and token_complex_fuzz_test.go (added by #426), which #419 never touched. frameReplyPackets(..., frag byte) vs (..., chunk int) breaks the same way.
Nothing else caught it because pr-validation.yml is gated on branches: [main] and every PR in this stack targets another stack branch. AppVeyor is the only check compiling the merge ref here.
To fix: rebase from #424 up, converting frag byte to chunk int and adding the collect bool argument at the 4 stale call sites. Nothing to change in #429 for this.
Review of the workflow
A compile failure makes the nightly job report success
mapfile -t TARGETS < <(go test "$PKG" -list '^Fuzz' 2>/dev/null | grep '^Fuzz' || true)
if [ ${#TARGETS[@]} -eq 0 ]; then
echo "No fuzz targets in $PKG; skipping."
exit 0
fiRun against the current (broken) tree:
go test . -list '^Fuzz' -> exit=1
FAIL github.com/microsoft/go-mssqldb [build failed]
2>/dev/null discards the compiler diagnostics, the FAIL line on stdout doesn't match ^Fuzz, || true swallows the exit code, TARGETS ends up empty, and the job exits 0. A broken build turns the fuzzer off silently and the workflow stays green — which is the state this stack is in right now.
set -euo pipefail doesn't help here, and it's set after go version anyway (GitHub's shell: bash already applies -eo pipefail).
Suggestion:
set -euo pipefail
go version
go build ./... && go vet "$PKG"
mapfile -t TARGETS < <(go test "$PKG" -list '^Fuzz' | grep '^Fuzz' || true)At minimum, drop 2>/dev/null and check go test -list's exit status separately from the grep.
timeout-minutes: 90 doesn't hold for workflow_dispatch
Package . has 17 fuzz targets (21 repo-wide, not the 18 in the description). At the 180s default that's ~51 min plus build, which fits. Dispatch with 10m and it's ~170 min, so the job gets killed mid-target and reports a failure with no crasher to show for it. Either derive the timeout from fuzztime x target count, or validate/cap the input.
Hardcoded go-version: '1.25.7'
The devcontainer alignment check only compares go.mod against .devcontainer/Dockerfile, so this is a third unguarded copy of the version. go-version-file: go.mod picks it up from the existing toolchain go1.25.7 line.
No corpus persistence
Every night restarts from the seed corpus, so 180s per target re-explores the same shallow paths indefinitely. Caching ~/.cache/go-build/fuzz between runs is what makes a scheduled fuzzer find anything new.
Minor
fuzz-crashers-${{ strategy.job-index }}— a slug of the package name reads better than an index when triaging an artifact.${{ github.event.inputs.fuzztime || '180s' }}works;${{ inputs.fuzztime || '180s' }}is the current form. The|| '180s'is still needed for thescheduletrigger sincedefault:doesn't apply there.- No
concurrency:group, so a manual dispatch can overlap the nightly run.
Good
Pinned action SHAs match pr-validation.yml and devcontainer.yml exactly. permissions: {} at workflow level with contents: read scoped to the job. fail-fast: false. The -parallel 4 rationale is documented and correct given the allocation caps from #421.
Refs #418 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
f387273 to
6e5760d
Compare
There was a problem hiding this comment.
Pull request overview
Adds scheduled and manually triggered CI fuzzing for all native Go fuzz targets across five packages.
Changes:
- Dynamically discovers and runs fuzz targets with bounded parallelism.
- Supports configurable fuzz duration.
- Uploads fuzz crashers as artifacts.
- Uses pinned actions and minimal permissions.
Suppressed comments (1)
.github/workflows/fuzz.yml:35
- This hard-codes one patch release, unlike the existing CI matrix (
.github/workflows/pr-validation.yml:26) which uses1.25.x. Once a newer 1.25 patch ships, nightly fuzzing will continue on 1.25.7 and miss its compiler/runtime/security fixes; use the patch-stream selector instead.
go-version: '1.25.7'
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| run: | | ||
| go version | ||
| set -euo pipefail | ||
| mapfile -t TARGETS < <(go test "$PKG" -list '^Fuzz' 2>/dev/null | grep '^Fuzz' || true) |
There was a problem hiding this comment.
Confirming this independently — I found the same thing and reproduced it against this stack's own tree, so it isn't theoretical.
I ran the exact snippet under bash against #424's merge ref while that ref was still broken:
| tree | compiles | go test -list |
this step |
|---|---|---|---|
| #424 head | yes | exit 0, 9 targets | GREEN, "Targets: 9" |
| #424 merge | no | exit 1, FAIL ... [build failed] |
GREEN, "No fuzz targets in .; skipping" |
So on 2026-08-23 this workflow would have reported success on the exact breakage that was sitting in this stack. 2>/dev/null discards the build error, grep matches nothing, || true masks the non-zero status, and the job exits 0.
Suggested fix that keeps the legitimate skip working:
if ! LIST=$(go test "$PKG" -list '^Fuzz'); then
echo "::error::$PKG failed to build"
exit 1
fi
mapfile -t TARGETS < <(printf '%s\n' "$LIST" | grep '^Fuzz' || true)
if [ ${#TARGETS[@]} -eq 0 ]; then
echo "No fuzz targets in $PKG; skipping."
exit 0
fiThe || true on the grep is still correct — "package builds but has no Fuzz functions" is a real skip for ./msdsn, ./batch and friends. It is only the build failure that must not be swallowed.
| jobs: | ||
| fuzz: | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 90 |
There was a problem hiding this comment.
Agreeing, and the count is worse than stated — it's 14, not 13. Measured on this PR's head (6e5760d):
$ go test . -list '^Fuzz'
FuzzParseDAC FuzzParseInstances FuzzReadPrelogin
FuzzEnvChange FuzzFeatureExtAckAndLoginAck
FuzzReturnValue FuzzErrorInfoTokens FuzzAlwaysEncryptedMetadata
FuzzProcessSingleResponse FuzzColMetadataAndRow FuzzNbcRow
FuzzTypeInfoAndValue FuzzVariantValue FuzzPLPValue
So a 10m dispatch is 140 minutes of fuzztime alone against timeout-minutes: 90, before checkout, toolchain setup and per-target corpus loading. The . leg cannot finish.
Two things worth folding into the fix:
The default is also short of the acceptance criteria. #418 asks for "at least three minutes" per target. At 14 targets that is 42 minutes of pure fuzztime for the . leg — under 90, but not by much once overhead lands, and it grows every time a target is added. Deriving the timeout from the budget rather than hardcoding 90 would stop this recurring.
The time is not evenly useful. Measured on the full stack, 15s per target:
FuzzParseInstances 16794 exec/sec FuzzNbcRow 15603 exec/sec
FuzzColMetadataAndRow 14035 FuzzEnvChange 8831
FuzzTypeInfoAndValue 3475 FuzzReadPrelogin 2122
FuzzPLPValue 379
FuzzPLPValue is roughly 40x slower than the fastest targets, with repeated multi-second windows at zero executions. Under a fixed per-target fuzztime it consumes an equal share of the wall clock for a fraction of the coverage — so raising the timeout to fit 14×10m mostly buys more time in the slowest target. Bounding the generated length in that target would be worth more than a longer budget.
David Levy (dlevy-msft-sql)
left a comment
David Levy (dlevy-msft-sql)
left a comment
There was a problem hiding this comment.
Review
Good workflow with one blocker. The hygiene here is better than most of what's already in this repo — actions pinned to SHAs, permissions: {} at the top with contents: read scoped to the job, fail-fast: false, an rc accumulator so every target runs and a failure still propagates, and crasher artifact upload on failure. The rc pattern in particular is the right call; it would have been easy to exit 1 on the first failing target and lose the rest.
The blocker is in the target-detection step, and I tested it against this stack's own tree.
Blocker: the detection step fails open on a build error
mapfile -t TARGETS < <(go test "$PKG" -list '^Fuzz' 2>/dev/null | grep '^Fuzz' || true)
if [ ${#TARGETS[@]} -eq 0 ]; then
echo "No fuzz targets in $PKG; skipping."
exit 0
fiIf the package does not compile, go test -list exits non-zero and writes the build error to
stderr. 2>/dev/null discards it, grep matches nothing, || true masks the non-zero status,
TARGETS ends up empty, and the job exits 0 reporting "no fuzz targets".
Tested by running that exact snippet under bash against #424's merge ref, which was broken at
the time:
| tree | compiles | go test -list |
this workflow step |
|---|---|---|---|
| #424 head | yes | exit 0, 9 targets | GREEN, "Targets: 9" |
| #424 merge | no | exit 1, FAIL ... [build failed] |
GREEN, "No fuzz targets in .; skipping" |
So the nightly would have reported success on exactly the breakage that was sitting in this stack
until this morning. A fuzz job that goes green when the package cannot build is worse than no fuzz
job, because it produces a daily green signal that means nothing.
Suggested fix — separate "no targets here" from "cannot build":
if ! LIST=$(go test "$PKG" -list '^Fuzz'); then
echo "::error::$PKG failed to build"
exit 1
fi
mapfile -t TARGETS < <(printf '%s\n' "$LIST" | grep '^Fuzz' || true)
if [ ${#TARGETS[@]} -eq 0 ]; then
echo "No fuzz targets in $PKG; skipping."
exit 0
fiThe || true on the grep is still correct there — "package builds but has no Fuzz functions" is
a legitimate skip for ./msdsn and friends. It is only the build failure that must not be swallowed.
Suggestion: go-version: '1.25.7' is pinned to an EOL release
Go 1.25 went end-of-life on 2026-08-19, and main now builds the 1.25.x / 1.26.x / 1.27.x
matrix. A hardcoded patch version will drift silently. Prefer:
- name: Setup go
uses: actions/setup-go@... # v7.0.0
with:
go-version-file: go.modThat tracks the module floor automatically and cannot go stale.
Suggestion: the corpus does not accumulate
Interesting inputs are discarded unless the run fails, so every nightly starts from whatever is
committed under testdata/fuzz/. Over time the job re-explores the same ground instead of
building on yesterday's coverage. Two common options: upload the corpus as an artifact on success
and restore it at the start of the next run, or have a scheduled job open a PR committing new
entries. Worth a follow-up issue rather than blocking this PR.
Nits
set -euo pipefailis on line 2, aftergo version, so the first command runs unguarded.- The artifact name uses
strategy.job-index, so a crasher requires mapping an index back to a
package. Package names contain/so they need sanitising, but${{ matrix.package }}with
/replaced would be friendlier than a bare number. timeout-minutes: 90is fine today. Worth knowing the budget is uneven: measured on the full
stack,FuzzPLPValueruns at ~180–380 exec/sec against 8,000–16,000/sec for most targets, so a
fixed per-targetfuzztimespends most of the wall clock there.
What I verified
Ran all 14 fuzz targets on the complete stack tip, 15s each, -parallel 4 — all pass:
FuzzParseDAC 12895/sec FuzzParseInstances 16794/sec FuzzReadPrelogin 2122/sec
FuzzEnvChange 8831/sec FuzzFeatureExtAckAndLoginAck 13783/sec
FuzzReturnValue 8081/sec FuzzErrorInfoTokens 13616/sec
FuzzAlwaysEncryptedMetadata 8433/sec FuzzProcessSingleResponse 9179/sec
FuzzColMetadataAndRow 14035/sec FuzzNbcRow 15603/sec
FuzzTypeInfoAndValue 3475/sec FuzzVariantValue FuzzPLPValue 379/sec
One FuzzVariantValue run failed and did not reproduce on two retries, writing no corpus file.
I am treating it as a flake rather than a finding, but flagging it because under this workflow a
non-reproducing failure will look like a real crasher and burn triage time.
I did not verify the workflow end to end on GitHub — the fail-open was reproduced locally
under bash, not by pushing a broken tree and watching the job go green.
Verdict
- Blockers: the fail-open in target detection.
- Suggestions:
go-version-file: go.modinstead of the EOL pin; corpus persistence (follow-up). - Nits:
set -euo pipefailplacement, artifact naming, uneven time budget.
Also note this PR now sits on the rebased stack — I force-pushed the cascade this morning, so the
head here has moved. Details on #424.
|
Two follow-ups on the action pins, both minor. The pins themselves are correct. I verified all three SHAs resolve to the tags claimed in the comments:
Nit: Confirmed for the record: this stack adds no dependencies. I checked every PR for Every import introduced across all eight is stdlib, the driver's own |
Benchmark Results (main vs PR)Click to expand benchstat outputGenerated by CI — commit 38880be |
Summary
Final layer (4/4) of the proactive TDS parser fuzz-hardening stack tracked by #418.
This PR adds
.github/workflows/fuzz.yml, a scheduled (nightly 07:00 UTC) + manually-dispatchable workflow that runs every native Go fuzz target in the repo and uploads any crashers as artifacts. These are pure TDS/parser fuzz targets — no SQL Server instance is required.What it does
workflow_dispatch(with a configurablefuzztimeinput, default180s)..,./batch,./integratedauth/ntlm,./internal/querytext,./msdsn) that covers all 18 fuzz targets in the repo.go test <pkg> -list '^Fuzz', then fuzzes each one.-parallel 4: the TDS value decoders eagerly allocate up to a large per-length cap (bounded by fix: bound TDS response parser allocations against malformed streams #421 but still large), so the default 32 workers would OOM a 7GB CI runner. The per-package matrix keeps memory and wall-time bounded.testdata/fuzz/**crashers as artifacts on failure.permissionsconventions frompr-validation.yml.The fuzz-hardening stack
This is the final layer of a dependent stack that proactively fuzzed the TDS parser and fixed the bugs it uncovered:
Stacks on PR #428 (
saurabh500-fix-427-loginack).Refs #418