Repository navigation
perf(security): necessary-condition guards for ScanSQL [W6-01] - #900
Conversation
ScanSQL ran three expensive regex paths on inputs that cannot match them.
Three guards now skip that work when a match is impossible:
- hasDollarOpener: no '$' can open a dollar quote, so the 32-state
dollar-quote stripping lexer is skipped.
- hasEqualIdentPair: around some '=', a suffix of the left identifier run
equals a prefix of the right run (case-insensitive, at most 51 bytes, the
regex's capture bound). Otherwise the identifier tautology regex is
skipped. The 51-byte cap keeps the guard linear; the audit prototype was
quadratic on long unequal identifiers (64 KB took ~1.8 s).
- hasTableFnCandidate: some '(' follows a listed ClickHouse table-function
name preceded by whitespace. Otherwise the FROM/JOIN table-function regex
is skipped.
Each guard is a necessary condition and the regex still confirms every
finding. The identifier and table-function guards run only on ASCII input,
because (?i) also folds U+017F and U+212A. Findings are identical: an
equivalence test runs the corpora plus 200k seeded inputs (20k under -race
or -short), and the interesting inputs seed FuzzScanSQL. A scaling test
counts comparisons to keep hasEqualIdentPair linear.
M4, GOGC=off -cpu 1, 10 interleaved rounds: ScanSQLRW per statement -57%
to -69% per dialect, geomean -66.0%.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Claude finished @ajitpratap0's task in 25s —— View job Review of PR #900: ScanSQL necessary-condition guardsVerdict: looks good. I have no blocking issues. I read the diff and Correctness of the guards
Minor suggestions (non-blocking)
Tests
Performance and security
Because it is a draft held for W0-02 and W0-09, merging after those land seems right. |
…-01] Merge origin/main (wave 0). The wave-0 matrix owns BenchmarkScanSQLRW. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
|
Claude finished @ajitpratap0's task in 26s —— View job Review of PR #900: ScanSQL necessary-condition guardsVerdict: no blocking issues. I read the diff and Correctness
Suggestions (non-blocking)
Tests, performance and security
|
Perf gate: PASS (shadow mode: report only, this check never fails)Base
Rules: time regresses at >= +3% with p < 0.01 (Mann-Whitney U), or a suite geomean >= +1.5% (per-round geomeans, same test); allocs/op must not increase; B/op must not rise more than 2%; instructions/op must not rise more than 1.5%. An A/A control with more than 10% of benchmarks moving over 3% is noisy: re-run once, then inconclusive (neutral). Gated suite (GOGC=off, GOMAXPROCS=1)
A/A control: 0 of 32 benchmarks moved more than 3%. Instructions per operation (cmd/perfdriver)
Secondary metric: default GC, GOMAXPROCS=1 (informational)
Environmentbenchstat (primary metric) |
Merge origin/main (keep both CHANGELOG entries). Add tests that kill two surviving mutations: the 51-byte identifier capture bound and the 16-byte azureBlobStorage table-function name; the latter also locks the guard name set to the regex alternation in scanner.go. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
|
Claude finished @ajitpratap0's task in 23s —— View job Review of PR #900: ScanSQL necessary-condition guardsVerdict: no blocking issues. I read Correctness
Suggestions (non-blocking)
Tests
Performance and security
|
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
|
Claude finished @ajitpratap0's task in 19s —— View job Review of PR #900: ScanSQL necessary-condition guardsVerdict: no blocking issues. I read Correctness
Earlier suggestions: status
Remaining nits (non-blocking)
Performance and security
|
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
|
Claude finished @ajitpratap0's task in 23s —— View job Review of PR #900: ScanSQL necessary-condition guardsVerdict: no blocking issues. I read Correctness
Nits (non-blocking)
Performance and security
Test coverage: strong. It has on/off equivalence, boundary tests at the 51/52-byte capture limit, a regex-name lockstep test, Unicode folding cases and fuzz seeds. |
Summary
Wave-1 task W6-01 from the 2026-11 performance audit (SEC-1). The prototype was confirmed by an independent verifier. Codex ported it, and Opus reviewed it and ran the full gate. Review found that the audit's prototype was quadratic on adversarial input, and this PR fixes that.
ScanSQL ran three expensive regex paths on inputs that cannot match them. Each now has a guard that skips the work when no match is possible:
hasDollarOpener: some$can open a dollar quote (the lexer's own opener condition)hasEqualIdentPair: around some=, a suffix of the left identifier run equals a prefix of the right run, case-insensitively, up to 51 bytes\b([a-z_][a-z0-9_.]{0,50})[\s\v]*=[\s\v]*([a-z_][a-z0-9_.]{0,50})\bhasTableFnCandidate: some(follows a listed ClickHouse table-function name that is preceded by whitespaceFROM|JOIN <table function>(regexWhy the guards are safe:
hasEqualIdentPairis the regex's own capture limit. It keeps the guard linear: the audit prototype compared every suffix and prefix length, so a 64 KB input of unequal identifiers took about 1.8 s.(?i)also folds U+017F (ſ) to S and U+212A (K) to K. Non-ASCII input takes the unguarded path.Test Plan
TestScanSQLGuardsEquivalence. Findings are identical with the guards on and off over every statement and whole file intestdata/realworld/*andtestdata/corpus/*, plus 200,000 seeded random and mutated inputs. Those include U+017F, U+212A, dollar quotes, identifier pairs and table functions.-raceor-short, a deterministic 20,000-input subset runs instead. The full corpus and the Unicode cases always run. Under-raceit finishes in 19.6 s; the whole security package takes 31.6 s.TestHasEqualIdentPairLinear. A timing-ratio test (wall clock, not a comparison count): min of 3 samples at 16 KiB vs 64 KiB, fails if the ratio exceeds 10x. Before the fix, quadrupling the input cost 16.3x; after, about 3.5-3.7x (stable over 30 normal and 10-raceruns).TestHasEqualIdentPairCaptureBoundary(new). Border-free 51-byte equal identifiers on both sides of=(plain, spaced, case-folded) plus 52-byte cases checked against the regex-only path. Kills thek <= 51tok <= 50mutation.TestTableFnGuardCoversEveryRegexName(new). Extracts every name from the ClickHouse alternation inscanner.go, requires the guard name set to match it exactly, and asserts the guard andScanSQLfire for each name (mixed case, tab/VT whitespace). Kills the 16-byteazureBlobStoragelength mutation and lockschNamesto the regex.FuzzScanSQLseed corpus.go run honnef.co/go/tools/cmd/staticcheck@latest ./pkg/sql/security/...go test -race -count=1 ./pkg/sql/security/...task checktask test:cbindinggo test -count=1 ./pkg/compatibility/...Re-measured after merging wave 0 (
task bench:ab BASE=origin/main BENCH='./pkg/sql/security=^BenchmarkScanSQLRW$', Apple M4, shared machine so indicative only, 10 interleaved rounds, benchstat). Uses the wave-0 matrixBenchmarkScanSQLRW; the Codex duplicate benchmark was removed.B/op -0.01%, allocs/op unchanged. Absolute numbers are inflated by machine load; the earlier quiet-machine run gave -66.0% geomean. Both exceed the plan's -45% target.
Reviewer re-measurement (6 interleaved rounds vs base
c05c5fd5,ab.sh, M4 shared):ScanSQLRWimproved significantly on all 8 dialects, -63.5% to -67.4% (p=0.002 each).ScanSQLParams: pg_dollar -95%, qmark -73%, at -74%.ScanSQLLargethroughput 30KB +11.4%, 1MB +12.1%. B/op -0.01%; allocs/op did not rise (30KB 32 to 31). So the earlier 'noisy' dialects are in fact significant.amd64 evidence for the target (CI, run 41333802244, workflow_dispatch gate mode, base origin/main vs scratch branch
perf/w6-01-gate-probe= this PR plus the single line./pkg/sql/security ^BenchmarkScanSQLRW$inscripts/perf/gate-suite.txt; 10 interleaved rounds, GOGC=off GOMAXPROCS=1): verdict pass.This is the authoritative amd64 confirmation (matches the M4 figures). The PR's own shadow gate still runs the narrower suite on main (parse/tokenize: no regression, suite geomean -0.03%). Widening
gate-suite.txtpermanently (CRITIQUE item 27) is left to a W0-lane PR; the probe branch is scratch and not for merge.Follow-ups (not in this PR):
TestHasEqualIdentPairLinearis a wall-clock ratio test, like the existingTestWPPScanSQLLinear; W0-04 owns converting those to deterministic checks.isASCII(view)is recomputed per guarded pattern (cache once per view under W6-08/W6-10).hasEqualIdentPaircould be made O(51) per=with Z/KMP if it ever matters.Update: merged origin/main (69a5a72, W2-01) to resolve the CHANGELOG conflict; AC-STD gate re-run green. Wave-0 goldens (
testdata/goldens/security.sha256) pass unchanged: no golden changed.Breaking Changes
None. Findings are byte-identical, and the API is unchanged.
🤖 Generated with Claude Code
https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2