feat(InstanceContext): Avoid passing illegal pointers across CGo FFI … #83
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
AdamSLevy
force-pushed
the
InstanceContextDataRegistry
branch
3 times, most recently
from
4e24271 to
f9066be
Compare
|
The latest force-push has all tests passing now. |
…for user data
Previously an unsafe.Pointer to user data was passed across the CGo FFI
which resulted in a panic if the user data included any other Go
pointers. This commit adds a global ctxData registry and assigns a
unique int id to each Instance that calls SetContextData. This id is
stored in the actual C Instance Context Data that crosses the CGo FFI
and is used to look up the actual user data. A sync.RWMutex is used to
synchronize access to the ctxData map.
BREAKING CHANGE: This changes the API for Instance.SetContextData and
InstanceContext.Data to accept and return an interface{} instead of an
unsafe.Pointer.
re wasmerio#44
AdamSLevy
force-pushed
the
InstanceContextDataRegistry
branch
from
f9066be to
61347df
Compare
Hywan
added
🎉 enhancement
|
cc @losfair |
|
Would it be better to store a pointer to This would reduce some overhead and complexity. |
|
@losfair it's possible I'm misunderstanding your suggestion, but that sounds to me like it would be passing a Go pointer, which may point to something that contains other Go pointers, across the CGo FFI, which would cause a runtime panic and is what this PR is attempting to avoid. |
|
@AdamSLevy CGo checks are not applied on |
|
In that case it would be much simpler. I will test the uintptr approach and report back. |
AdamSLevy
added a commit
to AdamSLevy/go-ext-wasm
that referenced
this pull request
Nov 18, 2019
Previously the given unsafe.Pointer to user data was passed directly across the CGo FFI resulting in a panic for any reference types or types that included any reference types or other Go pointers. This commit avoids a panic by casting the unsafe.Pointer to a uintptr, which is not analyzed by the runtime. In order for compatability with the existing CGo API and bridge code, which expects (void*), a *uintptr is passed. The memory for the user provided unsafe.Pointer and the *uintptr is cached in the instance to protect it from garbage collection. The tests are expanded to ensure that reference types do not cause panics. re wasmerio#44 wasmerio#83
|
Closed in favor of #85 |
AdamSLevy
added a commit
to AdamSLevy/go-ext-wasm
that referenced
this pull request
Dec 3, 2019
Previously an uintptr to user data was passed across the CGo FFI which would become invalid if any stack frame occurred. This commit avoids this by never passing any pointers across the CGo FFI. A registry map is used instead to keep Go data on the Go side, and only an int index into the map is passed across to the C side. fix wasmerio#93 re wasmerio#44 wasmerio#85 wasmerio#83
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.
…for user data
Previously an unsafe.Pointer to user data was passed across the CGo FFI
which resulted in a panic if the user data included any other Go
pointers. This commit adds a global ctxData registry and assigns a
unique int id to each Instance that calls SetContextData. This id is
stored in the actual C Instance Context Data that crosses the CGo FFI
and is used to look up the actual user data. A sync.RWMutex is used to
synchronize access to the ctxData map.
BREAKING CHANGE: This changes the API for Instance.SetContextData and
InstanceContext.Data to accept and return an interface{} instead of an
unsafe.Pointer.
re #44
Additional details
This approach is an improvement on the approach being used in the Gossamer project's runtime package.
I believe the work around for this limitation should be built directly into this package, because while it is fairly simple, there are subtleties that are easy to miss.
At minimum, the documentation of
Instance.SetContextData()should be updated to mention this limitation. Issue #44 is an example of confusion about this limitation.