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: stack growth within newobject sees junk pointer because of incorrect liveness info #16249

Closed
quentinmit opened this issue Jul 1, 2016 · 8 comments
Closed

cmd/compile: stack growth within newobject sees junk pointer because of incorrect liveness info #16249

quentinmit opened this issue Jul 1, 2016 · 8 comments
Labels
FrozenDueToAge
Milestone
Go1.7

Comments

@quentinmit
Copy link
Contributor

Go 1.7 calls runtime.newobject before initializing all return parameters; if this happens during a GC, the GC will see garbage data on the stack. @dr2chase can fill in more details here.
@quentinmit quentinmit added this to the Go1.7 milestone Jul 1, 2016
@ianlancetaylor
Copy link
Contributor

CC @randall77 @josharian

@dr2chase
Copy link
Contributor

dr2chase commented Jul 1, 2016

And we have a reproducer: https://play.golang.org/p/RiYHHWryIj

For a less-commented version of same test:

go build -gcflags -live deferr.go
# command-line-arguments
./deferr.go:8: live at entry to f: err
./deferr.go:10: live at call to writebarrierptr: err
./deferr.go:15: live at call to newobject: &err
...

That last line is wrong-wrong-wrong, because the result of newobject is assigned to &err -- i.e., it is live before it is initialized.

@dr2chase dr2chase changed the title cmd/compile: return parameter initialization racy with gc cmd/compile: stack growth within newobject sees junk pointer because of incorrect liveness info Jul 1, 2016
@dr2chase
Copy link
Contributor

dr2chase commented Jul 1, 2016

CC @dsnet

@dr2chase
Copy link
Contributor

dr2chase commented Jul 1, 2016

Note this is believed to be a result of https://go-review.googlesource.com/c/24213/ (according to @dsnet)

@ianlancetaylor
Copy link
Contributor

The description of https://golang.org/cl/24213 says it does pretty much exactly what you describe as a symptom.

@ianlancetaylor
Copy link
Contributor

@dr2chase Pretty sure this is the fix. What do you think?

--- a/src/cmd/compile/internal/gc/plive.go
+++ b/src/cmd/compile/internal/gc/plive.go
@@ -1181,6 +1181,7 @@ func livenessepilogue(lv *Liveness) {
        if hasdefer {
                for _, n := range lv.vars {
                        if n.IsOutputParamHeapAddr() {
+                               n.Name.Needzero = true
                                xoffset := n.Xoffset + stkptrsize
                                onebitwalktype1(n.Type, &xoffset, ambig)
                        }

@dsnet
Copy link
Member

dsnet commented Jul 1, 2016

@ianlancetaylor, I cherry picked your change and used it to build and run a test that was failing on this issue. After 1000 runs, they all passed. For reference, it would fail on 734/1000 of them without this change.

@gopherbot
Copy link

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

@golang golang locked and limited conversation to collaborators Jul 2, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.7
Development

No branches or pull requests
5 participants
@quentinmit @dr2chase @ianlancetaylor @dsnet @gopherbot