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

Comments

Projects
None yet
7 participants
@mdempsky
Copy link
Member

commented Dec 20, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Dec 20, 2018

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.

@djadala

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

why this is not release-blocker ?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

Ah, sorry, I misunderstood. Thanks.

CC @dr2chase @randall77 @josharian @cherrymui

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

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link

commented Feb 18, 2019

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 gopherbot closed this in 0349f29 Feb 21, 2019

@gopherbot

This comment has been minimized.

Copy link

commented Feb 21, 2019

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

[release-branch.go1.12] cmd/compile: flow interface data to heap if C…
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.