lsp: prevent out-of-range panic in autoimports/completions when symbol name or position is invalid (fixes #1691)#1756
Closed
arash-mosavi wants to merge 15 commits intomicrosoft:mainfrom
Closed
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds bounds checking to prevent slice bounds out of range panics in the language server code. The changes add defensive programming measures to safely handle edge cases where positions or indices might be out of bounds when slicing strings or arrays.
- Added bounds checks before slicing source file text with position parameters
- Added validation for empty symbol names before accessing first character
- Added comprehensive bounds checking for node modules path slicing operations
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/ls/completions.go | Added bounds checks before slicing source file text in getWordLengthAndStart and getDotAccessor functions |
| internal/ls/autoimportsexportinfo.go | Added empty string check before accessing first character of symbol name |
| internal/ls/autoimports.go | Added comprehensive bounds checking for node modules path slicing operations and fixed missing closing brace |
arash-mosavi
changed the title
Author
|
@microsoft-github-policy-service agree |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
iisaduan
reviewed
Sep 30, 2025
Member
iisaduan
left a comment
iisaduan
left a comment
There was a problem hiding this comment.
The additions to autoimports look good, but I don't think the completions checks are necessary.
- Add bounds check before accessing names[0] when symbol names array may be empty - Add bounds checking in getNodeModuleRootSpecifier for component array access - Prevent panics when accessing array elements without verifying length Fixes microsoft#1691 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The exportInfoId counter was being read but never incremented, causing all export info entries to receive the same ID (1). When multiple goroutines called add() concurrently, they would all write to the same map entry (e.symbols[1]), creating a data race. This fix properly increments the counter after adding each entry, ensuring unique IDs and eliminating the race condition.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
internal/ls/autoimports.go:1326
- Extra closing brace added that doesn't match any opening brace in the getNodeModuleRootSpecifier function. This will cause a compilation error.
return ""
}
Member
|
These fixes could be plausible, but need fourslash tests to verify. |
Member
|
Superseded by #2390 |
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
4 participants
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.
Summary
Prevent a slice out-of-range panic in the LSP when computing completions/auto-imports for edge-case positions or empty symbol names.
The change adds minimal bounds checks and early returns around string/slice indexing in the language server hot paths.
Fixes #1691.
Repro (one-liner)
Typing at boundary positions (e.g., before the first char, at
len(text), or on an empty/whitespace symbol) could lead to computing invalid indices and slicing out of range, causing a panic in completions/auto-imports.Root Cause
Helpers assumed valid non-empty symbols and in-range positions, then immediately sliced the source text or accessed
s[0]. For empty names or out-of-bounds positions, indices became negative or > len, triggering a panic.What’s changed (small & localized)
internal/ls/completions.gogetWordLengthAndStart/getDotAccessorbefore slicing.start >= endor symbol is empty.internal/ls/autoimports.gointernal/ls/autoimportsexportinfo.goChanges are narrowly scoped to the points where invalid indices were observed; no refactors or logic rewrites.
Behavior & Compatibility
Tests
-1,len(text),len(text)+1) and empty/whitespace symbols.Risk
Low. Defensive bounds checks and early returns only; no broader behavior changes.
Notes for maintainers
This PR comes from a fork; could you please Approve and run the workflows so required checks can report?
All local checks pass (
go test ./..., also with-race).