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: Possible stack corruption when passing pointer to a "named return variable" to deferred function #13587

Closed
prashantv opened this issue Dec 11, 2015 · 5 comments
Closed

cmd/compile: Possible stack corruption when passing pointer to a "named return variable" to deferred function #13587

prashantv opened this issue Dec 11, 2015 · 5 comments
Labels
FrozenDueToAge
Milestone
Go1.5.4

Comments

@prashantv
Copy link
Contributor

Repro code: https://github.com/prashantv/defer-repro/blob/master/main.go

(The current version is the simplest repro, but takes longer to run. Older versions crash faster but have more code)

Using named returns, and passing a pointer to the return value to another function seems to cause some sort of stack corruption sometimes. Running the code tends to crash either either:

panic: runtime error: growslice: cap out of range

goroutine 480 [running]:
fmt.(*fmt).padString(0xc824ec82c8, 0x22e0, 0xc823ee0210)
        /usr/local/Cellar/go/1.5.1/libexec/src/fmt/format.go:130 +0x406
fmt.(*fmt).fmt_s(0xc824ec82c8, 0x22e0, 0xc823ee0210)
        /usr/local/Cellar/go/1.5.1/libexec/src/fmt/format.go:322 +0x61
fmt.(*pp).fmtString(0xc824ec8270, 0x22e0, 0xc823ee0210, 0xc800000076)
        /usr/local/Cellar/go/1.5.1/libexec/src/fmt/print.go:518 +0x144
fmt.(*pp).printArg(0xc824ec8270, 0xc7ac0, 0xc824e24020, 0x76, 0x0, 0x0)
        /usr/local/Cellar/go/1.5.1/libexec/src/fmt/print.go:797 +0xd91
fmt.(*pp).doPrint(0xc824ec8270, 0xc823a77f08, 0x2, 0x2, 0xc824e20101)
        /usr/local/Cellar/go/1.5.1/libexec/src/fmt/print.go:1254 +0x258
fmt.Fprintln(0x3a91c0, 0xc82000a3c0, 0xc823a77f08, 0x2, 0x2, 0x0, 0x0, 0x0)
        /usr/local/Cellar/go/1.5.1/libexec/src/fmt/print.go:254 +0x67
main.instrument.func1()
        /Users/prashant/gocode/src/github.com/prashantv/defer-repro/main.go:22 +0x1ae
main.methodWithError(0x0, 0x0)
        /Users/prashant/gocode/src/github.com/prashantv/defer-repro/main.go:28 +0xf7
main.main.func1()
        /Users/prashant/gocode/src/github.com/prashantv/defer-repro/main.go:13 +0x18
created by main.main
        /Users/prashant/gocode/src/github.com/prashantv/defer-repro/main.go:15 +0x3a

or with

runtime:objectstart Span weird: p=0xc8246b2000 k=0x6412359 s.start=0xc8246b2000 s.limit=0xc8246b4000 s.state=2
fatal error: objectstart: bad pointer in unexpected span

goroutine 1025 [running]:
runtime.throw(0x132d20, 0x2b)
        /usr/local/Cellar/go/1.5.1/libexec/src/runtime/panic.go:527 +0x90 fp=0xc82002fe10 sp=0xc82002fdf8
runtime.heapBitsForObject(0xc8246b2000, 0x0, 0x0, 0xc800000000, 0x580988)
        /usr/local/Cellar/go/1.5.1/libexec/src/runtime/mbitmap.go:217 +0x287 fp=0xc82002fe48 sp=0xc82002fe10
runtime.scanobject(0xc824682c40, 0xc82001e720)
        /usr/local/Cellar/go/1.5.1/libexec/src/runtime/mgcmark.go:878 +0x239 fp=0xc82002ff18 sp=0xc82002fe48
runtime.gcDrainUntilPreempt(0xc82001e720, 0x7d0)
        /usr/local/Cellar/go/1.5.1/libexec/src/runtime/mgcmark.go:741 +0x152 fp=0xc82002ff50 sp=0xc82002ff18
runtime.gcBgMarkWorker(0xc82001d500)
        /usr/local/Cellar/go/1.5.1/libexec/src/runtime/mgc.go:1329 +0x474 fp=0xc82002ffb8 sp=0xc82002ff50
runtime.goexit()
        /usr/local/Cellar/go/1.5.1/libexec/src/runtime/asm_amd64.s:1696 +0x1 fp=0xc82002ffc0 sp=0xc82002ffb8
created by runtime.gcBgMarkStartWorkers
        /usr/local/Cellar/go/1.5.1/libexec/src/runtime/mgc.go:1239 +0x93

Modifying methodWithError to not return directly, but set to the return variable, stops the panic.

func methodWithError() (err error) {
    defer instrument(&err)()
    err = errors.New("error")
    return err
}

Version information:

$ go version
go version go1.5.1 darwin/amd64
$ uname -prsv
Darwin 15.2.0 Darwin Kernel Version 15.2.0: Fri Nov 13 19:56:56 PST 2015; root:xnu-3248.20.55~2/RELEASE_X86_64 i386

(Does not seem OSX specific however, we also saw this issue on Linux x64)
@ianlancetaylor ianlancetaylor changed the title Possible stack corruption when passing pointer to a "named return variable" to deferred function cmd/compile: Possible stack corruption when passing pointer to a "named return variable" to deferred function Dec 12, 2015
@ianlancetaylor ianlancetaylor added this to the Go1.6 milestone Dec 12, 2015
@ianlancetaylor
Copy link
Contributor

Preliminary guess: problem with escape analysis. Sending to @dr2chase to check.

@mdempsky
Copy link
Member

It looks like maybe it's a missing write barrier due to err escaping to the heap. Both the working and failing programs are being compiled as roughly:

func methodWithError() (err error) {
    perr := new(error)
    defer instrument(perr)()
    *perr = errors.New("error")
    err = *perr
    return
}

However, the difference is that the implicit *perr = errors.New("error") assignment only uses a write barrier for the iface.data field in the working program (i.e., with the explicit err = errors.New("error") assignment).

CC @aclements

@gopherbot
Copy link

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

@randall77 randall77 modified the milestones: Go1.5.3, Go1.6 Dec 12, 2015
@randall77
Copy link
Contributor

I think this is worthy of 1.5.3, if there is one.

@gopherbot
Copy link

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

mdempsky added a commit that referenced this issue Dec 15, 2015
@mdempsky
cmd/compile: change dead code into assert
22a204d 
After fixing #13587, I noticed that the "OAS2FUNC in disguise" block
looked like it probably needed write barriers too.  However, testing
revealed the multi-value "return f()" case was already being handled
correctly.

It turns out this block is dead code due to "return f()" already being
transformed into "t1, t2, ..., tN := f(); return t1, t2, ..., tN" by
orderstmt when f is a multi-valued function.

Updates #13587.

Change-Id: Icde46dccc55beda2ea5fd5fcafc9aae26cec1552
Reviewed-on: https://go-review.googlesource.com/17759
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
@bradfitz bradfitz modified the milestones: Go1.5.4, Go1.5.3 Jan 14, 2016
@golang golang locked and limited conversation to collaborators Jan 13, 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.5.4
Development

No branches or pull requests
7 participants
@bradfitz @mdempsky @prashantv @dr2chase @ianlancetaylor @randall77 @gopherbot