lsp: prevent out-of-range panic in autoimports/completions when symbol name or position is invalid (fixes #1691) #1756
Conversation
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
|
@microsoft-github-policy-service agree |
- 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 <[email protected]>
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.
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 ""
}
|
|
||
| // Bounds check to prevent slice bounds out of range panic | ||
| fileName := moduleFile.FileName() | ||
| if topLevelPackageNameIndex+1 >= 0 && packageRootIndex >= 0 && topLevelPackageNameIndex+1 <= packageRootIndex && packageRootIndex <= len(fileName) { |
There was a problem hiding this comment.
[nitpick] The bounds checking condition is complex and hard to read. Consider extracting this logic into a helper function like isValidSliceRange(start, end, length int) bool to improve readability and maintainability.
Copilot uses AI. Check for mistakes.
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).