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.

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

x/tools/gopls: nil pointer dereference when editing a file from a cousin directory #41777

Closed
cespare opened this issue Oct 4, 2020 · 7 comments
Closed
Assignees
Labels
Milestone

Comments

@cespare
Copy link
Contributor

@cespare cespare commented Oct 4, 2020

What did you do?

I am editing code in a GOPATH project. $GOPATH/src/p is the root of a large Go source tree (all of our internal code at my company).

I can't make the crash happen with a small GOPATH; I suspect it requires a large amount of code to enlarge a race window or something like that. But I did minimize the repro a little:

$GOPATH/src/p/
├─ a/
├─ scratch/x.go
└─ ... many other deeply nested directories including many Go packages

The directory a is empty.

The file scratch/x.go contains a little bit of code (doesn't much matter what):

package scratch

func f() {
	x := "hello"
}

I am using gopls via govim. My working directory is $GOPATH/src/p/a and I edit x.go:

vim ../scratch/x.go

and immediately start making edits to the file (say, moving my cursor to the end of one of the lines and holding down backspace).

What did you expect to see?

No crashes

What did you see instead?

Govim freezes because gopls has crashed. Here is a stack trace:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x50 pc=0x7e4dba]

goroutine 6362 [running]:
golang.org/x/tools/internal/lsp/source.IsGenerated(0xef2aa0, 0xc010c9d4a0, 0x0, 0x0, 0xc00f6fd950, 0x2e, 0x0)
	/home/caleb/p/go/src/golang.org/x/tools/internal/lsp/source/util.go:90 +0x3a
golang.org/x/tools/internal/lsp.(*Server).didModifyFiles(0xc000296580, 0xef2aa0, 0xc010c9d4a0, 0xc00fcd7c20, 0x1, 0x1, 0x1, 0x0, 0x0)
	/home/caleb/p/go/src/golang.org/x/tools/internal/lsp/text_synchronization.go:244 +0xecf
golang.org/x/tools/internal/lsp.(*Server).didChange(0xc000296580, 0xef2aa0, 0xc010c9d4a0, 0xc00fcf21e0, 0x0, 0x0)
	/home/caleb/p/go/src/golang.org/x/tools/internal/lsp/text_synchronization.go:117 +0x266
golang.org/x/tools/internal/lsp.(*Server).DidChange(0xc000296580, 0xef2aa0, 0xc010c9d4a0, 0xc00fcf21e0, 0xc00fcf21e0, 0x0)
	/home/caleb/p/go/src/golang.org/x/tools/internal/lsp/server_gen.go:36 +0x49
golang.org/x/tools/internal/lsp/protocol.serverDispatch(0xef2aa0, 0xc010c9d4a0, 0xf065e0, 0xc000296580, 0xc00fcf21b0, 0x7f06743d80c0, 0xc010c9d440, 0x0, 0xedff00, 0xc002215550)
	/home/caleb/p/go/src/golang.org/x/tools/internal/lsp/protocol/tsserver.go:120 +0x1a7e
golang.org/x/tools/internal/lsp/protocol.ServerHandler.func1(0xef2aa0, 0xc010c9d4a0, 0xc00fcf21b0, 0x7f06743d80c0, 0xc010c9d440, 0x311e159c, 0x134bde0)
	/home/caleb/p/go/src/golang.org/x/tools/internal/lsp/protocol/protocol.go:63 +0xc5
golang.org/x/tools/internal/lsp/lsprpc.handshaker.func1(0xef2aa0, 0xc010c9d4a0, 0xc00fcf21b0, 0x7f06743d80c0, 0xc010c9d440, 0x0, 0x0)
	/home/caleb/p/go/src/golang.org/x/tools/internal/lsp/lsprpc/lsprpc.go:557 +0x452
golang.org/x/tools/internal/jsonrpc2.MustReplyHandler.func1(0xef2aa0, 0xc010c9d4a0, 0xc0102e3c20, 0x7f06743d80c0, 0xc010c9d440, 0xc00fd508a0, 0x1)
	/home/caleb/p/go/src/golang.org/x/tools/internal/jsonrpc2/handler.go:35 +0xcf
golang.org/x/tools/internal/jsonrpc2.AsyncHandler.func1.2(0xc0000425a0, 0xc010c9dc20, 0xc0007ca200, 0xef2aa0, 0xc010c9d4a0, 0xc0102e3c20, 0x7f06743d80c0, 0xc010c9d440)
	/home/caleb/p/go/src/golang.org/x/tools/internal/jsonrpc2/handler.go:103 +0x86
created by golang.org/x/tools/internal/jsonrpc2.AsyncHandler.func1
	/home/caleb/p/go/src/golang.org/x/tools/internal/jsonrpc2/handler.go:100 +0x173

If I edit the file from the project root ($GOPATH/src/p) or from inside scratch, the crash doesn't seem to happen. Being inside another directory and editing the file as ../scratch/x.go seems to matter.

I poked around in the code a little and I see that the map in question is indeed populated with nil snapshots a few lines earlier here so that when we get down to here we call source.IsGenerated with snapshot == nil.

I tried making the trivial change of not filling snapshotByURI with nils but that broke something else. I stared at the code for a while but it's hard for me to tell what the intent was.

It also seems like there's an unreachable condition immediately before the crash line here.

I'm guessing that for someone familiar with the code, the bug and fix will be obvious.

Build info

(I updated to latest to confirm the bug but I saw this with a variety of older gopls versions as well.)

golang.org/x/tools/gopls master
    golang.org/x/tools/gopls@(devel)
    github.com/BurntSushi/toml@v0.3.1 h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ=
    github.com/google/go-cmp@v0.5.1 h1:JFrFEBb2xKufg6XkJsJr+WbKb4FQlURi5RUcBveYu9k=
    github.com/sergi/go-diff@v1.1.0 h1:we8PVUC3FE2uYfodKH/nBHMSetSfHDR6scGdBi+erh0=
    golang.org/x/mod@v0.3.0 h1:RM4zey1++hCTbCVQfnWeKs9/IEsaBLA8vTkd0WVtmH4=
    golang.org/x/sync@v0.0.0-20200625203802-6e8e738ad208 h1:qwRHBd0NqMbJxfbotnDhm2ByMI1Shq4Y6oRJo21SGJA=
    golang.org/x/tools@v0.0.0-20200828161849-5deb26317202 => ../
    golang.org/x/xerrors@v0.0.0-20200804184101-5ec99f83aff1 h1:go1bK/D/BFZV2I8cIQd1NKEZ+0owSTG1fDTci4IqFcE=
    honnef.co/go/tools@v0.0.1-2020.1.5 h1:nI5egYTGJakVyOryqLs1cQO5dO0ksin5XXs2pspk75k=
    mvdan.cc/gofumpt@v0.0.0-20200802201014-ab5a8192947d h1:t8TAw9WgTLghti7RYkpPmqk4JtQ3+wcP5GgZqgWeWLQ=
    mvdan.cc/xurls/v2@v2.2.0 h1:NSZPykBXJFCetGZykLAxaL6SIpvbVy/UFEniIfHAa8A=

cc:

  • @stamblerre -- I think you added this nil snapshot code back in CL 215908.
  • @heschik -- Looks like you reviewed and touched most of this code.
  • @myitcv -- I found this via govim and this is similar to your "Stack trace 2" in #34366.
@gopherbot gopherbot added this to the Unreleased milestone Oct 4, 2020
@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Oct 4, 2020

Thank you for the detailed report!

/cc @pjweinb, who also saw this and was looking into fixing it

@kortschak
Copy link
Contributor

@kortschak kortschak commented Oct 4, 2020

@cespare Can you try with the changes I suggested in #41778?

@cespare
Copy link
Contributor Author

@cespare cespare commented Oct 4, 2020

@kortschak Sure. With your changes I get the same result as when I tried my fix. I get this stack trace:

panic: assignment to entry in nil map

goroutine 62 [running]:
golang.org/x/tools/internal/lsp.(*Server).didChange(0xc0000f4b00, 0xef2ac0, 0xc00040b590, 0xc000374000, 0x0, 0x0)
   /home/caleb/p/go/src/golang.org/x/tools/internal/lsp/text_synchronization.go:124 +0x2e5
golang.org/x/tools/internal/lsp.(*Server).DidChange(0xc0000f4b00, 0xef2ac0, 0xc00040b590, 0xc000374000, 0xc000374000, 0x0)
   /home/caleb/p/go/src/golang.org/x/tools/internal/lsp/server_gen.go:36 +0x49
golang.org/x/tools/internal/lsp/protocol.serverDispatch(0xef2ac0, 0xc00040b590, 0xf06600, 0xc0000f4b00, 0xc0003c7fb0, 0x7f6cac57a780, 0xc00040b4d0, 0x0, 0xedff20, 0xc000365600)
   /home/caleb/p/go/src/golang.org/x/tools/internal/lsp/protocol/tsserver.go:120 +0x1a7e
golang.org/x/tools/internal/lsp/protocol.ServerHandler.func1(0xef2ac0, 0xc00040b590, 0xc0003c7fb0, 0x7f6cac57a780, 0xc00040b4d0, 0x12087635, 0x134bde0)
   /home/caleb/p/go/src/golang.org/x/tools/internal/lsp/protocol/protocol.go:63 +0xc5
golang.org/x/tools/internal/lsp/lsprpc.handshaker.func1(0xef2ac0, 0xc00040b590, 0xc0003c7fb0, 0x7f6cac57a780, 0xc00040b4d0, 0x0, 0x0)
   /home/caleb/p/go/src/golang.org/x/tools/internal/lsp/lsprpc/lsprpc.go:557 +0x452
golang.org/x/tools/internal/jsonrpc2.MustReplyHandler.func1(0xef2ac0, 0xc00040b590, 0xc00042ca80, 0x7f6cac57a780, 0xc00040b4d0, 0xdcb791, 0x11)
   /home/caleb/p/go/src/golang.org/x/tools/internal/jsonrpc2/handler.go:35 +0xcf
golang.org/x/tools/internal/jsonrpc2.AsyncHandler.func1.2(0xc000042720, 0xc00044c1e0, 0xc0007ba200, 0xef2ac0, 0xc00040b590, 0xc00042ca80, 0x7f6cac57a780, 0xc00040b4d0)
   /home/caleb/p/go/src/golang.org/x/tools/internal/jsonrpc2/handler.go:103 +0x86
created by golang.org/x/tools/internal/jsonrpc2.AsyncHandler.func1
   /home/caleb/p/go/src/golang.org/x/tools/internal/jsonrpc2/handler.go:100 +0x173

It looks like wasFirstChanged initializes the map that's nil in the above trace. This additional tweak seems to resolve the crashes:

@@ -235,6 +236,7 @@ func (s *Server) didModifyFiles(ctx context.Context, modifications []source.File
                if mod.OnDisk || mod.Action != source.Change {
                        continue
                }
+               _ = s.wasFirstChange(mod.URI)
                snapshot, ok := snapshotByURI[mod.URI]
                if !ok {
                        continue

but I haven't read the code enough to understand what the proper fix might be.

@kortschak
Copy link
Contributor

@kortschak kortschak commented Oct 4, 2020

Yes, that's what I've come to as well. That is #41779.

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 4, 2020

Change https://golang.org/cl/259457 mentions this issue: x/tools/internal/lsp: avoid passing nil source.Snapshot to IsGenerated

@stamblerre stamblerre modified the milestones: gopls/v1.0.0, gopls/v0.5.2 Oct 5, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 5, 2020

Change https://golang.org/cl/259621 mentions this issue: internal/lsp: avoid nil pointer dereference in IsGenerated check

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 6, 2020

Change https://golang.org/cl/259877 mentions this issue: internal/lsp: improve handling of files not in views

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.