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

cmd/compile: escaping closure incorrectly allocated on the stack #17318

Closed
xenoscopic opened this issue Oct 2, 2016 · 15 comments
Closed

cmd/compile: escaping closure incorrectly allocated on the stack #17318

xenoscopic opened this issue Oct 2, 2016 · 15 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Go1.7.3

Comments

@xenoscopic
Copy link

Please answer these questions before submitting your issue. Thanks!

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

go version go1.7.1 darwin/amd64

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/jacob/Projects/gocode"
GORACE=""
GOROOT="/usr/local/Cellar/go/1.7.1/libexec"
GOTOOLDIR="/usr/local/Cellar/go/1.7.1/libexec/pkg/tool/darwin_amd64"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/d6/ynnsnz1x4m9188yk1r8xns3w0000gn/T/go-build882056681=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"

Also tested with same configuration using GOARCH=386 on Darwin (issue reproducible) and on windows/386 in a virtual machine (issue NOT reproducible).

What did you do?

Attempted to pass a closure as a callback to another function that generates values to be processed and invokes the callback with these values (see demo below).

What did you expect to see?

The program should run without issue.

What did you see instead?

After an indeterminate number of invocations, the closure seems to be invalidated, and invoking it as a callback generates a segmentation fault. Alternatively, with optimizations disabled, the garbage collector indicates that there is a reference to an invalid block of memory (which to my novice reading seems to be pointing at the closure).

A package demonstrating this problem (and including stack traces) is available here:

https://github.com/havoc-io/go-closure-issue

I apologize in advance for not being able to produce a more minimal test case - I really did try. This package is already extracted from a larger program.

Instructions and more details are included in the repository, but cloning and running go test (possibly 3 or 4 times) should reproduce the error.

None of the code here uses unsafe or cgo, nor is the callback ever changed after defined. Using GOGC=off seems to stop the issue. Disabling SSA makes no difference. My suspicion is that the GC is somehow not realizing that the closure is referenced and is reclaiming it (potentially due to the complex structure of the function invoking the callback), but I am not even remotely an expert on Go's internals or GC implementation.

Perhaps @kardianos can chime in since he wrote the underlying package.
@randall77
Copy link
Contributor

(Tip to reproduce - get godoc and install it in the distribution's bin directory.)

I can reproduce with 1.7.1 but not with tip (darwin/amd64).

So maybe this has already been fixed. Not sure what fixed it; might need to do some binary search. Possibly a candidate for 1.7.2 if it turns out to be a real bug.

@randall77
Copy link
Contributor

I lied, I can reproduce at tip with GOGC=1.

@xenoscopic
Copy link
Author

(Tip to reproduce - get godoc and install it in the distribution's bin directory.)

Can you clarify what you mean? I'm not sure if you're referring to the test case or if there's some other mechanism of triggering this.

@randall77
Copy link
Contributor

Running "go test" in the go-closure-issue directory gives me a "godoc not found" error. The test is exec'ing godoc as part of the test.
Godoc is bundled with the binary distribution. If you use a source distribution, you'll need to build godoc and put in in the distribution's bin directory.

@xenoscopic
Copy link
Author

Ah, my bad! Didn't even think about that. But that makes sense. Basically the test just needs a few big files to rsync, $GOROOT/bin seemed like a portable candidate. But good to know for the future.

@quentinmit quentinmit added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 2, 2016
@quentinmit quentinmit added this to the Go1.8Early milestone Oct 2, 2016
@quentinmit quentinmit changed the title Memory corruption when using closure as callback runtime: closure GC'd too early, causing memory corruption Oct 2, 2016
@quentinmit
Copy link
Contributor

/cc @aclements @RLH

@xenoscopic
Copy link
Author

GOGC=1 go test seems to reliably reproduce the more extended stack trace indicating "fatal error: found bad pointer in Go heap (incorrect use of unsafe or cgo?)" (as opposed to the more standard segmentation fault).

@mwhudson
Copy link
Contributor

mwhudson commented Oct 2, 2016

I can reproduce this on linux/amd64 as well. It happens with tip, 1.7 and 1.6, but apparently not 1.5. In fact, it happens almost every time with 1.6 (99 / 100) so it might even be easier to start debugging with that.

@randall77
Copy link
Contributor

This looks like a heap->stack pointer.
The thing on the stack is a closure. I think it is correctly allocated on the stack. That is, its lifetime does not exceed the lifetime of the stack frame it is in.
The pointer to this closure is passed down the callstack. A subsequent function stores that closure pointer in another closure. That other closure is allocated on the heap, establishing a heap->stack pointer. That isn't allowed (except in special cases, this not being one of them). The GC then barfs when it sees that pointer.

The original stack-allocated closure is from github.com/havoc-io/go-closure-issue/server.go:50. It is passed in the call at line 59 to CreateDelta. CreateDelta (in /bitbucket.org/kardianos/rsync/rsync.go) makes the heap closure at line 247. (I don't understand why this closure is heap allocated. That's a separate issue I think.)

I suspect the bug is that we need to make sure that if a pointer is captured in a heap-allocated closure, then its referents must be treated as escaping.

@dr2chase

@dr2chase dr2chase self-assigned this Oct 3, 2016
@dr2chase
Copy link
Contributor

dr2chase commented Oct 3, 2016

Fails for me, too.

@mwhudson mwhudson mentioned this issue Oct 3, 2016
Closed
@ToadKing
Copy link

ToadKing commented Oct 3, 2016

Fails for me, go version go1.7.1 windows/amd64.

@quentinmit quentinmit changed the title runtime: closure GC'd too early, causing memory corruption cmd/compile: escaping closure incorrectly allocated on the stack Oct 5, 2016
@dr2chase
Copy link
Contributor

dr2chase commented Oct 6, 2016

I have a tiny reproducer. I think I also see the reason for the spurious escape of enqueue in rsync.go; it appears to be related to the interface conversion, which might require us to insert a "copy()" somewhere (exact somewhere TBD).

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/30693 mentions this issue.

@nightlyone
Copy link
Contributor

Seems also like a Go1.7.x candidate to me @bradfitz / @drchase

@aclements aclements modified the milestones: Go1.7.2, Go1.8Early Oct 11, 2016
@gopherbot
Copy link
Contributor

CL https://golang.org/cl/31290 mentions this issue.

gopherbot pushed a commit that referenced this issue Oct 17, 2016
@dr2chase @broady
[release-branch.go1.7] cmd/compile: escape analysis needs to run "flo…
2287d95 
…od" to fixed point

In some cases the members of the root set from which flood
runs themselves escape, without their referents being also
tagged as escaping.  Fix this by reflooding from those roots
whose escape increases, and also enhance the "leak" test to
include reachability from a heap-escaped root.

Fixes #17318.

Change-Id: Ied1e75cee17ede8ca72a8b9302ce8201641ec593
Reviewed-on: https://go-review.googlesource.com/30693
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Reviewed-on: https://go-review.googlesource.com/31290
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: David Chase <drchase@google.com>
ceseo pushed a commit to powertechpreview/go that referenced this issue Dec 1, 2016
@dr2chase @ceseo
[release-branch.go1.7] cmd/compile: escape analysis needs to run "flo…
d61c85d 
…od" to fixed point

In some cases the members of the root set from which flood
runs themselves escape, without their referents being also
tagged as escaping.  Fix this by reflooding from those roots
whose escape increases, and also enhance the "leak" test to
include reachability from a heap-escaped root.

Fixes golang#17318.

Change-Id: Ied1e75cee17ede8ca72a8b9302ce8201641ec593
Reviewed-on: https://go-review.googlesource.com/30693
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Reviewed-on: https://go-review.googlesource.com/31290
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: David Chase <drchase@google.com>
@golang golang locked and limited conversation to collaborators Oct 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Go1.7.3
Development

No branches or pull requests
9 participants
@quentinmit @mwhudson @ToadKing @xenoscopic @nightlyone @dr2chase @aclements @randall77 @gopherbot