Occasional runtime/cgo: misuse of an invalid Handle panic when using vsock

[cfergeau]

Describe the bug

When using vsock to connect from the VM to the host, I sometimes get a panic:

panic: runtime/cgo: misuse of an invalid Handle

goroutine 17 [running, locked to thread]:
runtime/cgo.Handle.Value(...)
	/usr/local/go/src/runtime/cgo/handle.go:124
github.com/Code-Hex/vz/v3.shouldAcceptNewConnectionHandler(0x14000182000?, 0x0?, 0x14000000001?)
	/Users/teuf/go/pkg/mod/github.com/!code-!hex/vz/v3@v3.0.6/socket.go:231 +0x11c

To Reproduce

I'm afraid I don't have a clear reproducer.

Expected behavior
No panic
Screenshots
If applicable, add screenshots to help explain your problem.

Environment that you use to compile (please complete the following information):

• Xcode version: [xcodebuild -version | pbcopy]
• macOS Version: [sw_vers | pbcopy]

ProductName:		macOS
ProductVersion:		13.4.1
BuildVersion:		22F82

• mac architecture: [arm64]
• Go Version: [go version | pbcopy]

% go version
go version go1.20.5 darwin/arm64

Additional context
The fix might be similar to some of the recent fixes in the v3 branch (?)

[cfergeau]

I've managed to reproduce it in this branch: https://github.com/cfergeau/vz/tree/issue_131:

$ make test/run TARGET=TestRunIssue131
[...]
Run socat as vsock ssh proxyserver (port=2222)

Login with root and no password.

localhost login: [    3.053740] random: crng init done
finalizing handler 0x140000a2260 value: 0x100a3b020
panic: runtime/cgo: misuse of an invalid Handle

goroutine 17 [running, locked to thread]:
runtime/cgo.Handle.Value(...)
        /usr/local/go/src/runtime/cgo/handle.go:124
github.com/Code-Hex/vz/v3.shouldAcceptNewConnectionHandler(0x14000102000?, 0x0?, 0x14000000001?)
        /Users/teuf/dev/vz/socket.go:235 +0x11c
2023/07/07 13:49:05 exit status 2
exit status 1
FAIL    github.com/Code-Hex/vz/v3       5.220s
testing: warning: no tests to run
PASS
ok      github.com/Code-Hex/vz/v3/internal/progress     0.351s [no tests to run]
FAIL
make: *** [test/run] Error 1

[mpoindexter]

I also see this, it seems like it's timing dependent.

I think the problem here is how cgo.Handle values are passed/used between the Go side and the native side. Everywhere this project passes a cgo.Handle to the native side it's done by the pattern unsafe.Pointer(&cgoHandle). This pattern is recommended in the docs as the way to pass a cgo.Handle to native code that expects a void *, but when the native side then stores this value for use in callback it violates one of the cgo pointer use rules at https://pkg.go.dev/cmd/cgo#hdr-Passing_pointers - namely C code may not keep a copy of a Go pointer after the call returns. Violation of this rule seems to create a pointer to a memory location that can be stomped on in a timing dependent way.

I think a better way to handle this is to pass the cgo.Handle value to native code as a uintptr_t since all the methods called by the Go side are defined by this project. In a few cases this means that the uintptr_t will have to be cast to a void * on the native side, but this is OK - it's just the cast in Go to an unsafe.Pointer that is not safe, see https://groups.google.com/g/golang-nuts/c/nL2FGeIWpfQ

I've made the changes above locally, and it makes the test created by @cfergeau pass (well, actually just time out after 5 minutes of running, but no longer crash with the invalid handle error)

I can create a PR with my changes if this seems like a good change, please let me know @Code-Hex

[mpoindexter]

My changes: main...mpoindexter:vz:fix-invalid-handle-main

[Code-Hex]

@cfergeau Thanks to create this issue! and I'm sorry for the late.

Good catch @mpoindexter !!
Could you create PR? Thank you so much!

[cfergeau]

Great to see this fixed! Do you want a PR to add the test case from https://github.com/cfergeau/vz/tree/issue_131 to this repo to prevent future regressions?

[mpoindexter]

I did not add that test case to my PR since with the fix in place it runs for a very long time and makes the test suite time out with the fixed code. I could not think of a suitable thing to use as a unit test since the behavior appeared to be timing dependent.

…

On Wed, Aug 16, 2023 at 1:47 AM Christophe Fergeau ***@***.***> wrote: Great to see this fixed! Do you want a PR to add the test case from https://github.com/cfergeau/vz/tree/issue_131 to this repo to prevent future regressions? — Reply to this email directly, view it on GitHub <#131 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMNCGLRSVRBP6NOCZXU7C3XVSCITANCNFSM6AAAAAAZ4PZFVY> . You are receiving this because you were mentioned.Message ID: ***@***.***>