Skip to content

fix(grammar): stop the Swift scanner shifting past the width of an int - #1977

Open
CaptainMittens wants to merge 1 commit into
DeusData:mainfrom
CaptainMittens:fix/swift-scanner-int-shift
Open

fix(grammar): stop the Swift scanner shifting past the width of an int#1977
CaptainMittens wants to merge 1 commit into
DeusData:mainfrom
CaptainMittens:fix/swift-scanner-int-shift

Conversation

@CaptainMittens

@CaptainMittens CaptainMittens commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

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):

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 this is an int shift. TOKEN_COUNT is larger than 32, so once suppressor reaches 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.

1ULL makes 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 writes URL(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 PASS either way. That is exactly why the bug survived. Reverting the fix and rebuilding shows:

swift_force_unwrap_scanner_shift   scanner.c:514:47: runtime error:
    left shift of 1 by 31 places cannot be represented in type 'int'

and with the fix that line is gone, with 0 runtime error hits across the whole extraction suite.

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 when broad_platforms: true — and only release.yml sets 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

File Change
internal/cbm/vendored/grammars/swift/scanner.c 11ULL on line 514
scripts/vendored-checksums.txt the one recorded hash for that file, as scripts/security-vendored.sh --update writes it
tests/test_extraction.c swift_force_unwrap_scanner_shift, which parses let 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.sh exits 0 on this branch.

Two things worth your call

  • Upstream. The same bug is in tree-sitter-swift itself. Happy to send it there if you would rather carry a patch note than a local edit — say the word and I will open it upstream and reference it here.
  • fix(extraction): reach a Swift URL built by a constructor #1976 keeps a copy of this commit. Not because its own CI needs it — the ARM64 trap leg does not run on pull requests — but so fix(extraction): reach a Swift URL built by a constructor #1976 still carries the fix if it merges first, and so neither branch reaches a release build without it. When this merges, that PR's commit list shrinks on its own; the content is byte-identical, so it merges without conflict.

Refs #1978 — the tracking issue for this defect, per CONTRIBUTING.md. Deliberately
Refs and not Fixes: #1978 covers two shifts in this file, and this PR fixes only
the one at line 514. The second, 1UL << FAKE_TRY_BANG at line 131, is undefined on
Windows 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 Fixes here and track line 131 separately.

Refs #1892

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>
@github-actions

Copy link
Copy Markdown

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. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

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.

@CaptainMittens

Copy link
Copy Markdown
Contributor Author

Upstream already fixed this, and it fixed a second shift in the same file

I went to send this fix to alex-pinkus/tree-sitter-swift. It is already there, so
there is nothing to send. That changes the provenance of this PR, and it turned up a
second bug we still carry, so I am writing both down here.

Our vendored copy is five months stale. MANIFEST.md pins the Swift grammar at
8abb3e8b3325 (2026-03-20). Upstream is active — last push 2026-08-30.

This exact one-token change landed upstream on 2026-04-06, 17 days after our pin,
in fb63a7004f07, "Fix UB in eat_operators":

-                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
change and called it undefined behaviour. I confirmed our copy is a faithful vendor and
not a local edit: at 8abb3e8b3325 upstream line 514 was byte-identical to ours.

The second bug, which this PR does NOT fix

Upstream made a second shift fix on 2026-08-10 in 6ab8d1d74ebd, "Fix undefined 32-bit
shift in OP_SYMBOL_SUPPRESSOR on LLP64 targets":

-    1UL << FAKE_TRY_BANG, // BANG,
+    1ULL << FAKE_TRY_BANG, // BANG,

We still carry the 1UL form, at internal/cbm/vendored/grammars/swift/scanner.c:131.
FAKE_TRY_BANG is 32 (I counted the TokenType enum — 33 entries, so index 32). On
LP64, which covers our Linux and macOS legs, unsigned long is 64 bits and the shift is
fine. On Windows unsigned long is 32 bits, so 1UL << 32 shifts by the full width of
the type, which is undefined.

Two things make that worse than the bug this PR fixes:

  • It is a compile-time constant expression, so no sanitizer sees it at run time. The
    compiler folds it and moves on.
  • CLANG64 on windows-latest is in CORE_WIN, so unlike the trap-UBSan ARM64 leg it
    runs on every pull request. If the compiler folds that entry to 0, the try!
    suppression is simply off on Windows, and Swift extraction differs there from Linux
    and macOS with nothing failing to say so.

I have not proved that it does fold to 0, and I am not claiming a visible symptom. I am
reporting the undefined expression and where it sits.

What I suggest

Re-vendoring the Swift grammar from a newer upstream commit gets both fixes and
everything else since March. That is the real answer, but it is a bigger change —
TOKEN_COUNT goes 33 to 34 upstream, so the grammar moved — and it does not belong in
this PR.

For now I would add the second one-token fix to this PR. It is the same defect, in the
same file, and the checksum manifest line is already being changed here, so a follow-up
PR would cost you a second review of the same supply-chain line for one character. Say
the word and I will push it; say no and I will file it separately instead.

@CaptainMittens

Copy link
Copy Markdown
Contributor Author

Answering my own question above so you are not left holding it: I filed the tracking
issue instead. It is #1978, and it covers both shifts in this file rather than only the
one I asked about.

Two things changed my mind after I posted that comment:

  • CONTRIBUTING.md:123 says every PR must reference a tracking issue or it gets closed.
    This PR referenced none, only Refs #1892. So an issue was needed regardless of where
    the second fix lands.
  • Filing both lines together gives you one place to judge the whole defect, and it means
    this PR does not grow while it is already green and waiting.

So this PR is unchanged — same branch, same commit de2ed3b7, no CI restart. Only the
body gained the Refs #1978 line.

One deliberate choice worth flagging: I used Refs rather than Fixes, because #1978
covers two lines and this PR fixes one. Fixes would close the issue on merge and leave
the Windows defect open with nothing tracking it. Tell me if you would rather have
Fixes here and a separate issue for line 131, and I will switch it.

The line-131 fix will come as its own PR that closes #1978, once you have had a chance to
look at the issue.

@CaptainMittens

Copy link
Copy Markdown
Contributor Author

The follow-up I mentioned is now open as #1986 — the second shift in this file, 1UL << FAKE_TRY_BANG on line 131, which is the one that is undefined on Windows.

It is stacked on this branch rather than cut from main, because both changes rewrite the same recorded hash in scripts/vendored-checksums.txt and two independent branches would collide there. So this PR merges first, and #1986 then narrows to a single token.

#1986 carries Fixes #1978; this PR keeps Refs, as described above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants