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

Jump to bottom

x/tools/gopls: panic on filenames containing % #36999

Closed
trapgate opened this issue Feb 3, 2020 · 11 comments
Closed

x/tools/gopls: panic on filenames containing % #36999

trapgate opened this issue Feb 3, 2020 · 11 comments
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/v0.3.1

Comments

@trapgate
Copy link

trapgate commented Feb 3, 2020

What did you do?

At some point, someone added a directory to our source tree that contained '%' characters.

This causes gopls to crash (the crash seems to be a recent development, since the directory has been there for about a year.)

You can reproduce this easily, at least on a Mac. I haven't tried this on Linux but I don't see why it wouldn't crash there too. Steps:

  1. Create a directory ~/src/%crash_pls%
  2. Create a file ~/src/%crash_pls%/main.go
  3. Start your editor - I'm using VSCode configured to use gopls, so code ~/src/%crash_pls%
  4. Verify that gopls has crashed via notifications and logs.

What did you expect to see?

'%' is a valid character in file paths, so gopls should have worked.

What did you see instead?

The editor reports that gopls has crashed multiple times and won't be restarted.

Stack trace:

goroutine 50 [running]:
golang.org/x/tools/internal/span.URI.Filename(...)
/Users/geoffhickey/pkg/mod/golang.org/x/tools@v0.0.0-20200130224948-02f1738cbe39/internal/span/uri.go:28
golang.org/x/tools/internal/lsp/cache.(*view).getGoEnv(0xc00034c000, 0x1a55100, 0xc000299890, 0xc0001a6480, 0x46, 0x46, 0xc0003cce50, 0x100e194, 0xc00033a0a0, 0xa0)
/Users/geoffhickey/pkg/mod/golang.org/x/tools@v0.0.0-20200130224948-02f1738cbe39/internal/lsp/cache/view.go:730 +0x77c
golang.org/x/tools/internal/lsp/cache.(*view).setBuildInformation(0xc00034c000, 0x1a55100, 0xc000299890, 0xc0001ca270, 0x29, 0xc0001a6480, 0x46, 0x46, 0x0, 0x0, ...)
/Users/geoffhickey/pkg/mod/golang.org/x/tools@v0.0.0-20200130224948-02f1738cbe39/internal/lsp/cache/view.go:622 +0x8f
golang.org/x/tools/internal/lsp/cache.(*session).createView(0xc0000e3b80, 0x1a55100, 0xc000299890, 0xc0002de080, 0xb, 0xc0001ca270, 0x29, 0x4000000000000000, 0x10101, 0xc00016ffd8, ...)
/Users/geoffhickey/pkg/mod/golang.org/x/tools@v0.0.0-20200130224948-02f1738cbe39/internal/lsp/cache/session.go:109 +0x4c9
golang.org/x/tools/internal/lsp/cache.(*session).NewView(0xc0000e3b80, 0x1a55100, 0xc000299890, 0xc0002de080, 0xb, 0xc0001ca270, 0x29, 0x4000000000000000, 0x10101, 0xc00016ffd8, ...)
/Users/geoffhickey/pkg/mod/golang.org/x/tools@v0.0.0-20200130224948-02f1738cbe39/internal/lsp/cache/session.go:62 +0x13d
golang.org/x/tools/internal/lsp.(*Server).addView(0xc000244a80, 0x1a55100, 0xc000299890, 0xc0002de080, 0xb, 0xc0001ca270, 0x29, 0x8bee37f, 0x1f8a640, 0xc000338000, ...)
/Users/geoffhickey/pkg/mod/golang.org/x/tools@v0.0.0-20200130224948-02f1738cbe39/internal/lsp/workspace.go:42 +0x1fe
golang.org/x/tools/internal/lsp.(*Server).addFolders(0xc000244a80, 0x1a55100, 0xc000299890, 0xc00018ec80, 0x1, 0x4)
/Users/geoffhickey/pkg/mod/golang.org/x/tools@v0.0.0-20200130224948-02f1738cbe39/internal/lsp/general.go:168 +0x1bc
golang.org/x/tools/internal/lsp.(*Server).initialized(0xc000244a80, 0x1a55100, 0xc000299890, 0x1fa76d0, 0x0, 0x0)
/Users/geoffhickey/pkg/mod/golang.org/x/tools@v0.0.0-20200130224948-02f1738cbe39/internal/lsp/general.go:156 +0x263
golang.org/x/tools/internal/lsp.(*Server).Initialized(0xc000244a80, 0x1a55100, 0xc000299890, 0x1fa76d0, 0x1fa76d0, 0x0)
/Users/geoffhickey/pkg/mod/golang.org/x/tools@v0.0.0-20200130224948-02f1738cbe39/internal/lsp/server_gen.go:112 +0x49
golang.org/x/tools/internal/lsp/protocol.serverHandler.Deliver(0x1a73100, 0xc000244a80, 0x1a55100, 0xc000299890, 0xc0002b6180, 0xc000299800, 0xc00023e8c0)
/Users/geoffhickey/pkg/mod/golang.org/x/tools@v0.0.0-20200130224948-02f1738cbe39/internal/lsp/protocol/tsserver.go:110 +0x970
golang.org/x/tools/internal/jsonrpc2.(*Conn).Run.func1(0xc00003ad20, 0xc0002b6180, 0xc000296780, 0x1a55100, 0xc000299890, 0x0, 0x0, 0xc000247270)
/Users/geoffhickey/pkg/mod/golang.org/x/tools@v0.0.0-20200130224948-02f1738cbe39/internal/jsonrpc2/jsonrpc2.go:370 +0x170
created by golang.org/x/tools/internal/jsonrpc2.(*Conn).Run
/Users/geoffhickey/pkg/mod/golang.org/x/tools@v0.0.0-20200130224948-02f1738cbe39/internal/jsonrpc2/jsonrpc2.go:354 +0x877
[Error - 2:34:35 PM] Connection to server got closed. Server will not be restarted.
[Error - 2:34:35 PM] Request textDocument/documentSymbol failed.
Error: Connection got disposed.
at Object.dispose (/Users/geoffhickey/.vscode/extensions/ms-vscode.go-0.13.0/node_modules/vscode-jsonrpc/lib/main.js:876:25)
at Object.dispose (/Users/geoffhickey/.vscode/extensions/ms-vscode.go-0.13.0/node_modules/vscode-languageclient/lib/client.js:71:35)
at LanguageClient.handleConnectionClosed (/Users/geoffhickey/.vscode/extensions/ms-vscode.go-0.13.0/node_modules/vscode-languageclient/lib/client.js:2153:42)
at LanguageClient.handleConnectionClosed (/Users/geoffhickey/.vscode/extensions/ms-vscode.go-0.13.0/node_modules/vscode-languageclient/lib/main.js:151:15)
at closeHandler (/Users/geoffhickey/.vscode/extensions/ms-vscode.go-0.13.0/node_modules/vscode-languageclient/lib/client.js:2140:18)
at CallbackList.invoke (/Users/geoffhickey/.vscode/extensions/ms-vscode.go-0.13.0/node_modules/vscode-jsonrpc/lib/events.js:62:39)
at Emitter.fire (/Users/geoffhickey/.vscode/extensions/ms-vscode.go-0.13.0/node_modules/vscode-jsonrpc/lib/events.js:120:36)
at closeHandler (/Users/geoffhickey/.vscode/extensions/ms-vscode.go-0.13.0/node_modules/vscode-jsonrpc/lib/main.js:226:26)
at CallbackList.invoke (/Users/geoffhickey/.vscode/extensions/ms-vscode.go-0.13.0/node_modules/vscode-jsonrpc/lib/events.js:62:39)
at Emitter.fire (/Users/geoffhickey/.vscode/extensions/ms-vscode.go-0.13.0/node_modules/vscode-jsonrpc/lib/events.js:120:36)
at StreamMessageReader.fireClose (/Users/geoffhickey/.vscode/extensions/ms-vscode.go-0.13.0/node_modules/vscode-jsonrpc/lib/messageReader.js:111:27)
at Socket. (/Users/geoffhickey/.vscode/extensions/ms-vscode.go-0.13.0/node_modules/vscode-jsonrpc/lib/messageReader.js:151:46)
at Socket.emit (events.js:205:15)
at Pipe. (net.js:586:12)
[Error - 2:34:35 PM] Request textDocument/documentLink failed.
Error: Connection got disposed.
at Object.dispose (/Users/geoffhickey/.vscode/extensions/ms-vscode.go-0.13.0/node_modules/vscode-jsonrpc/lib/main.js:876:25)
at Object.dispose (/Users/geoffhickey/.vscode/extensions/ms-vscode.go-0.13.0/node_modules/vscode-languageclient/lib/client.js:71:35)
at LanguageClient.handleConnectionClosed (/Users/geoffhickey/.vscode/extensions/ms-vscode.go-0.13.0/node_modules/vscode-languageclient/lib/client.js:2153:42)
at LanguageClient.handleConnectionClosed (/Users/geoffhickey/.vscode/extensions/ms-vscode.go-0.13.0/node_modules/vscode-languageclient/lib/main.js:151:15)
at closeHandler (/Users/geoffhickey/.vscode/extensions/ms-vscode.go-0.13.0/node_modules/vscode-languageclient/lib/client.js:2140:18)
at CallbackList.invoke (/Users/geoffhickey/.vscode/extensions/ms-vscode.go-0.13.0/node_modules/vscode-jsonrpc/lib/events.js:62:39)
at Emitter.fire (/Users/geoffhickey/.vscode/extensions/ms-vscode.go-0.13.0/node_modules/vscode-jsonrpc/lib/events.js:120:36)
at closeHandler (/Users/geoffhickey/.vscode/extensions/ms-vscode.go-0.13.0/node_modules/vscode-jsonrpc/lib/main.js:226:26)
at CallbackList.invoke (/Users/geoffhickey/.vscode/extensions/ms-vscode.go-0.13.0/node_modules/vscode-jsonrpc/lib/events.js:62:39)
at Emitter.fire (/Users/geoffhickey/.vscode/extensions/ms-vscode.go-0.13.0/node_modules/vscode-jsonrpc/lib/events.js:120:36)
at StreamMessageReader.fireClose (/Users/geoffhickey/.vscode/extensions/ms-vscode.go-0.13.0/node_modules/vscode-jsonrpc/lib/messageReader.js:111:27)
at Socket. (/Users/geoffhickey/.vscode/extensions/ms-vscode.go-0.13.0/node_modules/vscode-jsonrpc/lib/messageReader.js:151:46)
at Socket.emit (events.js:205:15)
at Pipe. (net.js:586:12)

Build info

golang.org/x/tools/gopls v0.3.0
    golang.org/x/tools/gopls@v0.3.0 h1:l9KKK1/n6CIbfgaUvHBWAvCfOxcl1N+KSOK79OlPIao=
    github.com/BurntSushi/toml@v0.3.1 h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ=
    github.com/sergi/go-diff@v1.0.0 h1:Kpca3qRNrduNnOQeazBd0ysaKrUJiIuISHxogkT9RPQ=
    golang.org/x/mod@v0.1.1-0.20191105210325-c90efee705ee h1:WG0RUwxtNT4qqaXX3DPA8zHFNm/D9xaBpxzHt1WcA/E=
    golang.org/x/sync@v0.0.0-20190423024810-112230192c58 h1:8gQV6CLnAEikrhgkHFbMAEhagSSnXWGV915qUMm9mrU=
    golang.org/x/tools@v0.0.0-20200130224948-02f1738cbe39 h1:5ERHXLQfA0b8cHOwaOfWaaGekrA4+Ka/N74zilLnsIk=
    golang.org/x/xerrors@v0.0.0-20191011141410-1b5146add898 h1:/atklqdjdhuosWIl6AIbOeHJjicWYPqR9bpxqxYG2pA=
    honnef.co/go/tools@v0.0.1-2019.2.3 h1:3JgtbtFHMiCmsznwGVTUWbgGov+pVqnlf1dEJTNAXeM=
    mvdan.cc/xurls/v2@v2.1.0 h1:KaMb5GLhlcSX+e+qhbRJODnUUBvlw01jt4yrjFIHAuA=

Go info

go version go1.13.6 darwin/amd64

GO111MODULE="off"
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/geoffhickey/Library/Caches/go-build"
GOENV="/Users/geoffhickey/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY="*.internal.digitalocean.com,github.com/digitalocean"
GONOSUMDB="*.internal.digitalocean.com,github.com/digitalocean"
GOOS="darwin"
GOPATH="/Users/geoffhickey/do/cthulhu/docode"
GOPRIVATE="*.internal.digitalocean.com,github.com/digitalocean"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.13.6/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.13.6/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/v7/w5lfw5qs1bd9np31z9vv04h80000gn/T/go-build902245116=/tmp/go-build -gno-record-gcc-switches -fno-common"
@gopherbot gopherbot added this to the Unreleased milestone Feb 3, 2020
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Feb 3, 2020
@gopherbot
Copy link

Thank you for filing a gopls issue! Please take a look at the Troubleshooting guide, and make sure that you have provided all of the relevant information here.

@gopherbot gopherbot added the gopls Issues related to the Go language server, gopls. label Feb 3, 2020
@stamblerre stamblerre changed the title x/tools/gopls: x/tools/gopls: panic on filenames containing % Feb 3, 2020
@stamblerre stamblerre modified the milestones: Unreleased, gopls/v0.3.1 Feb 3, 2020
@stamblerre
Copy link
Contributor

Thanks for filing this issue - I am able to reproduce and will investigate.

@jayconrod: Does the go command support leading % in filenames? I believe that is invalid.

@jayconrod
Copy link
Contributor

% in file names should be okay. I had to look this up though (in module.CheckFilePath):

CheckFilePath checks that a slash-separated file path is valid. The definition of a valid file path is the same as the definition of a valid import path except that the set of allowed characters is larger: all Unicode letters, ASCII digits, the ASCII space character (U+0020), and the ASCII punctuation characters “!#$%&()+,-.=@[]^_{}~”. (The excluded punctuation characters, " * < > ? ` ' | / \ and :, have special meanings in certain shells or operating systems.)

@gopherbot
Copy link

Change https://golang.org/cl/217542 mentions this issue: internal/lsp: attempt handle %s in filepaths

@trapgate
Copy link
Author

trapgate commented Feb 3, 2020

I did a little more digging. There are some problems with the way filenames are being URL-encoded. I've attached a patch that adds a testcase for this, and some updates to make the test pass. The problems are:

  1. In span.NewURI(), the code was calling url.PathUnescape() on the path. This is where the panic was coming from. NewURI is trying to accept both filenames and URIs here, but it's calling PathUnescape before it determines which it has been passed. A filename from the system would never be URL-escaped, so this seems like a bug.

  2. There was no place where url.PathEscape() was being called on a filename to turn it into a URI. To do this correctly, I think you have to call PathEscape() on each segment of the path - passing in the whole path will escape the directory separators too, which isn't what you want. I added a loop that does this.

  3. A couple of the test cases were expecting to get back URIs with spaces in them. That's not a valid URI - spaces should be escaped as %20.

tools.txt

@trapgate
Copy link
Author

trapgate commented Feb 3, 2020

Sorry, I've committed a cretinous error and given you a backwards patch. Here's the same patch with the diff params in the right order.

I rebuilt gopls with this patch and fed it to my editor. It does fix the problem, and seems otherwise to be working as expected.

tools.txt

@gopherbot
Copy link

Change https://golang.org/cl/217599 mentions this issue: internal/lsp: handle percents by not unescaping URIs

@stamblerre
Copy link
Contributor

Thank you for investigating and for figuring out the issue. We came to a similar conclusion around the same time :) Just mailed the above CL, which should hopefully address the issue.

@gopherbot
Copy link

Change https://golang.org/cl/217639 mentions this issue: internal/span, internal/lsp: fix URI escaping

@stamblerre
Copy link
Contributor

This has been cherry-picked onto gopls-release-branch.0.3 and will be in gopls/v0.3.1.

gopherbot pushed a commit to golang/tools that referenced this issue Feb 4, 2020
@stamblerre
internal/span, internal/lsp: fix URI escaping
0eded08 
We had previously worked around a VS Code URI bug by unescaping URIs.
This is incorrect, so stop doing it and then add a specific workaround
just for that one bug.

Fixes golang/go#36999

Change-Id: I92f1a5f71749af7a6b1020eee1272586515f7084
Reviewed-on: https://go-review.googlesource.com/c/tools/+/217599
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
(cherry picked from commit 35ac94b)
Reviewed-on: https://go-review.googlesource.com/c/tools/+/217639
@trapgate
Copy link
Author

trapgate commented Feb 4, 2020

Thank you for the quick fix! :)

@stamblerre stamblerre mentioned this issue Feb 4, 2020
Closed
@golang golang locked and limited conversation to collaborators Feb 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
gopls/v0.3.1
Development

No branches or pull requests
4 participants
@trapgate @jayconrod @stamblerre @gopherbot