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

cgo: Taking a reference to a cgo Handle, as documented, appears to return bad handles. #71566

Closed
stroiman opened this issue Feb 5, 2025 · 6 comments
Labels
BugReport Issues describing a possible bug in the Go implementation. Documentation Issues describing a change to documentation.

Comments

@stroiman
Copy link

stroiman commented Feb 5, 2025

Go version

go version go1.23.6 darwin/arm64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/peter/Library/Caches/go-build'
GOENV='/Users/peter/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/peter/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/peter/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/opt/homebrew/Cellar/go/1.23.6/libexec'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/opt/homebrew/Cellar/go/1.23.6/libexec/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.23.6'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/Users/peter/Library/Application Support/go/telemetry'
GCCGO='gccgo'
GOARM64='v8.0'
AR='ar'
CC='cc'
CXX='c++'
CGO_ENABLED='1'
GOMOD='/Users/peter/src/harmony/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/ww/5ffgmr_s2kq5pfqbgf_9tf800000gq/T/go-build1440665527=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

Using v8go, I store a reference to a Go object as an "external" in an internal field, a mechanism in V8 intended to store pointers to objects in the host. The API takes a C void* as argument.

The v8go version is my own fork, using the following branch: https://github.com/stroiman/v8go/tree/go-dom-support

The cgo package documentation suggests that you can take a reference to a cgo.Handle.

It is not safe to coerce a cgo.Handle (an integer) to a Go unsafe.Pointer, but instead we can pass the address of the cgo.Handle to the void* parameter, as in this variant of the previous example:

//export MyGoPrint
func MyGoPrint(context unsafe.Pointer) {
	h := *(*cgo.Handle)(context)
	val := h.Value().(string)
	println(val)
	h.Delete()
}

func main() {
	val := "hello Go"
	h := cgo.NewHandle(val)
	C.myprint(unsafe.Pointer(&h))
	// Output: hello Go
}

So to store a reference to a Go object as a V8 external, I

func NewValueExternal(iso *Isolate, val unsafe.Pointer) *Value {
	return &Value{
		ptr: C.NewValueExternal(iso.ptr, val),
	}
}

func NewValueExternalHandle(iso *Isolate, val cgo.Handle) *Value {
	return &Value{
		ptr: C.NewValueExternal(iso.ptr, unsafe.Pointer(&val)),
	}
}

I read the handle again using:

func (v *Value) External() unsafe.Pointer {
	if !v.IsExternal() {
		return nil
	}
	return C.ValueToExternal(v.ptr)
}

func (v *Value) ExternalHandle() cgo.Handle {
	unsafePtr := v.External()
	if unsafePtr == nil {
		return 0
	}
	return *(*cgo.Handle)(unsafePtr)
}

What did you see happen?

I get erratic panics

panic: runtime/cgo: misuse of an invalid Handle

After a lot of trial and error, I logged

  • The cgo.Handle after conversion
  • The void* value retrieved from the external value

During a single test, the same value is read many times from the same object, and I get the following output

 -------- XMLHttpRequest HANDLE: 31 - 0x1400047c698
Prototype: XMLHttpRequest
By us: true - <nil>
 -------- XMLHttpRequest HANDLE: 31 - 0x1400047c698
Prototype: XMLHttpRequest
By us: true - <nil>
 -------- XMLHttpRequest HANDLE: 31 - 0x1400047c698
Prototype: XMLHttpRequest
By us: true - <nil>
 -------- XMLHttpRequest HANDLE: 31 - 0x1400047c698
Prototype: XMLHttpRequest
By us: true - <nil>
 -------- XMLHttpRequest HANDLE: 31 - 0x1400047c698
Prototype: XMLHttpRequest
By us: true - <nil>
 -------- XMLHttpRequest HANDLE: 31 - 0x1400047c698
Prototype: XMLHttpRequest
By us: true - <nil>
 -------- XMLHttpRequest HANDLE: 3985729651616 - 0x1400047c698

So I get the same void* from the v8 external, but suddenly, it translates to a different value. I assume that this is because GC has kicked in?

I can trigger an even harder crash by calling runtime.GC() before reading the handle.

What did you expect to see?

I expected that the same void*/unsafe.Pointer should consistently be converted to the same cgo.Handle when using the documented approach.

I tried to create a minimal example in the playground, create a handle, convert to unsafe ptr, GC, and convert back, but that resulted in build errors in the playground.

I will be happy to try to reproduce a more minimal example, but it will not be this week.

As a workaround, I'll try to treat the handles as uint32 values, which is also possible to store in a v8 object.

@gopherbot gopherbot added the Documentation Issues describing a change to documentation. label Feb 5, 2025
@stroiman
Copy link
Author

stroiman commented Feb 5, 2025

Maybe the system works exactly as intended, but it's just the documentation that shouldn't suggest taking a reference to a handle?

@gabyhelp gabyhelp added the BugReport Issues describing a possible bug in the Go implementation. label Feb 5, 2025
@seankhliao
Copy link
Member

I don't think we can judge what's wrong without seeing a complete example, though the error suggests you're not properly pinning memory.

given the reported panic, I think this is a question on how to pass memory, rather than a bug report.

Unlike many projects, the Go project does not use GitHub Issues for general discussion or asking questions. GitHub Issues are used for tracking bugs and proposals only.

For questions please refer to https://github.com/golang/go/wiki/Questions

@seankhliao seankhliao closed this as not planned Won't fix, can't repro, duplicate, stale Feb 5, 2025
@ianlancetaylor
Copy link
Member

The example code at https://pkg.go.dev/runtime/cgo#Handle looks somewhat misleading. The approach works fine provided the C code does not hold on to the pointer. It's impossible to tell for sure without a complete example, but I suspect that your C code want to hold on to the pointer.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/647095 mentions this issue: runtime/cgo: clarify that C code must not retain pointer

@stroiman
Copy link
Author

stroiman commented Feb 6, 2025

The example code at pkg.go.dev/runtime/cgo#Handle looks somewhat misleading.

Thanks, that's what I suspected, and yes. I do hold on to the pointer. I can see the updated comment clarifies the case :)

gopherbot pushed a commit that referenced this issue Feb 6, 2025
For #71566

Change-Id: I6dc365dd799d7b506b4a55895f1736d3dfd4684b
Reviewed-on: https://go-review.googlesource.com/c/go/+/647095
Reviewed-by: Alan Donovan <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
Auto-Submit: Ian Lance Taylor <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Commit-Queue: Ian Lance Taylor <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BugReport Issues describing a possible bug in the Go implementation. Documentation Issues describing a change to documentation.
Projects
None yet
Development

No branches or pull requests

5 participants