alexcrichton requested fitzgen for a review on PR #12688.
alexcrichton requested dicej for a review on PR #12688.
alexcrichton requested wasmtime-fuzz-reviewers for a review on PR #12688.
alexcrichton requested wasmtime-wasi-reviewers for a review on PR #12688.
alexcrichton opened PR #12688 from alexcrichton:relax-panics to bytecodealliance:main:
This commit is an admittance that I don't believe we're going to get to a point where we are confident enough in the fuzzing of component-model-async such that we could confidently say we're exercising the vast majority of possible panics. Development of component-model-async has shown a steady trickle of panics over the course of the development of the feature, and this trend has been persistent over time as well.
An attempt was made in #12119 to add a fuzzer dedicated to async events but that didn't actually find anything in development and it has missed a number of panics present before and discovered after its introduction. Overall I do not know how to improve the fuzzer to the point that it would find pretty much all of the existing async-related panics over time.
To help address this concern of the
concurrent.rsimplementation this commit goes through and replaces things likeunwrap(),assert!,panic!, andunreachable!with an error-producing form. The benefit of this is that a bug in the implementation is less likely to result in a panic and instead just results in a non-spec-compliant trap. The downside of doing this though is that it can become unclear what errors are "first class traps", or expected to be guest reachable, and which are expected to be bugs in Wasmtime. To help address this I've performed a few refactorings here as well.
Some traps previously present as error strings are now promoted to using
Trap::Fooinstead. This has some refactoring of the Rust/C side as well to make it easier to define new variants. Tests were additionally added for any trap messages that weren't previously tested as being reachable.A new
bail_bug!macro was added (internally) for Wasmtime. This is coupled with a concreteWasmtimeBugerror type (exported aswasmtime::WasmtimeBug). The intention is thatbail!continues to be "here's a string and I'm a bit too lazy to make a concrete error" whilebail_bug!indicates "this is a bug in wasmtime please report this if you see it".The rough vision is that if an error condition is reached, and the system is not broken in such a way that panicking is required, then
bail_bug!can be used to indicate a bug in Wasmtime as opposed to panicking. This reduces the real-world impact of hitting these scenarios by downgrading a CVE-worthypanic!into a bug-worthy non-spec-compliant trap. Not all panics are able to be transitioned to this as some are load bearing from a safety perspective or similar (or indicate something equally broken), but the vast majority of cases are suitable for "return a trap, lock down the store, and let destructors take care of everything else".This change additionally has resulted in API changes for
FutureReaderandStreamReader. For example creation of these types now returns aResultfor when theResourceTableis full, for example, instead of panicking.<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
alexcrichton requested wasmtime-core-reviewers for a review on PR #12688.
alexcrichton commented on PR #12688:
I'm asking for two reviews here: @fitzgen from a high level and the concept of
bail_bug!andWasmtimeBugand general thrust here (aka the PR description). @dicej if you can review the specifics of all the changes here to make sure I didn't get anything wrong, that'd also be appreciated!The only remaining major area of panics that I know of are the various public-facing buffer types within streams/futures. There's a lot of
unreachable!()andunwrap()and such through those methods as assumptions are made about the state of streams/etc. None of the methods returnResultsince the type is supposed to be a static proof of everything in a particular state. I didn't know quite how to thread that needle myself, so @dicej if you've got thoughts on that it'd be appreciated. (and example here isDirectSource)
alexcrichton updated PR #12688.
alexcrichton updated PR #12688.
github-actions[bot] added the label wasi on PR #12688.
github-actions[bot] added the label fuzzing on PR #12688.
github-actions[bot] added the label wasmtime:api on PR #12688.
github-actions[bot] added the label wasmtime:c-api on PR #12688.
github-actions[bot] commented on PR #12688:
Subscribe to Label Action
cc @fitzgen
<details>
This issue or pull request has been labeled: "fuzzing", "wasi", "wasmtime:api", "wasmtime:c-api"Thus the following users have been cc'd because of the following labels:
- fitzgen: fuzzing
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
dicej submitted PR review:
LGTM; thanks for doing this! Just a few questions/comments inline.
The only remaining major area of panics that I know of are the various public-facing buffer types within streams/futures. There's a lot of unreachable!() and unwrap() and such through those methods as assumptions are made about the state of streams/etc. None of the methods return Result since the type is supposed to be a static proof of everything in a particular state. I didn't know quite how to thread that needle myself, so @dicej if you've got thoughts on that it'd be appreciated. (and example here is DirectSource)
As a user of the API, I'd be a bit annoyed by a function which could only ever return
Err(_)due to a bug in the implementation, so I'd be inclined to leave those alone (i.e. not addResultto the return types), and I can't think of a lightweight alternative (I assumecatch_unwindis a non-starter). Maybe we could focus on fuzzing those APIs directly, e.g. without even involving Wasm, necessarily?
dicej created PR review comment:
If I'm reading this correctly, this was previously an error the guest could recover from but now it's a trap. Is that intended?
dicej created PR review comment:
What's the rationale for removing this assertion without replacing it with a trap?
dicej created PR review comment:
Not related to your changes since the original code was already using
Mutex::lock, but I'm wondering if we should usetry_lockinstead oflockhere and inwithgiven that there shouldn't be any actual contention, i.e. we could treat it like a thread-safeRefCell. Seems better to panic (or trap if preferred) if there _is_ unexpected contention than to block silently and unexpectedly.
dicej created PR review comment:
None => bail_bug!("expected task when exiting"),
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Oh this was just moved upwards a bit to avoid having extra unwraps and make it a bit easier to assert.
alexcrichton updated PR #12688.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Oops I was rejiggering this a bit too much and forgot to come back...
alexcrichton commented on PR #12688:
Agreed yeah I don't want to have a
Result-returning API that's purely for "we have a bug". More-or-less I don't think we can get to the point of confidence with fuzzing these APIs though, insofar as if we could I don't think this PR would be necessary.
alexcrichton updated PR #12688.
alexcrichton commented on PR #12688:
@dicej in https://github.com/bytecodealliance/wasmtime/pull/12688/changes/7e09f4f19a565951b57ffd83d707c11bc9edba88 I refactored things a bit to implement methods in terms of an internal
Result-returning method with a.unwrap()at the API layer, so we at least have thing structured to avoid panics where possible in the guts and preferResult-returning things. Doesn't move the needle much but it's something that stylistically seems reasonable to me.
alexcrichton commented on PR #12688:
I talked with @fitzgen a bit and he didn't have anything hard-against this approach but recommended we talk about this in an upcoming Wasmtime meeting. I've added an agenda item and in the meantime I'm going to go ahead and merge this -- happy to back it out later should we decide against this.
alexcrichton added PR #12688 Relax panics in async/futures to traps/errors to the merge queue.
alexcrichton merged PR #12688.
alexcrichton removed PR #12688 Relax panics in async/futures to traps/errors from the merge queue.
Last updated: Mar 23 2026 at 16:19 UTC