Fix fatal runtime panic passing invalid pointer #40
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The last argument to hs_scan is "void*", a pointer to an abitrary context. "hsScan" incorrectly uses "unsafe.Pointer(h)" when h is not a pointer. This can cause fatal runtime panic when Go passes arguments across the Go <-> C boundaries. This does not happen all the time, as it usually "looks" like a pointer. In reality, h is an unsigned long value belonging to "handles". We can coerce this into "void*" on the C side with the added "safe_hs_scan"
adonovan
reviewed
Nov 17, 2021
|
See golang/go#49633, which suggests a much simpler solution of passing the address of the cgo.Handle through the C code. |
adonovan
reviewed
Nov 18, 2021
| @@ -1013,7 +1009,8 @@ type hsMatchEventContext struct { | |||
|
|
|||
| //export hsMatchEventCallback | |||
| func hsMatchEventCallback(id C.uint, from, to C.ulonglong, flags C.uint, data unsafe.Pointer) C.int { | |||
| ctx, ok := handle.Handle(data).Value().(hsMatchEventContext) | |||
| h := (*handle.Handle)(data) | |||
There was a problem hiding this comment.
*(*handle.Handle)(data)
The current code is not wrong because of the implicit dereference in h.Value, but h should be a Handle, not a pointer to one.
|
Well, this is indeed an unthought of problem, thank you very much for your hard work :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The last argument to hs_scan is "void*", a pointer to an abitrary
context. "hsScan" incorrectly uses "unsafe.Pointer(h)" when h is not a
pointer. This can cause fatal runtime error when Go passes arguments
across the Go <-> C boundaries. The runtime error does not happen all
the time, as it usually "looks" like a pointer. This makes it difficult
to reproduce.
We can fix this by actually passing a real pointer (of our
cgo.Handle)and dereferencing the pointer within the callback. I did a quick audit,
and think I updated all places where we use the callback.
I did not audit other parts of the API spec that take in a
void*arg.Thanks @adonovan for helping with most of the analysis.
Below is a small snippet of the fatal runtime panic: