Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

go/parser: StructType.End() can underflow the File #66683

Open
adonovan opened this issue Apr 4, 2024 · 7 comments
Open

go/parser: StructType.End() can underflow the File #66683

adonovan opened this issue Apr 4, 2024 · 7 comments
Labels
gopls/parsing Issues related to parsing / poor parser recovery. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Refactoring Issues related to refactoring tools
Milestone
Go1.24

Comments

@adonovan
Copy link
Member

adonovan commented Apr 4, 2024 •
edited
Loading

A struct type with a missing close brace has an StructType.End pos of zero. That means the enclosing nodes such as ReturnStmt may also have a computed End that is zero, or perhaps slightly larger:

Screenshot 2024-04-04 at 2 43 08 PM

I believe this is the root cause of at least some of the many gopls crashes recorded in #64547.

Ultimately this is a problem of parser error recovery. We need to establish the invariants that:

  • all tree nodes have valid Pos/End positions; and
  • all tree nodes have nested Pos/End subranges (with a possible exception for the func keyword in FuncDecl/FuncType).

See also:
@adonovan adonovan added gopls/parsing Issues related to parsing / poor parser recovery. Refactoring Issues related to refactoring tools labels Apr 4, 2024
@adonovan adonovan changed the title go/parser: StructType.End() can overflow the File go/parser: StructType.End() can underflow the File Apr 4, 2024
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 4, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/576655 mentions this issue: gopls/internal/cache: analysis: repair start/end and refine bug report

gopherbot pushed a commit to golang/tools that referenced this issue Apr 5, 2024
@adonovan
gopls/internal/cache: analysis: repair start/end and refine bug report
c7b6b8d 
Poor parser error recovery may cause Node.End to be zero, or
a small positive displacement from zero due to recursive Node.End
computation (#66683).

This change further refines the bug.Reports for such problems,
and additionally repairs the values heuristically to avoid
downstream bug.Reports after toGobDiagnostics (#64547).

Updates golang/go#66683
Updates golang/go#64547

Change-Id: I7c795622ec6b63574978d2953c82036fcc4425af
Reviewed-on: https://go-review.googlesource.com/c/tools/+/576655
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
@findleyr
Copy link
Member

findleyr commented Apr 8, 2024

Let's try to fix this for Go 1.23. Adding to the milestone.

@findleyr findleyr added this to the Go1.23 milestone Apr 8, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/577595 mentions this issue: [gopls-release-branch.0.15] gopls/internal/cache: analysis: repair start/end and refine bug report

gopherbot pushed a commit to golang/tools that referenced this issue Apr 9, 2024
@adonovan @gopherbot
[gopls-release-branch.0.15] gopls/internal/cache: analysis: repair st…
91b4992 
…art/end and refine bug report

Poor parser error recovery may cause Node.End to be zero, or
a small positive displacement from zero due to recursive Node.End
computation (#66683).

This change further refines the bug.Reports for such problems,
and additionally repairs the values heuristically to avoid
downstream bug.Reports after toGobDiagnostics (#64547).

Updates golang/go#66683
Updates golang/go#64547
Updates golang/go#66730

Change-Id: I7c795622ec6b63574978d2953c82036fcc4425af
Reviewed-on: https://go-review.googlesource.com/c/tools/+/576655
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
(cherry picked from commit c7b6b8d)
Reviewed-on: https://go-review.googlesource.com/c/tools/+/577595
Auto-Submit: Alan Donovan <adonovan@google.com>
@adonovan
Copy link
Member Author

@findleyr suggests that a fuzzer connected to an invariant checker would be a good source of regression test cases.

@findleyr findleyr mentioned this issue Apr 5, 2024
Open
@findleyr
Copy link
Member

Shall we expand this issue to introducing and enforcing our stated invariant?

@adonovan
Copy link
Member Author

Shall we expand this issue to introducing and enforcing our stated invariant?

Perhaps we need an umbrella issue. See also #66790 (comment), which is the beginning of a theory of pos/end in ill-formed trees.

@gopherbot gopherbot modified the milestones: Go1.23, Go1.24 Aug 13, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/630675 mentions this issue: gopls/internal/crash: don't crash in xrefs on out of bound nodes

gopherbot pushed a commit to golang/tools that referenced this issue Nov 21, 2024
@findleyr @gopherbot
gopls/internal/crash: don't crash in xrefs on out of bound nodes
8b6e84b 
As we've seen many times, it is currently (unfortunately) possible that
go/ast nodes exceed the bounds of the parsed file. Handle this
possibility correctly while building the xrefs index.

Updates golang/go#66683
Fixes golang/go#70446

Change-Id: If6364876eb7b8ed8ca11a058417aa028d6b55b41
Reviewed-on: https://go-review.googlesource.com/c/tools/+/630675
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
gopls/parsing Issues related to parsing / poor parser recovery. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Refactoring Issues related to refactoring tools
Projects
None yet
Milestone
Go1.24
Development

No branches or pull requests
4 participants
@dmitshur @adonovan @gopherbot @findleyr