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

cmd/compile: escape analysis doesn't handle interface conversions correctly #29353

Closed
mdempsky opened this issue Dec 20, 2018 · 13 comments
Closed
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@mdempsky
Copy link
Member

Running the program below prints something like

allocated 0xc00001c160
finalized 0xc00001c160
accessing 0xc00001c160

This is due to esc.go not recognizing that the conversion from []*T to interface{} involves a heap allocation, so x leaks to the heap. However, esc.go instead only records a flow from x to the return parameter.

This is handled correctly by my escape analysis rewrite, which I hope to submit next release cycle. But maybe someone wants to try fixing it in esc.go for this release.

package main

import "runtime"

type T [4]int

//go:noinline
func f(x []*T) interface{} {
	return x
}

func main() {
	c := make(chan int)
	p := new(T)
	println("allocated", p)
	runtime.SetFinalizer(p, func(q *T) { println("finalized", q); c <- 0 })
	var s [10]*T
	s[0] = p
	h := f(s[:])
	runtime.GC()
	<-c
	println("accessing", h.([]*T)[0])
}
@josharian
Copy link
Contributor

I’m curious—did you find this by inspection? It might be a worthwhile exercise to set up go-fuzz to try to find other inputs for which esc.go and your rewrite disagree.

@mdempsky
Copy link
Member Author

I think I first noticed it by inspection while trying to understand esc.go, but my WIP branch is setup to run both esc.go and esc2.go and output any discrepancies, and this is probably the most common.

There are a handful of others where esc.go isn't wrong, but esc2.go is better. For example, esc2.go fixes #7714.

I think rather than fuzzing differences between esc.go and esc2.go, I'd think a better way would be to have a compiler and/or runtime debug mode where we make sure heap objects don't point to the stack, and then fuzz for triggering that.

@ALTree ALTree added this to the Go1.12 milestone Dec 20, 2018
@ALTree ALTree added NeedsFix The path to resolution is known, but the work has not been done. help wanted labels Dec 20, 2018
@ALTree ALTree modified the milestones: Go1.12, Go1.13 Dec 20, 2018
@djadala
Copy link
Contributor

djadala commented Feb 15, 2019

why this is not release-blocker ?

@ianlancetaylor
Copy link
Contributor

Because it is a missed optimization, not generation of invalid code. There are many missed optimizations in the compiler. They aren't release blockers.

@mdempsky
Copy link
Member Author

Because it is a missed optimization, not generation of invalid code.

Sorry, it seems my initial report was unclear if you got that impression. Escape analysis is resulting in invalid code here.

Test case output is meant to show that the finalizer runs on an object (that is, GC has reclaimed it), but we're still able to access it afterwards. That is, we can construct use-after-free scenarios.

@ianlancetaylor
Copy link
Contributor

Ah, sorry, I misunderstood. Thanks.

CC @dr2chase @randall77 @josharian @cherrymui

@ianlancetaylor ianlancetaylor modified the milestones: Go1.13, Go1.12 Feb 15, 2019
@ianlancetaylor
Copy link
Contributor

For those I just CC'ed, this is invalid code generated due to a bug in the escape analysis pass. It seems to be a regression since the 1.11 release.

@cherrymui
Copy link
Member

From the escape analysis' point of view, this is not a regression. It's been like that for a long time, at least since Go 1.4, that s does not escape but referenced from heap.

The reason the finalizer runs in Go 1.12 but not 1.11 is because of stack objects, where s is no longer statically live.

I can imagine it is possible to come up an example similar to this that can fail with Go 1.11.

@cherrymui
Copy link
Member

I can imagine it is possible to come up an example similar to this that can fail with Go 1.11.

https://play.golang.org/p/vinRUh4sAYI

This should print 42, but not, even with Go 1.11.

@ianlancetaylor
Copy link
Contributor

Nice example. Looks like it works through Go 1.8, and then fails in Go 1.9 going forward.

I'll mark this as a release blocker for 1.13 but if we can fix it in 1.12 that would also be nice.

@cherrymui
Copy link
Member

cherrymui commented Feb 18, 2019

I made a CL that fixes this. The effect of the CL on the standard library is as below. The generated machine code for all packages are unchanged, but the export data of some packages do change. I checked all of them and I think they all similar to the f above and the parameter should escape.

# io/ioutil
io/ioutil/ioutil.go:118
	old: leaking param: r to result ~r1 level=0
	new: leaking param: r

# image
image/image.go:943
	old: leaking param: p to result ~r0 level=1
	new: leaking param content: p

# net/url
net/url/url.go:200
	old: leaking param: s to result ~r2 level=0
	new: leaking param: s

(as a consequence)
net/url/url.go:183
	old: leaking param: s to result ~r1 level=0
	new: leaking param: s

net/url/url.go:194
	old: leaking param: s to result ~r1 level=0
	new: leaking param: s

net/url/url.go:699
	old: leaking param: u to result ~r0 level=1
	new: leaking param: u

net/url/url.go:775
	old: (*URL).String u does not escape
	new: leaking param content: u

net/url/url.go:1038
	old: leaking param: u to result ~r0 level=1
	new: leaking param: u

net/url/url.go:1099
	old: (*URL).MarshalBinary u does not escape
	new: leaking param content: u

# flag
flag/flag.go:235
	old: leaking param: s to result ~r0 level=1
	new: leaking param content: s

# go/scanner
go/scanner/errors.go:105
	old: leaking param: p to result ~r0 level=0
	new: leaking param: p

# database/sql
database/sql/sql.go:204
	old: leaking param: ns to result ~r0 level=0
	new: leaking param: ns

# go/constant
go/constant/value.go:303
	old: leaking param: re to result ~r2 level=0, leaking param: im to result ~r2 level=0
	new: leaking param: re, leaking param: im

go/constant/value.go:846
	old: leaking param: x to result ~r1 level=0
	new: leaking param: x

# encoding/xml
encoding/xml/xml.go:518
	old: leaking param: d to result ~r1 level=2
	new: leaking param content: d

encoding/xml/xml.go:122
	old: leaking param: leaking param: t to result ~r1 level=0
	new: leaking param: t

# crypto/x509
crypto/x509/verify.go:506
	old: leaking param: c to result ~r8 level=0
	new: leaking param: c

crypto/x509/verify.go:563
	old: leaking param: c to result ~r3 level=0, leaking param content: c
	new: leaking param: c

crypto/x509/verify.go:615
	old: (nothing)
	new: leaking closure reference c

crypto/x509/verify.go:996
	old: leaking param: c to result ~r1 level=0, leaking param content: c
	new: leaking param: c

# net/http
net/http/filetransport.go:30
	old: leaking param: fs to result ~r1 level=0
	new: leaking param: fs

net/http/h2_bundle.go:2684
	old: leaking param: mh to result ~r0 level=2
	new: leaking param content: mh

net/http/h2_bundle.go:7352
	old: http2checkConnHeaders req does not escape
	new: leaking param content: req

# net/http/pprof
net/http/pprof/pprof.go:221
	old: leaking param: name to result ~r1 level=0
	new: leaking param: name

# cmd/internal/bio
cmd/internal/bio/must.go:21
	old: leaking param: w to result ~r1 level=0
	new: leaking param: w

Not sure this is the best way to fix. And not sure if we want this in Go 1.12.

@gopherbot
Copy link

Change https://golang.org/cl/162829 mentions this issue: cmd/compile: flow interface data to heap if CONVIFACE of a non-direct interface escapes

@gopherbot
Copy link

Change https://golang.org/cl/163203 mentions this issue: [release-branch.go1.12] cmd/compile: flow interface data to heap if CONVIFACE of a non-direct interface escapes

gopherbot pushed a commit that referenced this issue Feb 22, 2019
…ONVIFACE of a non-direct interface escapes

Consider the following code:

func f(x []*T) interface{} {
	return x
}

It returns an interface that holds a heap copy of x (by calling
convT2I or friend), therefore x escape to heap. The current
escape analysis only recognizes that x flows to the result. This
is not sufficient, since if the result does not escape, x's
content may be stack allocated and this will result a
heap-to-stack pointer, which is bad.

Fix this by realizing that if a CONVIFACE escapes and we're
converting from a non-direct interface type, the data needs to
escape to heap.

Running "toolstash -cmp" on std & cmd, the generated machine code
are identical for all packages. However, the export data (escape
tags) differ in the following packages. It looks to me that all
are similar to the "f" above, where the parameter should escape
to heap.

io/ioutil/ioutil.go:118
	old: leaking param: r to result ~r1 level=0
	new: leaking param: r

image/image.go:943
	old: leaking param: p to result ~r0 level=1
	new: leaking param content: p

net/url/url.go:200
	old: leaking param: s to result ~r2 level=0
	new: leaking param: s

(as a consequence)
net/url/url.go:183
	old: leaking param: s to result ~r1 level=0
	new: leaking param: s

net/url/url.go:194
	old: leaking param: s to result ~r1 level=0
	new: leaking param: s

net/url/url.go:699
	old: leaking param: u to result ~r0 level=1
	new: leaking param: u

net/url/url.go:775
	old: (*URL).String u does not escape
	new: leaking param content: u

net/url/url.go:1038
	old: leaking param: u to result ~r0 level=1
	new: leaking param: u

net/url/url.go:1099
	old: (*URL).MarshalBinary u does not escape
	new: leaking param content: u

flag/flag.go:235
	old: leaking param: s to result ~r0 level=1
	new: leaking param content: s

go/scanner/errors.go:105
	old: leaking param: p to result ~r0 level=0
	new: leaking param: p

database/sql/sql.go:204
	old: leaking param: ns to result ~r0 level=0
	new: leaking param: ns

go/constant/value.go:303
	old: leaking param: re to result ~r2 level=0, leaking param: im to result ~r2 level=0
	new: leaking param: re, leaking param: im

go/constant/value.go:846
	old: leaking param: x to result ~r1 level=0
	new: leaking param: x

encoding/xml/xml.go:518
	old: leaking param: d to result ~r1 level=2
	new: leaking param content: d

encoding/xml/xml.go:122
	old: leaking param: leaking param: t to result ~r1 level=0
	new: leaking param: t

crypto/x509/verify.go:506
	old: leaking param: c to result ~r8 level=0
	new: leaking param: c

crypto/x509/verify.go:563
	old: leaking param: c to result ~r3 level=0, leaking param content: c
	new: leaking param: c

crypto/x509/verify.go:615
	old: (nothing)
	new: leaking closure reference c

crypto/x509/verify.go:996
	old: leaking param: c to result ~r1 level=0, leaking param content: c
	new: leaking param: c

net/http/filetransport.go:30
	old: leaking param: fs to result ~r1 level=0
	new: leaking param: fs

net/http/h2_bundle.go:2684
	old: leaking param: mh to result ~r0 level=2
	new: leaking param content: mh

net/http/h2_bundle.go:7352
	old: http2checkConnHeaders req does not escape
	new: leaking param content: req

net/http/pprof/pprof.go:221
	old: leaking param: name to result ~r1 level=0
	new: leaking param: name

cmd/internal/bio/must.go:21
	old: leaking param: w to result ~r1 level=0
	new: leaking param: w

Fixes #29353.

Change-Id: I7e7798ae773728028b0dcae5bccb3ada51189c68
Reviewed-on: https://go-review.googlesource.com/c/162829
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: David Chase <drchase@google.com>
(cherry picked from commit 0349f29)
Reviewed-on: https://go-review.googlesource.com/c/163203
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators Feb 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

7 participants