Add an unsafe API to unload process trap handlers #9022
Merged
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
This commit adds a new unsafe API `Engine::unload_process_handlers` which can be used to de-initialize trap-handling state in a process. In general this is an unsafe operation that we have no means of making safe, hence the `unsafe`. There are particular situations where this can be done safely though and this is provided for such situations. The safety conditions rely on invariants that Wasmtime itself cannot maintain so the burden is on callers to ensure that this is invoked in a safe manner. This is inspired by discussion [on Zulip][zulip] where Wasmtime's trap handling infrastructure prevented unloading a DLL with Wasmtime in it. There's possibly other reasons that unloading DLLs are unsafe in Rust but it feels appropriate to at least provide this knob to embedders to be able to turn if they like. [zulip]: https://bytecodealliance.zulipchat.com/#narrow/stream/206238-general/topic/How.20to.20safely.20drop.20wasmtime.3F/near/453366905
lann
reviewed
Jul 26, 2024
Comment on lines
+737
to
+740
| /// * All existing threads which have used this DLL or copy of Wasmtime may | ||
| /// no longer use this copy of Wasmtime. Per-thread state is not iterated | ||
| /// and destroyed. Only future threads may use future instances of this | ||
| /// Wasmtime itself. |
There was a problem hiding this comment.
Would it make sense to explicitly poison this copy of wasmtime with a global or is that already effectively happening?
There was a problem hiding this comment.
I'm hesitant to go out of our way to do that as it might slow down other parts for this niche unsafe part, so I'd lean towards making this part of the unsafe contract rather than trying to add more guard rails.
|
Good suggestions/questions! I've tried to wordsmith and update a bit. |
github-actions
bot
added
the
wasmtime:api
fitzgen
approved these changes
Jul 27, 2024
Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
# for free
to join this conversation on GitHub.
Already have an account?
# 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.
This commit adds a new unsafe API
Engine::unload_process_handlerswhich can be used to de-initialize trap-handling state in a process. In general this is an unsafe operation that we have no means of making safe, hence theunsafe. There are particular situations where this can be done safely though and this is provided for such situations. The safety conditions rely on invariants that Wasmtime itself cannot maintain so the burden is on callers to ensure that this is invoked in a safe manner.This is inspired by discussion on Zulip where Wasmtime's trap handling infrastructure prevented unloading a DLL with Wasmtime in it. There's possibly other reasons that unloading DLLs are unsafe in Rust but it feels appropriate to at least provide this knob to embedders to be able to turn if they like.