Stream: git-wasmtime

Topic: wasmtime / PR #7791 Update the Wasmi fuzzing oracle to ve...

Wasmtime GitHub notifications bot (Jan 18 2024 at 08:52):

Robbepop opened PR #7791 from Robbepop:rf-update-wasmi-fuzzer to bytecodealliance:main:

Currently Wasmtime uses the 2 years old Wasmi version 0.20.0.

Since then Wasmi has improved substantially and added support for new Wasm proposals such as bulk-memory, reference-types and tail-calls which we can now enable.

Besides that the most notable change are performance improvements which should make fuzzing with Wasmi a tiny bit faster.

Wasmtime GitHub notifications bot (Jan 18 2024 at 08:52):

Robbepop requested alexcrichton for a review on PR #7791.

Wasmtime GitHub notifications bot (Jan 18 2024 at 08:52):

Robbepop requested wasmtime-core-reviewers for a review on PR #7791.

Wasmtime GitHub notifications bot (Jan 18 2024 at 08:52):

Robbepop requested wasmtime-default-reviewers for a review on PR #7791.

Wasmtime GitHub notifications bot (Jan 18 2024 at 08:54):

Robbepop edited PR #7791:

Currently Wasmtime uses the 2 years old Wasmi version 0.20.0.

Since then Wasmi has improved substantially and added support for new Wasm proposals such as bulk-memory, reference-types and tail-calls which we can now enable.
Wasmi v0.31.0 has recently been audited and is used by some large projects, thus is a lot more battle tested than the previous v0.20.0.

Besides that the most notable change are performance improvements which should make fuzzing with Wasmi a tiny bit faster.

Wasmtime GitHub notifications bot (Jan 18 2024 at 08:56):

Robbepop edited PR #7791:

Currently Wasmtime uses the 2 years old Wasmi version 0.20.0.

Since then Wasmi has improved substantially and added support for new Wasm proposals such as bulk-memory, reference-types and tail-calls which we can now enable.
Wasmi v0.31.0 has recently been audited and is used by some large projects, thus is a lot more battle tested than the previous v0.20.0.

Besides that the most notable change are performance improvements which should make fuzzing with Wasmi a tiny bit faster.

Look into the future: Since roughly half a year I am working on the next major Wasmi version v0.32.0 which is a complete rewrite of the Wasmi executor featuring a much more powerful register-machine execution model. I hope that it becomes stable enough for use soon to provide it as fuzzing oracle to Wasmtime. Due to the changes we refer to the new Wasmi version as Wasmi (register) and the old Wasmi as Wasmi (stack). It might even make sense to have both versions as oracles at the same time because they have very different strengths.

Wasmtime GitHub notifications bot (Jan 18 2024 at 08:57):

Robbepop edited PR #7791:

Currently Wasmtime uses the 2 years old Wasmi version 0.20.0.

Since then Wasmi has improved substantially and added support for new Wasm proposals such as bulk-memory, reference-types and tail-calls which we can now enable.
Wasmi v0.31.0 has recently been audited and is used by some large projects, thus is a lot more battle tested than the previous v0.20.0.

Besides that the most notable change are performance improvements which should make fuzzing with Wasmi a tiny bit faster.

Look into the future: Since roughly half a year I am working on the next major Wasmi version v0.32.0 which is a complete rewrite of the Wasmi executor featuring a much more powerful register-machine execution model and optional lazy compilation & validation. I hope that it becomes stable enough for use soon to provide it as fuzzing oracle to Wasmtime. Due to the changes we refer to the new Wasmi version as Wasmi (register) and the old Wasmi as Wasmi (stack). It might even make sense to have both versions as oracles at the same time because they have very different strengths.

Wasmtime GitHub notifications bot (Jan 18 2024 at 08:58):

Robbepop commented on PR #7791:

With respect to cargo vet:

  wasmi:0.31.1 missing ["safe-to-run"]
  wasmi_arena:0.4.0 missing ["safe-to-run"]
  wasmi_core:0.13.0 missing ["safe-to-run"]
  wasmparser-nostd:0.100.1 missing ["safe-to-run"]

All those crates are maintained by Wasmi maintainers (me).

Wasmtime GitHub notifications bot (Jan 18 2024 at 10:44):

github-actions[bot] commented on PR #7791:

Subscribe to Label Action

cc @fitzgen

<details>
This issue or pull request has been labeled: "fuzzing"

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>

Wasmtime GitHub notifications bot (Jan 18 2024 at 13:57):

Robbepop updated PR #7791.

Wasmtime GitHub notifications bot (Jan 18 2024 at 14:10):

Robbepop updated PR #7791.

Wasmtime GitHub notifications bot (Jan 18 2024 at 14:12):

Robbepop commented on PR #7791:

I am a bit unsure about the new wasmi::Config handling in diff_wasmi.rs with the mutable Config parameter in WasmiEngine::new.

Wasmtime GitHub notifications bot (Jan 18 2024 at 14:12):

Robbepop edited a comment on PR #7791:

I am a bit unsure about the new wasmi::Config handling in diff_wasmi.rs with the mutable Config parameter in WasmiEngine::new. Review & feedback required.

Wasmtime GitHub notifications bot (Jan 18 2024 at 14:13):

Robbepop edited a comment on PR #7791:

I am a bit unsure about the new wasmi::Config handling in diff_wasmi.rs with the mutable Config parameter in WasmiEngine::new. Review & feedback required.
In the changes I made Wasmi now better respects the input config and adjusts its wasmi::Config to it as much as possible and sets the other config flags to false in case Wasmi does not support that feature, yet.

Wasmtime GitHub notifications bot (Jan 18 2024 at 17:03):

alexcrichton submitted PR review:

Thanks!

To avoid a new vet of wasmparser-nostd would it be possible to conditionally build wasmi with the official wasmparser crate instead? That way we could get that auto-vetted.

Wasmtime GitHub notifications bot (Jan 18 2024 at 17:03):

alexcrichton submitted PR review:

Thanks!

To avoid a new vet of wasmparser-nostd would it be possible to conditionally build wasmi with the official wasmparser crate instead? That way we could get that auto-vetted.

Wasmtime GitHub notifications bot (Jan 18 2024 at 17:03):

alexcrichton created PR review comment:

This all looks right to me, thanks! The intention here is that the differential engine (wasmi here) both configures itself based on Config but additionally updates Config with anything it can't support. So for example simd is disabled here unconditionally in Config because wasmi doesn't support it, but you're doing the right thing otherwise where features are conditionally enabled in wasmi depending on Config for those that wasmi supports.

Two minor comments here:

• This is setting threads_enabled = false twice
• The min_tables and max_tables handling can probably be removed if wasmi supports reference types which is where multiple tables were added.

If you'd like, it might be good to leave some comments here too along the lines of:

// force generated modules to never have features that wasmi doesn't support

or something like that

Wasmtime GitHub notifications bot (Jan 18 2024 at 19:02):

Robbepop updated PR #7791.

Wasmtime GitHub notifications bot (Jan 18 2024 at 19:02):

Robbepop submitted PR review.

Wasmtime GitHub notifications bot (Jan 18 2024 at 19:02):

Robbepop created PR review comment:

I have implemented your code review suggestions.

Wasmtime GitHub notifications bot (Jan 18 2024 at 19:03):

Robbepop commented on PR #7791:

Thanks!

To avoid a new vet of wasmparser-nostd would it be possible to conditionally build wasmi with the official wasmparser crate instead? That way we could get that auto-vetted.

Wasmi v0.31.0 is no longer in development so I am not going to support the standard wasmparser for it, however, I am going to think about the implications doing so for future Wasmi versions starting from v0.32.0 once released. Since Wasmi already has a std crate feature it might be simple to just use wasmparser if it is enabled and use wasmparser-nostd otherwise.

Wasmtime GitHub notifications bot (Jan 18 2024 at 19:04):

Robbepop edited a comment on PR #7791:

Thanks!

To avoid a new vet of wasmparser-nostd would it be possible to conditionally build wasmi with the official wasmparser crate instead? That way we could get that auto-vetted.

Wasmi v0.31.0 is no longer in development so I am not going to support the standard wasmparser for it, however, I am going to think about the implications doing so for future Wasmi versions starting from v0.32.0 once released. Since Wasmi already has a std crate feature it might be simple to just use wasmparser if it is enabled and use wasmparser-nostd otherwise.

From Wasmi's perspective though it would obviously the most ideal solution to simply have no_std support in the wasmparser crate.

Wasmtime GitHub notifications bot (Jan 18 2024 at 19:04):

Robbepop edited a comment on PR #7791:

Thanks!

To avoid a new vet of wasmparser-nostd would it be possible to conditionally build wasmi with the official wasmparser crate instead? That way we could get that auto-vetted.

Wasmi v0.31.0 is no longer in development so I am not going to support the standard wasmparser for it, however, I am going to think about the implications doing so for future Wasmi versions starting from v0.32.0 once released. Since Wasmi already has a std crate feature it might be simple to just use wasmparser if it is enabled and use wasmparser-nostd otherwise.

From Wasmi's perspective though it would obviously the most ideal solution to simply have no_std support in the wasmparser crate. Maintaining the wasmparser_nostd fork is okay but not perfect for synchronization.

Wasmtime GitHub notifications bot (Jan 18 2024 at 19:15):

Robbepop edited a comment on PR #7791:

Thanks!

To avoid a new vet of wasmparser-nostd would it be possible to conditionally build wasmi with the official wasmparser crate instead? That way we could get that auto-vetted.

Wasmi v0.31.0 is no longer in development so I am not going to support the standard wasmparser for it, however, I am going to think about the implications doing so for future Wasmi versions starting from v0.32.0 once released. Since Wasmi already has a std crate feature it might be simple to just use wasmparser if it is enabled and use wasmparser-nostd otherwise.

I have have a wishful thinking for no_std support in the official wasmparser crate one day. :)

Wasmtime GitHub notifications bot (Jan 19 2024 at 16:15):

Robbepop edited a comment on PR #7791:

Thanks!

To avoid a new vet of wasmparser-nostd would it be possible to conditionally build wasmi with the official wasmparser crate instead? That way we could get that auto-vetted.

Wasmi v0.31.0 is no longer in development so I am not going to support the standard wasmparser for it, however, I am going to think about the implications doing so for future Wasmi versions starting from v0.32.0 once released. Since Wasmi already has a std crate feature it might be simple to just use wasmparser if it is enabled and use wasmparser-nostd otherwise.

I have a wishful thinking for no_std support in the official wasmparser crate one day. :)

Wasmtime GitHub notifications bot (Jan 19 2024 at 16:53):

alexcrichton commented on PR #7791:

Ah ok, in that case this is going to have to hold off until one of us gets a chance to vet the dependencies here. I'm stretched a bit thin at the moment so it may be a bit before I can personally get to this (but others can of course beat me to it)

Wasmtime GitHub notifications bot (Jan 19 2024 at 17:03):

Robbepop commented on PR #7791:

Please tell me if and how I can help with vetting. :)

Wasmtime GitHub notifications bot (Jan 22 2024 at 20:28):

alexcrichton commented on PR #7791:

Reading over some code I think that this impl is not sound as it's supposed to be T: Sync. Additionally though I don't think you should need the unsafe impl at all. You can try changing the PhantomData to either fn() -> Idx or fn(Idx) I think as one of them might require the impl and one might not.

Otherwise the wasmparser-nostd diff is quite large or larger than I remember from the wasmparser diff, so diffing those directories is requiring a good deal of time to go through to verify it's the same. Additionally I haven't even gotten to wasmi yet which glancing at it is quite large and additionally contains a good deal of unsafe code to check.

Currently though wasmi is only used for fuzzing so it doesn't necessarily need a thorough review or strict vetting. Do others have thoughts on whether we should add exemptions for this dependency as opposed to vetting it?

Wasmtime GitHub notifications bot (Jan 22 2024 at 21:25):

Robbepop commented on PR #7791:

Reading over some code I think that this impl is not sound as it's supposed to be T: Sync. Additionally though I don't think you should need the unsafe impl at all. You can try changing the PhantomData to either fn() -> Idx or fn(Idx) I think as one of them might require the impl and one might not.

Thanks for catching this bug! The code is quite old and hasn't seen a lot of love lately.

Otherwise the wasmparser-nostd diff is quite large or larger than I remember from the wasmparser diff, so diffing those directories is requiring a good deal of time to go through to verify it's the same. Additionally I haven't even gotten to wasmi yet which glancing at it is quite large and additionally contains a good deal of unsafe code to check.

The unfortunate truth is that especially with the component model a lot of very no_std unfriendly abstractions such as the IndexMap has been added which required me to come up with my own no_std friendly version of it. I never intended this to be a long lasting effort but here we are.

Currently though wasmi is only used for fuzzing so it doesn't necessarily need a thorough review or strict vetting. Do others have thoughts on whether we should add exemptions for this dependency as opposed to vetting it?

To provide you with a bit of transparency here about the state of Wasmi:

• Cons:

  - I work on the project alone.
  - There are pretty much no code reviews due to above point.
  - There is a fair amount of unsafe code use, but mostly in the Wasmi bytecode executor.

• Pros:

  - There are a good deal of unit, integration, e2e and fuzz tests.
  - We do have a very extensive CI (incl. miri)
  - Wasmi v0.31.0 is already used in production by different companies some many months.
  - Wasmi v0.31.0 has received a security audit by SRLabs.
  - Wasmi 0.31.0 is much less complex than the Wasmi v0.32.0-beta.n (on master).
  

Wasmtime GitHub notifications bot (Jan 22 2024 at 21:50):

Robbepop edited a comment on PR #7791:

Reading over some code I think that this impl is not sound as it's supposed to be T: Sync. Additionally though I don't think you should need the unsafe impl at all. You can try changing the PhantomData to either fn() -> Idx or fn(Idx) I think as one of them might require the impl and one might not.

Thanks for catching this bug! The code is quite old and hasn't seen a lot of love lately.

Otherwise the wasmparser-nostd diff is quite large or larger than I remember from the wasmparser diff, so diffing those directories is requiring a good deal of time to go through to verify it's the same. Additionally I haven't even gotten to wasmi yet which glancing at it is quite large and additionally contains a good deal of unsafe code to check.

The unfortunate truth is that especially with the component model a lot of very no_std unfriendly abstractions such as the IndexMap has been added which required me to come up with my own no_std friendly version of it. I never intended this to be a long lasting effort but here we are.

Currently though wasmi is only used for fuzzing so it doesn't necessarily need a thorough review or strict vetting. Do others have thoughts on whether we should add exemptions for this dependency as opposed to vetting it?

To provide you with a bit of transparency here about the state of Wasmi:

• Cons:

  - I work on the project alone.
  - There are pretty much no code reviews due to above point.
  - There is a fair amount of unsafe code use, but mostly in the Wasmi bytecode executor.

• Pros:

  - There are a good deal of unit, integration, e2e and fuzz tests.
  - We do have a very extensive CI (incl. miri)
  - Wasmi v0.31.0 is already used in production by different companies for some months.
  - Wasmi v0.31.0 has received a security audit by SRLabs.
  - Wasmi 0.31.0 is much less complex than the Wasmi v0.32.0-beta.n (on master).
  

Wasmtime GitHub notifications bot (Jan 22 2024 at 21:52):

Robbepop edited a comment on PR #7791:

Reading over some code I think that this impl is not sound as it's supposed to be T: Sync. Additionally though I don't think you should need the unsafe impl at all. You can try changing the PhantomData to either fn() -> Idx or fn(Idx) I think as one of them might require the impl and one might not.

Thanks for catching this bug! The code is quite old and hasn't seen a lot of love lately.

Otherwise the wasmparser-nostd diff is quite large or larger than I remember from the wasmparser diff, so diffing those directories is requiring a good deal of time to go through to verify it's the same. Additionally I haven't even gotten to wasmi yet which glancing at it is quite large and additionally contains a good deal of unsafe code to check.

The unfortunate truth is that especially with the component model a lot of very no_std unfriendly abstractions such as the IndexMap has been added which required me to come up with my own no_std friendly version of it. I never intended this to be a long lasting effort but here we are.

Currently though wasmi is only used for fuzzing so it doesn't necessarily need a thorough review or strict vetting. Do others have thoughts on whether we should add exemptions for this dependency as opposed to vetting it?

To provide you with a bit of transparency here about the state of Wasmi:

• Cons:

  - I work on the project alone.
  - There are pretty much no code reviews due to above point.
  - There is a fair amount of unsafe code use, but mostly in the Wasmi bytecode executor.

• Pros:

  - There is a good deal of tests. (unit, integration, e2e, fuzz)
  - We do have a very extensive CI (incl. miri)
  - Wasmi v0.31.0 is already used in production by different companies for some months.
  - Wasmi v0.31.0 has received a security audit by SRLabs.
  - Wasmi 0.31.0 is much less complex than the Wasmi v0.32.0-beta.n (on master).
  

Wasmtime GitHub notifications bot (Jan 22 2024 at 21:53):

Robbepop edited a comment on PR #7791:

Reading over some code I think that this impl is not sound as it's supposed to be T: Sync. Additionally though I don't think you should need the unsafe impl at all. You can try changing the PhantomData to either fn() -> Idx or fn(Idx) I think as one of them might require the impl and one might not.

Thanks for catching this bug! The code is quite old and hasn't seen a lot of love lately.

Otherwise the wasmparser-nostd diff is quite large or larger than I remember from the wasmparser diff, so diffing those directories is requiring a good deal of time to go through to verify it's the same. Additionally I haven't even gotten to wasmi yet which glancing at it is quite large and additionally contains a good deal of unsafe code to check.

The unfortunate truth is that especially with the component model a lot of very no_std unfriendly abstractions such as the IndexMap has been added which required me to come up with my own no_std friendly version of it. I never intended this to be a long lasting effort but here we are. In that spirit I think it even lacks tests but wasmparser-nostd passes the wasmparser testsuite so I thought it would be good enough as temporary solution at that time.

Currently though wasmi is only used for fuzzing so it doesn't necessarily need a thorough review or strict vetting. Do others have thoughts on whether we should add exemptions for this dependency as opposed to vetting it?

To provide you with a bit of transparency here about the state of Wasmi:

• Cons:

  - I work on the project alone.
  - There are pretty much no code reviews due to above point.
  - There is a fair amount of unsafe code use, but mostly in the Wasmi bytecode executor.

• Pros:

  - There is a good deal of tests. (unit, integration, e2e, fuzz)
  - We do have a very extensive CI (incl. miri)
  - Wasmi v0.31.0 is already used in production by different companies for some months.
  - Wasmi v0.31.0 has received a security audit by SRLabs.
  - Wasmi 0.31.0 is much less complex than the Wasmi v0.32.0-beta.n (on master).
  

Wasmtime GitHub notifications bot (Jan 22 2024 at 21:54):

Robbepop edited a comment on PR #7791:

Reading over some code I think that this impl is not sound as it's supposed to be T: Sync. Additionally though I don't think you should need the unsafe impl at all. You can try changing the PhantomData to either fn() -> Idx or fn(Idx) I think as one of them might require the impl and one might not.

Thanks for catching this bug! The code is quite old and hasn't seen a lot of love lately.

Otherwise the wasmparser-nostd diff is quite large or larger than I remember from the wasmparser diff, so diffing those directories is requiring a good deal of time to go through to verify it's the same. Additionally I haven't even gotten to wasmi yet which glancing at it is quite large and additionally contains a good deal of unsafe code to check.

The unfortunate truth is that especially with the component model a lot of very no_std unfriendly abstractions such as the IndexMap has been added which required me to come up with my own no_std friendly version of it. Focus was to write it as simple as possible since I never intended this to be a long lasting effort but here we are. In that spirit I think it even lacks tests but wasmparser-nostd passes the wasmparser testsuite so I thought it would be good enough as temporary solution at that time.

Currently though wasmi is only used for fuzzing so it doesn't necessarily need a thorough review or strict vetting. Do others have thoughts on whether we should add exemptions for this dependency as opposed to vetting it?

To provide you with a bit of transparency here about the state of Wasmi:

• Cons:

  - I work on the project alone.
  - There are pretty much no code reviews due to above point.
  - There is a fair amount of unsafe code use, but mostly in the Wasmi bytecode executor.

• Pros:

  - There is a good deal of tests. (unit, integration, e2e, fuzz)
  - We do have a very extensive CI (incl. miri)
  - Wasmi v0.31.0 is already used in production by different companies for some months.
  - Wasmi v0.31.0 has received a security audit by SRLabs.
  - Wasmi 0.31.0 is much less complex than the Wasmi v0.32.0-beta.n (on master).
  

Wasmtime GitHub notifications bot (Jan 22 2024 at 22:01):

Robbepop edited a comment on PR #7791:

Reading over some code I think that this impl is not sound as it's supposed to be T: Sync. Additionally though I don't think you should need the unsafe impl at all. You can try changing the PhantomData to either fn() -> Idx or fn(Idx) I think as one of them might require the impl and one might not.

Thanks for catching this bug! The code is quite old and hasn't seen a lot of love lately.

Otherwise the wasmparser-nostd diff is quite large or larger than I remember from the wasmparser diff, so diffing those directories is requiring a good deal of time to go through to verify it's the same. Additionally I haven't even gotten to wasmi yet which glancing at it is quite large and additionally contains a good deal of unsafe code to check.

The unfortunate truth is that especially with the component model a lot of very no_std unfriendly abstractions such as the IndexMap has been added which required me to come up with my own no_std friendly version of it. Focus was to write it as simple as possible since I never intended this to be a long lasting effort but here we are. In that spirit I think it even lacks tests but wasmparser-nostd passes the wasmparser testsuite so I thought it would be good enough as temporary solution at that time.

Currently though wasmi is only used for fuzzing so it doesn't necessarily need a thorough review or strict vetting. Do others have thoughts on whether we should add exemptions for this dependency as opposed to vetting it?

To provide you with a bit of transparency here about the state of Wasmi:

• Cons:

  - I work on the project alone.
  - There are pretty much no code reviews due to above point.
  - There is a fair amount of unsafe code use, but mostly in the Wasmi bytecode executor.

• Pros:

  - There is a good deal of tests. (unit, integration, e2e, fuzz) (ignoring the indexmap-nostd crate)
  - We do have a very extensive CI (incl. miri)
  - Wasmi v0.31.0 is already used in production by different companies for some months.
  - Wasmi v0.31.0 has received a security audit by SRLabs.
  - Wasmi 0.31.0 is much less complex than the Wasmi v0.32.0-beta.n (on master).
  

Wasmtime GitHub notifications bot (Jan 22 2024 at 22:02):

Robbepop edited a comment on PR #7791:

Reading over some code I think that this impl is not sound as it's supposed to be T: Sync. Additionally though I don't think you should need the unsafe impl at all. You can try changing the PhantomData to either fn() -> Idx or fn(Idx) I think as one of them might require the impl and one might not.

Thanks for catching this bug! The code is quite old and hasn't seen a lot of love lately.

Otherwise the wasmparser-nostd diff is quite large or larger than I remember from the wasmparser diff, so diffing those directories is requiring a good deal of time to go through to verify it's the same. Additionally I haven't even gotten to wasmi yet which glancing at it is quite large and additionally contains a good deal of unsafe code to check.

The unfortunate truth is that especially with the component model a lot of very no_std unfriendly abstractions such as the IndexMap has been added which required me to come up with my own no_std friendly version of it. Focus was to write it as simple as possible since I never intended this to be a long lasting effort but here we are. In that spirit I think it even lacks tests but wasmparser-nostd passes the wasmparser testsuite so I thought it would be good enough as temporary solution at that time.

Currently though wasmi is only used for fuzzing so it doesn't necessarily need a thorough review or strict vetting. Do others have thoughts on whether we should add exemptions for this dependency as opposed to vetting it?

To provide you with a bit of transparency here about the state of Wasmi:

• Cons:

  - I work on the project alone.
  - There are pretty much no code reviews due to above point.
  - There is a fair amount of unsafe code use, but mostly in the Wasmi bytecode executor.

• Pros:

  - There is a good deal of tests. (unit, integration, e2e, fuzz) (ignoring the indexmap-nostd crate)
  - We do have a very extensive CI (incl. miri)
  - Wasmi v0.31.0 is already used in production by different companies for some months.
  - Wasmi v0.31.0 has received a security audit by SRLabs.
  - Wasmi 0.31.0 is much simpler than the Wasmi v0.32.0-beta.n (on master).
  

Wasmtime GitHub notifications bot (Jan 22 2024 at 22:04):

Robbepop edited a comment on PR #7791:

Reading over some code I think that this impl is not sound as it's supposed to be T: Sync. Additionally though I don't think you should need the unsafe impl at all. You can try changing the PhantomData to either fn() -> Idx or fn(Idx) I think as one of them might require the impl and one might not.

Thanks for catching this bug! The code is quite old and hasn't seen a lot of love lately.

Otherwise the wasmparser-nostd diff is quite large or larger than I remember from the wasmparser diff, so diffing those directories is requiring a good deal of time to go through to verify it's the same. Additionally I haven't even gotten to wasmi yet which glancing at it is quite large and additionally contains a good deal of unsafe code to check.

The unfortunate truth is that especially with the component model a lot of very no_std unfriendly abstractions such as the IndexMap has been added which required me to come up with my own no_std friendly version of it. Focus was to write it as simple as possible since I never intended this to be a long lasting effort but here we are. In that spirit I think it even lacks tests but wasmparser-nostd passes the wasmparser testsuite so I thought it would be good enough as temporary solution at that time.

Currently though wasmi is only used for fuzzing so it doesn't necessarily need a thorough review or strict vetting. Do others have thoughts on whether we should add exemptions for this dependency as opposed to vetting it?

To provide you with a bit of transparency here about the state of Wasmi:

• Cons:

  - I work on the project alone.
  - There are pretty much no code reviews due to above point.
  - There is a fair amount of unsafe code use, but mostly in the Wasmi bytecode executor.

• Pros:

  - There is a good deal of tests. (unit, integration, e2e, fuzz) (ignoring the indexmap-nostd crate)
  - We do have a very extensive CI (incl. miri)
  - Wasmi v0.31.1 is already used in production by different companies for some months.
  - Wasmi v0.31.1 has received a security audit by SRLabs.
  - Wasmi 0.31.1 is much simpler than the Wasmi v0.32.0-beta.n (on master).
  

Wasmtime GitHub notifications bot (Jan 22 2024 at 22:06):

Robbepop updated PR #7791.

Wasmtime GitHub notifications bot (Jan 22 2024 at 22:07):

Robbepop commented on PR #7791:

I just made this use Wasmi v0.31.1 instead of v0.31.0 which is more explicit. Note that v0.31.0 got yanked some time ago.

Wasmtime GitHub notifications bot (Jan 23 2024 at 11:29):

Robbepop edited a comment on PR #7791:

I just made this use Wasmi v0.31.1 instead of v0.31.0 which is more explicit. Note that v0.31.0 got yanked some time ago so even previously we already used 0.31.1.

Wasmtime GitHub notifications bot (Jan 23 2024 at 18:01):

pchickey commented on PR #7791:

Do others have thoughts on whether we should add exemptions for this dependency as opposed to vetting it?

I'm happy with adding a cargo vet exemption for wasmi 0.31.1 which notes that its only intended to be used in the context of testing. I don't think there is any real benefit to our project to have a higher bar than wasmi already meets for the sake of a fuzzing oracle.

Wasmtime GitHub notifications bot (Jan 23 2024 at 18:59):

fitzgen commented on PR #7791:

I'm happy with adding a cargo vet exemption for wasmi 0.31.1 which notes that its only intended to be used in the context of testing. I don't think there is any real benefit to our project to have a higher bar than wasmi already meets for the sake of a fuzzing oracle.

Agreed.

Wasmtime GitHub notifications bot (Jan 24 2024 at 12:37):

Robbepop commented on PR #7791:

Reading over some code I think that this impl is not sound as it's supposed to be T: Sync. Additionally though I don't think you should need the unsafe impl at all. You can try changing the PhantomData to either fn() -> Idx or fn(Idx) I think as one of them might require the impl and one might not.

btw. I just published a new wasmi_arena v0.4.1 and yanked v0.4.0 with the unsoundness fix. (https://crates.io/crates/wasmi_arena/0.4.1)

Wasmtime GitHub notifications bot (Jan 24 2024 at 12:37):

Robbepop edited a comment on PR #7791:

Reading over some code I think that this impl is not sound as it's supposed to be T: Sync. Additionally though I don't think you should need the unsafe impl at all. You can try changing the PhantomData to either fn() -> Idx or fn(Idx) I think as one of them might require the impl and one might not.

btw. I just published a new wasmi_arena v0.4.1 patch release and yanked v0.4.0.

Wasmtime GitHub notifications bot (Jan 24 2024 at 16:26):

alexcrichton commented on PR #7791:

Ok I've posted https://github.com/bytecodealliance/wasmtime/pull/7810 with new vet metadata. If you rebase this PR on top of that once it lands should be good to go

Wasmtime GitHub notifications bot (Jan 24 2024 at 18:33):

Robbepop updated PR #7791.

Wasmtime GitHub notifications bot (Jan 24 2024 at 18:38):

Robbepop commented on PR #7791:

Something went terribly wrong with the rebase ... let me investigate ...

Wasmtime GitHub notifications bot (Jan 24 2024 at 18:42):

Robbepop updated PR #7791.

Wasmtime GitHub notifications bot (Jan 24 2024 at 18:43):

Robbepop deleted a comment on PR #7791:

Something went terribly wrong with the rebase ... let me investigate ...

Wasmtime GitHub notifications bot (Jan 24 2024 at 18:46):

Robbepop updated PR #7791.

Wasmtime GitHub notifications bot (Jan 24 2024 at 19:19):

alexcrichton submitted PR review:

Thanks for the patience while we figure out the vetting, and thanks again for helping us out with the update!

Wasmtime GitHub notifications bot (Jan 24 2024 at 19:32):

Robbepop commented on PR #7791:

Thanks for the patience while we figure out the vetting, and thanks again for helping us out with the update!

Happy to help and thanks for having Wasmi as fuzzing oracle! :)

Wasmtime GitHub notifications bot (Jan 24 2024 at 20:08):

alexcrichton merged PR #7791.

Last updated: Nov 22 2024 at 16:03 UTC