fix(grammar): stop the Swift scanner shifting past the width of an int - #1977
fix(grammar): stop the Swift scanner shifting past the width of an int#1977CaptainMittens wants to merge 1 commit into
Conversation
The Swift scanner keeps a 64-bit mask of the symbols that suppress a
match -- the rule that stops `try!` emitting its `!` as a token of its
own. It tests one bit per candidate:
uint64_t suppressing_symbols = OP_SYMBOL_SUPPRESSOR[full_match];
for (uint64_t suppressor = 0; suppressor < TOKEN_COUNT; suppressor++) {
if (!(suppressing_symbols & 1 << suppressor)) {
The mask is uint64_t but the literal `1` is an int, so the shift is an
int shift. TOKEN_COUNT is larger than 32, so once suppressor reaches 31
the shift runs past the width of the type. That is undefined behavior,
and every bit above 31 is tested against a value the standard does not
define.
Nothing caught it because nothing in the tree reached the suppressor
path. Any Swift force-unwrap does: `cached!` is enough.
UBSan reports it as:
scanner.c:514:47: runtime error: left shift of 1 by 31 places
cannot be represented in type 'int'
`1ULL` makes the literal as wide as the mask it is tested against.
The new test in tests/test_extraction.c cannot go red on its own. The
normal test build prints the UBSan message and carries on, which is why
this survived. The Windows CLANGARM64 leg runs UBSan in trap mode, and
there the same shift is an illegal-instruction crash -- so the test
exists to make sure that leg keeps parsing a force-unwrap at all.
scripts/vendored-checksums.txt records the new hash for the one changed
file, as scripts/security-vendored.sh --update writes it. Layer 8 of the
security gate compares vendored content against that manifest, so the
edit and its recorded hash belong in the same commit.
Found while adding Swift URL extraction in DeusData#1892 / DeusData#1976, and split out
of that PR so the vendored change can be reviewed on its own.
Signed-off-by: Joshua Richter <jrichter5781@gmail.com>
|
Thanks for opening this — it has been seen, and it is queued. This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence. Current review status: working through a backlog. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
If this fixes a bug, a reproduction we can run is worth more than a description of the symptom. Thanks for contributing, and sorry in advance for the wait. |
Upstream already fixed this, and it fixed a second shift in the same fileI went to send this fix to Our vendored copy is five months stale. This exact one-token change landed upstream on 2026-04-06, 17 days after our pin, - if (!(suppressing_symbols & 1 << suppressor)) {
+ if (!(suppressing_symbols & 1ULL << suppressor)) {So this is not my reading of the standard. The grammar's own author made the identical The second bug, which this PR does NOT fixUpstream made a second shift fix on 2026-08-10 in - 1UL << FAKE_TRY_BANG, // BANG,
+ 1ULL << FAKE_TRY_BANG, // BANG,We still carry the Two things make that worse than the bug this PR fixes:
I have not proved that it does fold to 0, and I am not claiming a visible symptom. I am What I suggestRe-vendoring the Swift grammar from a newer upstream commit gets both fixes and For now I would add the second one-token fix to this PR. It is the same defect, in the |
|
Answering my own question above so you are not left holding it: I filed the tracking Two things changed my mind after I posted that comment:
So this PR is unchanged — same branch, same commit One deliberate choice worth flagging: I used The line-131 fix will come as its own PR that closes #1978, once you have had a chance to |
|
The follow-up I mentioned is now open as #1986 — the second shift in this file, It is stacked on this branch rather than cut from #1986 carries |
One token in a vendored grammar. Split out of #1976 so the vendored change can be reviewed on its own.
The bug
The Swift scanner keeps a 64-bit mask of the symbols that suppress a match — the rule that stops
try!emitting its!as a token of its own. It tests one bit per candidate (internal/cbm/vendored/grammars/swift/scanner.c:514):The mask is
uint64_t, but the literal1is anint, so this is anintshift.TOKEN_COUNTis larger than 32, so oncesuppressorreaches 31 the shift runs past the width of the type — undefined behaviour, and every bit above 31 is tested against a value the standard does not define.1ULLmakes the literal as wide as the mask it is tested against.Why it went unnoticed
Nothing in the tree reached the suppressor path. Any Swift force-unwrap does —
cached!is enough — and no test used one until I added Swift URL extraction (#1976), where real code writesURL(string: "…")!.What the test can and cannot prove
I want to be straight about this rather than call it a red-first test, because it is not one.
In the normal test build UBSan prints and carries on, so the suite reports
PASSeither way. That is exactly why the bug survived. Reverting the fix and rebuilding shows:and with the fix that line is gone, with 0
runtime errorhits across the wholeextractionsuite.The enforcement is elsewhere, and to be exact about where: the Windows CLANGARM64 leg (
windows-11-arm) runs UBSan in trap mode (_test.yml:455), where the same shift is an illegal-instruction crash rather than a message. That leg lives in the broad matrix (_test.yml:63), which only turns on whenbroad_platforms: true— and onlyrelease.ymlsets it. So the trap does not fire in pull-request CI. It fires on a release run. The test exists so that leg keeps parsing a force-unwrap at all — a coverage guarantee, not an assertion.Files
internal/cbm/vendored/grammars/swift/scanner.c1→1ULLon line 514scripts/vendored-checksums.txtscripts/security-vendored.sh --updatewrites ittests/test_extraction.cswift_force_unwrap_scanner_shift, which parseslet u = cached!Layer 8 of the security gate compares vendored content against that manifest, so the edit and its recorded hash have to land together.
scripts/security-vendored.shexits 0 on this branch.Two things worth your call
Refs #1978 — the tracking issue for this defect, per CONTRIBUTING.md. Deliberately
Refsand notFixes: #1978 covers two shifts in this file, and this PR fixes onlythe one at line 514. The second,
1UL << FAKE_TRY_BANGat line 131, is undefined onWindows and needs its own follow-up, and that follow-up is the PR to close the issue. Say the
word if you would rather I use
Fixeshere and track line 131 separately.Refs #1892