fix(extraction): reach a Swift URL built by a constructor - #1976
fix(extraction): reach a Swift URL built by a constructor#1976CaptainMittens wants to merge 5 commits into
Conversation
|
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. |
90dd4de to
458f9fc
Compare
|
Reviewed. The extraction change is good and I have nothing to ask of it.
Pulling the Three things about the stack, so you can sequence it without guessing. 1. #1896 should go first, and it can go on its own. It is the base layer, it is self-contained, and — unlike this one — it touches no vendored file, so it has no manifest obligation. Once it merges, this PR narrows to its own commit and gets much easier to read. 2. This PR needs a 3. #1977 is closed — #1986 supersedes it, and that changes one of your assumptions here. #1986 is by you and strictly larger: it fixes The consequence for this branch: this PR's description says the carried grammar commit is byte-identical to the standalone one and will therefore merge without conflict. That was true of #1977 but is not true of #1986 — the superset has a hunk yours does not. So if #1986 lands first, expect this branch to need a rebase rather than to shrink on its own. Worth knowing before it surprises you. One unrelated heads-up: Thanks for splitting the grammar fix out into its own PR rather than leaving it buried in this one, and for laying the stack out in the description — the commit-to-PR table made this quick to follow. |
DeusData
added
bug
handle_calls reads a call's arguments through one tree-sitter field lookup, ts_node_child_by_field_name(node, "arguments"). The vendored Swift grammar declares no "arguments" field at all -- its own ts_field_names[] table has zero occurrences, against one in Go and two in TypeScript. Swift models a call as a target expression plus a call_suffix, and the arguments hang off the suffix as value_arguments. So args was null for every Swift call ever parsed, first_string_arg was never populated, and no Swift HTTP call could raise an HTTP_CALLS edge or a Route node. Alamofire, Moya and URLSession have been in the service-pattern table the whole time and match the callee text correctly; the URL simply never arrived. Three changes, all in extract_calls.c: - swift_call_args() reaches the argument list through call_suffix, in the same shape as the existing objectscript_call_args() fallback and used from the same place. A trailing closure has no value_arguments, so it returns a null node and that call behaves as before. - extract_url_or_topic_arg() unwraps Swift's per-argument value_argument node, stepping past a leading value_argument_label. Without it, dataTask(with: "/api/v1/widgets") yields the label "with" rather than the path. PHP and C# already had the same unwrap for their own "argument" node. - is_string_like() gains line_string_literal, which is what Swift calls an ordinary "..." literal. The list already held raw_string_literal, so only Swift's common case was missing. This is the layer underneath DeusData#1892 rather than the whole of it. A literal URL argument now arrives; a literal nested inside a constructor, as in URLSession.shared.data(from: URL(string: "...")!), still does not, because extract_positional_url reads a literal, a template string, a concatenation or a named constant and that shape is none of them. Reproduce-first: all three tests fail without the fix, two on a null first_string_arg and one on HTTP_CALLS being 0. Fixes DeusData#1892 Signed-off-by: Joshua Richter <jrichter5781@gmail.com>
Swift has no URL literal, so almost no real code passes a bare string to a request. It writes URL(string: "https://…")! instead, and the literal then sits two levels below the argument list: past the trailing "!", which the grammar models as a postfix_expression, and inside the constructor's own value_arguments. extract_url_or_topic_arg saw only the outer node and gave up, so the URL never reached the service-pattern table and no Route node formed. That is the shape issue DeusData#1892 reported from a real project — reaching a bare string argument was only the layer underneath it. swift_unwrap_url_constructor() steps past both wrappers. It unwraps only URL, URLComponents and URLRequest, so any other constructor keeps its own meaning and the outer call does not borrow the inner call's string. A non-literal argument such as URL(string: base + path) falls through to the ordinary handling unchanged. The value_argument unwrap added for the bare-string case is now swift_argument_value(), because the nested argument list needs the same step and the code was identical. Refs DeusData#1892 Signed-off-by: Joshua Richter <jrichter5781@gmail.com>
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>
scanner.c line 131 shifts a literal 1UL by 32 to build the entry that suppresses the bang of a try!. unsigned long is 64 bits on Linux and macOS but 32 bits on Windows, so there the shift count equals the width of the type, which is undefined. 1ULL is 64 bits on every target this project builds for. Nothing reports it today. The expression is a compile-time constant, so no sanitizer sees it run, and Makefile.cbm:719 builds vendored grammars with -w, which switches off -Wshift-count-overflow. Upstream tree-sitter-swift made the same change in 6ab8d1d74ebd, after the commit this grammar is pinned to. Refs DeusData#1892 Signed-off-by: Joshua Richter <jrichter5781@gmail.com>
The vendoring table claims our copy of a grammar matches the pinned upstream commit. A local patch makes that claim false unless the patch is written down, so a future re-vendor would drop the fix without anyone noticing. The row covers both changed lines, because both are the same defect in the same file: OP_SYMBOL_SUPPRESSOR at line 131 and eat_operators at line 514. MANIFEST.md is itself listed in scripts/vendored-checksums.txt, so its own recorded checksum moves with the edit. Refs DeusData#1892 Signed-off-by: Joshua Richter <jrichter5781@gmail.com>
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>
458f9fc to
82e1de8
Compare
|
Manifest row added, branch rebuilt on current The stale assumption, and what I did with itYou were right that "byte-identical, so it merges without conflict" was true of #1977 and false of #1986. Rather than delete the claim, I made it true again: this branch now carries #1986's three commits in full — I checked that rather than assuming it. Diffing That also answers your manifest point: the row arrives with #1986's commit rather than as a separate edit here, so the two branches cannot drift into two different rows for one patch. Current stack
Merge order as you set it: #1896, then #1986, then this one narrows to a single commit. Verified on the rebuilt branch with On the couplingTaken, and it was fair. Five open PRs across two stacks with shared files is how a stale assumption survived into a description in the first place — I could not hold all of it, so I stopped re-reading the parts I thought I already knew. I am opening nothing further on these stacks until #1896 and #1986 are in. |
2 participants
Swift has no URL literal, so real code almost never passes a bare string to a request. It writes
URL(string: "https://…")!, and the literal then sits two levels below the argument list — past the trailing!, which the grammar models as apostfix_expression, and inside the constructor's ownvalue_arguments.extract_url_or_topic_argsaw only the outer node and gave up, so the URL never reached the service-pattern table and no Route node formed. That is the shape #1892 reported from a real project.This PR stacks on #1896 and #1986
GitHub cannot base a cross-fork pull request on a branch in the fork, so this PR shows five commits. Only the second is this PR's own work:
2fd719d8reach Swift call arguments throughcall_suffix68cc0658reach a Swift URL built by a constructor93ebe29estop the Swift scanner shifting past the width of anint8f00939dstop the Swift try-bang entry shifting past its own type82e1de8crecord the Swift scanner shift patch inMANIFEST.md#1896 is the layer underneath: Swift models a call as a target plus a
call_suffix, soargswas null for every Swift call and no URL ever arrived. Reaching a bare string argument was only half the problem — this PR reaches the constructor that wraps it. Merge #1896 and #1986 first and this one narrows to its single commit.The fix
swift_unwrap_url_constructor()steps past both wrappers. It unwraps onlyURL,URLComponentsandURLRequest, so any other constructor keeps its own meaning and the outer call does not borrow the inner call's string. A non-literal argument such asURL(string: base + path)falls through to the ordinary handling unchanged.The
value_argumentunwrap added for the bare-string case becameswift_argument_value(), because the nested argument list needs the same step and the code was identical.The vendored grammar fix, corrected
An earlier version of this description said the carried grammar commit was #1977's and would merge without conflict because it was byte-identical. That is no longer true and the branch has been rebuilt. #1977 is closed; #1986 supersedes it and is strictly larger — it fixes
eat_operatorsas #1977 did andOP_SYMBOL_SUPPRESSOR(1UL <<→1ULL <<), which matters becauseunsigned longis 32 bits under LLP64, so that one is undefined on exactly the Windows CLANGARM64 leg.This branch now carries #1986's three commits in full, including the
MANIFEST.mdrow, rather than #1977's single commit. The vendored diff on this branch and on #1986 are byte-identical again — verified by diffingorigin/main...HEADforswift/scanner.c,MANIFEST.mdandscripts/vendored-checksums.txtacross both branches — so this PR shrinks on its own when #1986 lands instead of needing a rebase.This PR's test is the first thing in the tree to reach the Swift scanner's suppressor path — the rule that stops
try!emitting its!as its own token, which a force-unwrappedURL(...)!hits. Keeping the fix on this branch is what stops that test reaching a release build without it.Tests
Three in
tests/test_extraction.c, one of them a negative control:swift_nested_url_constructor_issue1892URL(string: "…")!— the force-unwrapped shape from the reportswift_nested_url_no_bang_issue1892URLRequest(url: "…")— no!, so nopostfix_expressionwrapperswift_non_url_constructor_untouched_issue1892Formatter(pattern: "%s-%d")must yield no string, so the unwrap cannot over-reachPlus a pipeline test in
tests/test_pipeline.cthat the Route node now forms.Refs #1892