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

x/tools/playground: deletes compiled user binary too early, before it has finished executing and sending output #40902

Closed
owulveryck opened this issue Aug 19, 2020 · 5 comments
Closed

x/tools/playground: deletes compiled user binary too early, before it has finished executing and sending output #40902

owulveryck opened this issue Aug 19, 2020 · 5 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@owulveryck
Copy link
Contributor

owulveryck commented Aug 19, 2020 •
edited

What version of Go are you using (go version)?

$ go version
go version go1.15 darwin/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/olivier.wulveryck/Library/Caches/go-build"
GOENV="/Users/olivier.wulveryck/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/olivier.wulveryck/GOPROJECTS/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/olivier.wulveryck/GOPROJECTS"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/olivier.wulveryck/GOPROJECTS/src/golang.org/x/tools/go.mod"
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/n3/_n442h65067_9gzjq31vh06r0000gn/T/go-build691133751=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I am writing a slide to use with the latest version of present (188abfa).
I use the play capability, but when I try to run any simple code from the browser, it fails with the message:

program exited: signal: killed

What did you expect to see?

The result of the program

What did you see instead?

program exited: signal: killed

Diagnostic.

I looked into MacOS log files, and it complains with:

Unable (errno: 2) to read file at <private> for process path: <private> library path: (null)
Terminating process due to Gatekeeper rejection: 5582, <private>

I made a debug session, and it looks like present is removing the binary prematurely; therefore, MacOS is killing it.

Removing this line,
https://github.com/golang/tools/blob/cf83efe03cf89b09f33be7e464f1c95f6c3393aa/playground/socket/socket.go#L362
fixes the error.

The file produced by the execution process are cleaned here https://github.com/golang/tools/blob/cf83efe03cf89b09f33be7e464f1c95f6c3393aa/playground/socket/socket.go#L207

I can submit the fix if this is ok.

EDIT, the link to the fix in socket.go was leading to the wrong line
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Aug 19, 2020
@gopherbot gopherbot added this to the Unreleased milestone Aug 19, 2020
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 19, 2020
@dmitshur
Copy link
Contributor

Thanks for reporting.

I made a debug session, and it looks like present is removing the binary prematurely; therefore, MacOS is killing it.

Do you mind describing the fix you have in mind? What event do you think the playground should wait for before it's okay to remove the binary?

@owulveryck
Copy link
Contributor Author

I am thinking of simply removing the https://github.com/golang/tools/blob/cf83efe03cf89b09f33be7e464f1c95f6c3393aa/playground/socket/socket.go#L362

The cleanup process is actually done in the func (p *process) end(err error) which occurs after the cmd.Wait.

Removing this line should not lead to any garbage left on the filesystem.

@dmitshur
Copy link
Contributor

dmitshur commented Aug 19, 2020 •
edited

That seems reasonable. There's even a comment on a line below in that method:

p.path = path // to be removed by p.end

Thanks. I'll move this issue to NeedsFix state.

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Aug 19, 2020
@dmitshur dmitshur changed the title x/tools/playground: cannot run go code from present on MacOS x/tools/playground: deletes compiled user binary too early, before it has finished executing and sending output Aug 19, 2020
owulveryck added a commit to owulveryck/tools that referenced this issue Aug 19, 2020
@owulveryck
fix(socket): remove the os cleanup from the start method of process
f08767d 
The cleaning process is already handled by the `end` method of the `process` struct.
This premature removal causes trouble on MacOS preventing the present tool to work correctly.

This should fixe issue golang/go#40902
@owulveryck owulveryck mentioned this issue Aug 19, 2020
Closed
owulveryck added a commit to owulveryck/tools that referenced this issue Aug 21, 2020
@owulveryck
playground/socket: remove the os cleanup from start method of process
0b5f1b0 
The cleaning process happens in the end method of process, after cmd.Wait.
Deleting it early while the cmd is still running is causing issues for
security tools.

Fixes golang/go#40902
@gopherbot
Copy link

Change https://golang.org/cl/249777 mentions this issue: playground/socket: remove the os cleanup from start method of process

@dmitshur
Copy link
Contributor

dmitshur commented Sep 3, 2020 •
edited

I could reproduce the issue on macOS and confirmed that CL 249777 fixes it.

@golang golang locked and limited conversation to collaborators Sep 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 NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
3 participants
@dmitshur @gopherbot @owulveryck