Repository navigation
Conversation
a16f730 to
9a3a120
Compare
| (bottom.as_ptr(), csi.asan_stack_size) | ||
| } | ||
|
|
||
| #[cfg_attr(asan, sanitize(address = "off"))] |
There was a problem hiding this comment.
How come these annotations are necessary? These aren't present in the craters/fiber code so I'm mostly just curious
There was a problem hiding this comment.
I have put them conservatively here. Based on my understanding the wrapper functions invoking the ASan primitives shouldn't themselves be instrumented as it can lead to false-positives or crashes under certain ASan configurations, e.g. I got it from reading this issue google/sanitizers#1760. Maybe it is being overly cautious?
| std::thread_local! { | ||
| /// The stack whose bounds ASan will report at the next matching | ||
| /// `__sanitizer_finish_switch_fiber` call. | ||
| static PENDING_SOURCE_CSI: Cell<*mut VMCommonStackInformation> = |
There was a problem hiding this comment.
I'm personally always a bit loathe to introduce more global/thread-local state -- would it be possibel to use some preexisting pointer storage for this? I'm not fully following what this is used for, but given the pointers and/or VMCommonStackInformation it naively seems like one of those can be used, but I'm also likely missing something
There was a problem hiding this comment.
The purpose of this global is to serve as a sort of mailbox for transferring bookkeeping data between stack switches. It is needed because ASan reports the old stack's bounds only after execution has moved to the destination stack. I don't think the API provides any way to transfer the necessary source metadata across that switch. I think it is important to note that this slot holds only the state during the switch handshake, i.e. its lifetime is brief and not dependent on the lifetime of the suspended continuation.
Regarding using a global: I tend to agree, but in this case I think a private global is the right thing, because it is used only in an esoteric debug build. So in a certain sense everything is nicely localised here. An alternative would be to stash it on the VMCommonStackInformation. I don't like this because it affects the continuation layout for production builds (relatedly I do plan to look into making the layout leaner).
There was a problem hiding this comment.
I will try to experiment with adding it to VMCommonStackInformation, seeing that there are two other ASan-related things there already I figured it is better to bundle them together such that I have all the bits in one place then I later get to optimise the representation.
There was a problem hiding this comment.
I've implemented the change now. Note I had to slightly reorganise the fields of VMCommonStackInformation to ensure that revision remains at an 8-aligned offset.
github-actions
Bot
added
the
wasmtime:api
This patch fixes an issue with stack-switching on ASan-enabled builds where resuming continuations from a different host-to-Wasm invocation causes a panic. Because each host invocation creates a fresh initial stack information, it would inadvertently discard the previous stack bounds on a suspended continuation, leaving it without the `asan_stack_bottom` information. The fix is to extend the ASan stack-switch handshake with both source and target `VMCommonStackInformation` pointers: 1. Before switching, the start hook receives `(source_csi, target_csi)`. 2. It supplies the target's known bounds to `__sanitizer_start_switch_fiber`. 3. It temporarily records the source CSI in a thread-local in-flight slot. 4. Immediately after switching, the finish hook obtains the previous stack’s bounds from `__sanitizer_finish_switch_fiber`. 5. It stores those bounds in the recorded source CSI and clears the in-flight slot. prtest:full
9a3a120 to
acd9edb
Compare
structure. This commit also removes the `PENDING_CSI` global, which was used to communicate bookkeeping information between ASan stack switches. This change does inflate the `VMCommonStackInformation` and by extension `VMContRef` objects. However, I think this space can subsequently be reclaimed selecting a zero-size no-op implementation of the structure in non-ASan builds (requires changing direct field accessor to be indirect via function calls). I will look into this in a future PR. prtest:full
acd9edb to
8ece790
Compare
This branch has not been deployed
2 participants
This patch fixes an issue with stack-switching on ASan-enabled builds where resuming continuations from a different host-to-Wasm invocation causes a panic. Because each host invocation creates a fresh initial stack information, it would inadvertently discard the previous stack bounds on a suspended continuation, leaving it without the
asan_stack_bottominformation.The fix is to extend the ASan stack-switch handshake with both source and target
VMCommonStackInformationpointers:(source_csi, target_csi).__sanitizer_start_switch_fiber.__sanitizer_finish_switch_fiber.Resolves #14508