fix(editor): stop Format Query crashing on an unterminated SQL literal - #2164
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What you see now
Format Query no longer crashes the app on a string literal that is still open and ends in a
backslash, and it no longer pushes the last character of an unclosed
/*comment out of the commentand reformats it as code.
Found while investigating #2158 (PR #2163). Different root cause, different trigger, so it ships on
its own.
Root cause
SQLTokenizer.tokenizehas two branches that mishandle "the input ran out while we were inside aconstruct", in opposite directions.
Overshoot, and it traps. Inside a quoted literal,
if chars[i] == "\\" { i += 2; continue }wasunguarded. With the backslash at the last index
ibecomescount + 1, thewhile i < countloopexits, and
String(chars[start..<i])slices past the end. Array index out of range. A trap isuncatchable, so the
do/catchinSQLEditorCoordinatordoes not help and the app dies.Reproduced with a standalone probe extracting the exact loop:
count=33,i=34, slice[29..<34],SIGTRAP, exit 133. All three quote characters do it.
Type
select * from t where c like 'C:\and press Format Query.Undershoot, and it corrupts. The block-comment scan is
while i + 1 < count && !(chars[i] == "*" && chars[i + 1] == "/"), and thei += 2that skips theclosing
*/is correctly gated. But when the comment is unterminated the loop stops ati == count - 1and nothing advancesi, so the token is sliced short and the outer loop resumes onthe leftover character.
Measured:
/* abcproducedcomment:"/* ab"plusidentifier:"c". That last character is thenformatted as code, so it can be uppercased as a keyword or have whitespace and newlines inserted
around it. Silent corruption of the user's text by a formatting command.
The line-comment branch already gets this right with
while i < count && chars[i] != "\n", which isthe shape the other two should have had. These are one root cause fifteen lines apart in one
function, so fixing only the trap would have left the corruption shipping.
The change
Two bounded exits, following the local convention in
MongoShellFormatter.readStringLiteral:87(bound the lookahead, keep the current character):
i = min(i + 2, count)else { i = count }when the closing*/was never foundThe formatter needs no decline path. An unterminated
.stringtoken already ships and alreadyworks:
'abcwith no backslash exits cleanly today and emits one.stringholding the tail.Assembly is verbatim,
SQLFormatterService.swift:187routes.stringtoappendToken, andappendTokenonly prepends indent or a single space. Uppercasing is confined to.keyword. Sobounding the scanner is the smallest complete fix.
Blast radius
SQLTokenizerhas exactly one production caller,SQLFormatterService.swift:52, so this reachesFormat Query only. Syntax highlighting does not use it. Entry points are the query menu, the editor
view, and the editor context menu, all funnelling into
performFormatSQL.Files
TablePro/Core/Services/Formatting/SQLTokenizer.swiftTableProTests/Core/Services/SQLTokenizerTests.swiftTableProUITests/QueryFormatUnterminatedLiteralUITests.swift(new)CHANGELOG.mdVerification
generate,buildtest SQLTokenizerTestspre-fixExceeded max restart count of 2 (Underlying Error: Crash: TablePro)test7 suites after the fixuitest QueryFormatUnterminatedLiteralUITestspre-fixuitest QueryFormatUnterminatedLiteralUITestsafterswiftlinton the production fileSuites run:
SQLTokenizerTests,SQLFormatterServiceTests,FormatScopeResolverTests,KeywordUppercaseHelperTests,SQLEditorCoordinatorTests,SQLEditorCoordinatorEscapeMenuTests,SQLEditorCoordinatorCleanupTests.The pre-fix red baseline is a host crash rather than a clean assertion, because a trap cannot be
caught by
#expect,withKnownIssue, orXCTExpectFailure. That is the expected shape of theevidence here, not an environment problem.
About the UI test
It types a backtick literal, not a single quote, and that is deliberate.
BracketPairs.allValuesauto-closes'and"throughStandardOpenPairFilter, so a typed quotegets a closing partner and the document no longer ends in the backslash that trips the tokenizer. An
auto-closed
'C:\'puts the backslash at index 3 of 5,ilands exactly oncount, and nothingtraps. A first version of this test used
'and would have passed both before and after the fix.Backticks are not paired and take the same tokenizer branch, so the backtick version builds the
trapping document deterministically. Proven by running it against the pre-fix code, where it fails.
Limitations
SQLFormatterServicetrims the whole output, so trailing whitespace inside a trailing unterminatedliteral is still lost. Pre-existing and unchanged.
verbatim rather than reformatted. That is the correct conservative behaviour for text the tokenizer
cannot parse.
SQLTokenizertakes no dialect and treats\as an escape unconditionally, which is wrong for Postgres with
standard_conforming_stringson.That is queued separately against
SQLStatementScanner, which is on the execution path and needsits own review.
JsonSyntaxParser.swift:200has the same unguardedindex += 2shape but cannot trap: it failsclosed, returning
nilon exhaustion and only slicing after a validated in-bounds quote. Recordedso it is not re-raised.
independent read-only Claude adversarial review ran in its place. Reported as not cross-vendor
rather than as passed.