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: memory corruption from //go:notinheap interface method wrappers #46903

Open
mdempsky opened this issue Jun 24, 2021 · 12 comments
Open

cmd/compile: memory corruption from //go:notinheap interface method wrappers #46903

mdempsky opened this issue Jun 24, 2021 · 12 comments
Labels
NeedsFix
Milestone

Comments

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jun 24, 2021

If T is a //go:notinheap type, then *T is treated like uintptr rather than pointer-shaped. One consequence of this is that the interface method wrappers for T need to use **T instead of *T.

However, reflectdata.methodWrapper doesn't handle this correctly. For example, this program panics, whereas it succeeds if you remove the //go:notinheap directive: https://play.golang.org/p/p7TiaXlyJzX

Discovered while reimplementing wrapper generation for unified IR.

@mdempsky mdempsky added the NeedsFix label Jun 24, 2021
@mdempsky mdempsky added this to the Go1.18 milestone Jun 24, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Jun 24, 2021

Change https://golang.org/cl/330569 mentions this issue: [dev.typeparams] cmd/compile: generate wrappers within unified IR

gopherbot pushed a commit that referenced this issue Jun 25, 2021
This CL extends unified IR to handle creating wrapper methods. There's
relatively little about this code that's actually specific to unified
IR, but rewriting this logic allows a few benefits:

1. It decouples unified IR from reflectdata.methodWrapper, so the
latter code can evolve freely for -G=3's needs. This will also allow
the new code to evolve to unified IR's wrapper needs, which I
anticipate will operate slightly differently.

2. It provided an opportunity to revisit a lot of the code and
simplify/update it to current style. E.g., in the process, I
discovered #46903, which unified IR now gets correctly. (I have not
yet attempted to fix reflectdata.methodWrapper.)

3. It gives a convenient way for unified IR to ensure all of the
wrapper methods it needs are generated correctly.

For now, the wrapper generation is specific to non-quirks mode.

Change-Id: I5798de6b141f29e8eb6a5c563e7049627ff2868a
Reviewed-on: https://go-review.googlesource.com/c/go/+/330569
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
@mknyszek
Copy link
Contributor

@mknyszek mknyszek commented Nov 10, 2021

@mdempsky This looks like it could be fixed, but I haven't been following the unified IR work so I'm not sure if that's been enabled by default. Please close if so. Thanks!

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jan 28, 2022

The program in the initial comment now causes a compiler crash:

# command-line-arguments
esc
.   CALL tc(1) # <autogenerated>:1:0
.   .   DOTPTR main.M tc(1) # <autogenerated>:1:0
.   .   .   NAME-main..this ld(1) Class:PPARAM Offset:0 OnStack Used PTR-**A tc(1) # <autogenerated>:1:0
<autogenerated>:1: .this.M undefined (type **A has no field or method M)

This only happens when usin the go:notinheap directive, so this doesn't seem critical. The go:notinheap directive isn't even documented. So changing milestone to 1.19.

@ianlancetaylor ianlancetaylor removed this from the Go1.18 milestone Jan 28, 2022
@ianlancetaylor ianlancetaylor added this to the Go1.19 milestone Jan 28, 2022
@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented May 18, 2022

Maybe we can disallow putting notinheap types in interfaces. Do we need notinheap types in interfaces?

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented May 18, 2022

Do we need notinheap types in interfaces?

I just tested hacking unified IR to reject converting notinheap types to interface, and the only error building the standard library is this code in netpoll.go for getting the *_type for *pollDesc:

netpoll.go:     pdEface any    = (*pollDesc)(nil)
netpoll.go:     pdType  *_type = efaceOf(&pdEface)._type

It's possible end users put cgo C.foo and *C.foo types into interfaces too, though cgo types don't have methods.

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented May 18, 2022

Thanks. These two don't need methods. So maybe we can make it that notinheap type doesn't satisfy any interface methods. So (only) empty interface is fine.

@randall77
Copy link
Contributor

@randall77 randall77 commented May 19, 2022

I like the idea of forbidding methods on notinheap types.
Or just that their methods don't satisfy otherwise corresponding interface methods?

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented May 20, 2022

It probably suffices to just make it not satisfy interface methods. We can still have methods on them, and calling them directly is still fine. (We currently have methods on notinheap types, e.g. mheap).

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented May 20, 2022

I'm a bit worried about subtle desyncs between static and dynamic interface satisfiability.

How do folks feel about instead disallowing conversion of T and *T to any interface type (including empty interface), when T is notinheap and has methods?

The alternative is to implicitly treat all methods on a notinheap type as nointerface. I think that should work fine, but currently nointerface is only used for GOEXPERIMENT=fieldtrack, so it's a bit less exercised than other compiler code paths.

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented May 20, 2022

The alternative is to implicitly treat all methods on a notinheap type as nointerface

Yeah, this is what I thought. It is determined statically. The methods are not in the type descriptor's method table, so it cannot be put into an interface with method dynamically (and also statically).

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented May 20, 2022

Okay, that SGTM. I'll double check the nointerface logic.

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented May 20, 2022

It's unfortunately not as simple as implicitly marking them all as nointerface methods, because we have to worry about promoted methods too. But we only want methods promoted to a NIH type's method set to be marked nointerface, whereas currently the //go:nointerface bit is shared across the base method and all promotions.

It might be a little more robust to require all methods (including promoted methods) on //go:notinheap types to be explicitly marked as //go:nointerface. That's perhaps more tedious for the runtime team, but would be more obvious what's happening too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix
Projects
None yet
Development

No branches or pull requests

6 participants