golang/go

x/tools/gopls: nil deref in Snapshot.MetadataForFile

Opened this issue · 2 comments

adonovan commented 
#!stacks
"sigpanic" && "MetadataForFile:+1"

Issue created by stacks.

This stack InroIA was reported by telemetry:

// MetadataForFile returns a new slice containing metadata for each
// package containing the Go file identified by uri, ordered by the
// number of CompiledGoFiles (i.e. "narrowest" to "widest" package),
// and secondarily by IsIntermediateTestVariant (false < true).
// The result may include tests and intermediate test variants of
// importable packages.
// It returns an error if the context was cancelled.
func (s *Snapshot) MetadataForFile(ctx context.Context, uri protocol.DocumentURI) ([]*metadata.Package, error) {
	if s.view.typ == AdHocView { <------- panic

There are several implicated derefs here: Snapshot.view, View.viewDefinition, viewDefinition.typ. I suspect the first one is the problem, as I can reproduce the crash by adding this patch and running the tests:

--- a/gopls/internal/server/command.go
+++ b/gopls/internal/server/command.go
@@ -1611,6 +1611,9 @@ func showDocumentImpl(ctx context.Context, cli protocol.Client, url protocol.URI
 }
 
 func (c *commandHandler) ChangeSignature(ctx context.Context, args command.ChangeSignatureArgs) (*protocol.WorkspaceEdit, error) {
+
+       args.Location.URI = ""
+

which corresponds to a client providing a bad value in an RPC. That's a client bug (probably), but it shouldn't cause a server crash.

We should make commandHandler.run more defensive about a missing Snapshot.

golang.org/x/tools/gopls@v0.18.1 go1.23.7 darwin/arm64 neovim (2)
adonovan commented

Here's the regression test, but I'm not sure what the right fix is.

gopls/internal/test/integration/misc/failures_test.go:

func TestIssue73114(t *testing.T) {
       // Previously, a (malformed) request with an empty URI would
       // cause the server's handler to be called with a nil Snapshot
       // (#73114); panic ensued.
       Run(t, "", func(t *testing.T, env *Env) {
               args, err := command.MarshalArgs(command.ChangeSignatureArgs{})
               if err != nil {
                       t.Fatal(err)
               }
               params := &protocol.ExecuteCommandParams{
                       Command:   command.ChangeSignature.String(),
                       Arguments: args,
               }
               env.ExecuteCommand(params, nil)
       })
}

The problem is that each call to commandHandler.run needs to check for an empty URI (early) or a nil Snapshot (late), which is error-prone:

	... check for empty URI? ... 
	return c.run(ctx, commandConfig{
		forURI: args.URI,
	}, func(ctx context.Context, deps commandDeps) error {
		... check for nil deps.snapshot? ...

One possibility would be to change the type of forURI to *URI or any so that we can detect the intent to use a URI even though the actual value is empty. Another would be change commandConfig into a sequence of functional option types so that the presence of the forURI option is visible even when its value is empty.

findleyr commented

This is my bug -- I'll fix.
Have been meaning to revisit the dependency injection of commands for a while.