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
Comments
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. |
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. |
why this is not release-blocker ? |
Because it is a missed optimization, not generation of invalid code. There are many missed optimizations in the compiler. They aren't release blockers. |
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. |
Ah, sorry, I misunderstood. Thanks. |
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. |
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 The reason the finalizer runs in Go 1.12 but not 1.11 is because of stack objects, where 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. |
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. |
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
Not sure this is the best way to fix. And not sure if we want this in Go 1.12. |
Change https://golang.org/cl/162829 mentions this issue: |
Change https://golang.org/cl/163203 mentions this issue: |
…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>
Running the program below prints something like
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.
The text was updated successfully, but these errors were encountered: