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

runtime: provide centralized facility for managing (c)go pointer handles #37033

Closed
oakad opened this issue Feb 5, 2020 · 21 comments
Closed

runtime: provide centralized facility for managing (c)go pointer handles #37033

oakad opened this issue Feb 5, 2020 · 21 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Milestone

Comments

@oakad
Copy link

oakad commented Feb 5, 2020

Any non-trivial program relying on cgo eventually will find itself in need to use callbacks and interact with go objects. The current "customary" way to deal with this situation is for package implementer to come with some sort of global map to establish correspondence between go pointers and some sort of custom cgo handle.

(I'm talking about pointers to complex go objects, which are normally disallowed by cgo call checker).

This is unfortunate, as many cgo enabled packages are required to carry boilerplate code with a lot of potential for hard to find bugs, such as:

  1. Handle assignment collisions
  2. Pointer "leaks"
  3. Concurrency issues, accessing those "pointer to handle" maps

Therefore, it will be nice if go runtime provided a small API to handle the above case cleanly, to the tune of:

// Make i non-collectable and non-movable and return a cgo handle h for it
func cgoMakeHandle(i interface{}) (h uintptr)

// Given a cgo handle, return a Go reference for the object, if valid
func cgoGetHandle(h uintptr) (i interface{}, ok bool)

// Same as "get" above, but the handle h becomes invalid, and reference i becomes collectable
// and movable again
func cgoReleaseHandle(h uintptr) (i interface{}, ok bool)

The h handle, while valid, is then supposed to be safely passable to native code and back.

@gopherbot gopherbot added this to the Proposal milestone Feb 5, 2020
@ianlancetaylor
Copy link
Contributor

We aren't going to add a general purpose facility to make an arbitrary number of pointers non-collectable and non-movable. That goes against one of the main goals of https://go.googlesource.com/proposal/+/refs/heads/master/design/12416-cgo-pointers.md, which is to have a fixed known number of such pointers at any moment in time.

I don't see anything wrong with the handle part of this proposal, but I don't see any reason it has to be in the runtime package. It can be in any package, using a map.

@oakad
Copy link
Author

oakad commented Feb 5, 2020

Even if it stays a map, it's much better to have a truly global, pointer keyed, concurrency-safe map.

Because currently it's just one big mess. Every complex cgo package comes with some creative ways to store pointers and allocate handles. This causes proliferation of global variables with unclear semantics, contributing to non-obvious side effects.

There are a great deal of "application domain specific" libraries which will be infeasible to port to Go anytime soon. On the other hand, Go had already reached a sufficient level of popularity to proliferate into places, not conceivable earlier, and link with those libraries.

Therefore, it makes all sense to standardize the pointer passing issue a bit further. For comparison, Java had a reasonably full featured JNI spec covering all aspects of native code interactions from day one, and it helped adoption a lot in the early 00' when pure Java code was not abundant yet.

@ianlancetaylor
Copy link
Contributor

I'm not objecting to the basic idea, I'm just saying that it doesn't have to be in the runtime package.

For reference, the version of this idea that I wrote for use inside Google has this API:

// H is the type of a handle to a Go value.  The underlying type may change, but
// values are guaranteed to always fit in a C.uintptr_t.
//
// The zero H is never a valid handle, and is thus safe to use as a sentinel in
// C APIs.
type H cUintptr

// New returns a handle for a Go value.  The handle is valid until the
// program calls Delete on it.  The handle uses resources, and this
// package assumes that C++ code may hold on to the handle, so the
// program must explicitly call Delete when the handle is no longer
// needed.  The intended use is to pass the returned handle to C
// code, which passes it back to Go, which calls Value.
func New(v interface{}) H

// Delete removes a handle.  This should be called when C code no
// longer has a copy of the handle, and the program no longer needs
// it.  This returns an error if the handle is not valid.
func Delete(h H) error

// MustDelete removes a handle, and calls log.Fatal if the handle is
// not valid.
func MustDelete(h H)

// Value returns the Go value for a handle.  If the handle is not
// valid, this returns an error.
func Value(h H) (interface{}, error)

// MustValue looks up a handle.  If the handle is not valid, it calls
// log.Fatal.
func MustValue(h H) interface{}

@oakad
Copy link
Author

oakad commented Feb 6, 2020

We've got a package like that as well. Just like many other people/companies. This was exactly my point - something like that should be standardized.

@ianlancetaylor
Copy link
Contributor

Understood. Again, I'm not objecting to the basic idea, I'm just saying that it doesn't have to be in the runtime package.

The next step is to pick a package and define an API.

@rsc
Copy link
Contributor

rsc commented Feb 12, 2020

And you could easily publish an package outside the standard library and see how much use it gets. It's unclear that this must be in the standard library.

@bcmills
Copy link
Contributor

bcmills commented Feb 12, 2020

it's much better to have a truly global, pointer keyed, concurrency-safe map.

Note that a single global map would likely encounter a lot of cache contention if implemented using the primitives we have in the standard library today.

(#21035 would need to be addressed in order to implement this library using sync.Map in a way that scales appropriately with CPU core count.)

@rsc
Copy link
Contributor

rsc commented Feb 26, 2020

If we were going to do this in the standard library, there is already a runtime/cgo package with no exported API, but we could add

type Handle uintptr

func NewHandle(v interface{}) Handle
func (h Handle) Delete() // panics if h already deleted or invalid
func (h Handle) Value() interface{} // panics if h already deleted or invalid

It does seem to come up often, and we could do a good implementation. Thoughts?

@rsc
Copy link
Contributor

rsc commented Mar 4, 2020

Based on the discussion above and the reactions to the last comment, this seems like a likely accept.

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Mar 4, 2020
@rsc rsc moved this from Likely Accept to Active in Proposals (old) Mar 4, 2020
@rsc rsc moved this from Active to Likely Accept in Proposals (old) Mar 4, 2020
@oakad
Copy link
Author

oakad commented Mar 5, 2020

And all the people rejoiced!

@rsc
Copy link
Contributor

rsc commented Mar 11, 2020

No change in consensus, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Mar 11, 2020
@ianlancetaylor ianlancetaylor added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Mar 11, 2020
@rsc rsc modified the milestones: Proposal, Backlog Mar 18, 2020
@rsc rsc changed the title proposal: runtime: provide centralized facility for managing (c)go pointer handles runtime: provide centralized facility for managing (c)go pointer handles Mar 18, 2020
@nightlyone
Copy link
Contributor

I like the NewHandle function. But could it return an error? How should duplicates be handled? What if we just mixed up types? Returning an error and a uintptr of 0 seems clearer here.

@ianlancetaylor
Copy link
Contributor

I don't think we have to worry about duplicates. It's not a problem to have multiple handles for a single Go value. I don't see how any error is possible here (other than running out of memory which will always cause the program to crash).

@bcmills bcmills added help wanted and removed help labels Apr 9, 2020
@gopherbot
Copy link

Change https://golang.org/cl/294670 mentions this issue: runtime/cgo: add Handle for managing (c)go pointers

@changkun
Copy link
Member

I just sent a prototype, it roughly works except it breaks non-cgo programs that request to build in external link mode. It is because the implementation will introduce other dependencies in the cgo package, whereas non-cgo programs in external link mode do not explicitly import cgo but will need to load cgo for TLS initialization and thus cause failure in the link phase.

I don't have a good sense of where should the fix be landed, any ideas or suggestions?

@gopherbot
Copy link

Change https://golang.org/cl/295369 mentions this issue: runtime/cgo: add Handle for managing (c)go pointers

@changkun
Copy link
Member

In case anyone wonders why there are two CLs. During the implementation it turns out that NewHandle can provide two different behaviors:

  1. NewHandle returns a unique handle when the provided Go value is referring to the same object (CL 294670)
  2. NewHandle always returns a different handle and a Go value can have multiple handles (CL 295369)

Either way, this seems to be a decision question.

@bcmills
Copy link
Contributor

bcmills commented Feb 23, 2021

I think if the function is named NewHandle then in needs to really return a unique handle each time. I would also be ok with a function that returns the same (reference-counted) handle every time, but it would need a different name.

That said, I suspect that it will be much easier to detect and diagnose use-after-free bugs if NewHandle returns a unique handle for each call.

@changkun
Copy link
Member

Personally, I would prefer the first behavior, as it totally makes sense that NewHandle naming does not fully match the behavior itself, thus if we choose the first, maybe could be named as CreateHandle, OpenHandle or something fits more. There are examples, such as: os.Create, os.OpenFile, etc.

@ianlancetaylor
Copy link
Contributor

I don't understand why we would want to try to provide the same handle for the same Go value. For Go types that are not comparable, that isn't well defined. And I don't see why it addresses the purpose of this issue.

If it were trivial to produce the same handle for the same Go value, that would be one thing. But I think the CLs show that it is not trivial. So I don't think we should try.

@changkun
Copy link
Member

No, don't get my comment wrong. I was just express the possibilities of implementation, not arguing one is better than the other. I can live with them both, and doesn't matter which is decided eventually :)

@rsc You approved the proposal, want to take another look?

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. Proposal Proposal-Accepted
Projects
No open projects
Development

No branches or pull requests

7 participants