Stream: git-wasmtime

Topic: wasmtime / PR #8872 Upstream wasm-wave instances in wasmtime

Wasmtime GitHub notifications bot (Jun 25 2024 at 17:51):

pchickey opened PR #8872 from bytecodealliance:pch/upstream_wave to bytecodealliance:main:

<!--
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.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Jun 25 2024 at 17:58):

pchickey updated PR #8872.

Wasmtime GitHub notifications bot (Jun 25 2024 at 18:37):

pchickey updated PR #8872.

Wasmtime GitHub notifications bot (Jun 26 2024 at 21:05):

pchickey edited PR #8872:

This PR is upstreaming instances of the wasm-wave crate's traits for wasmtime values. However, we won't be able to land this PR in wasmtime until wasm-wave is written entirely in safe rust.

This PR is blocked on the cargo-vet: I hadn't looked into how logos works prior to making this PR, but I have discovered that logos derive macro generates significant amounts of unsafe rust. While I don't have any evidence logos is unsound, but it seems too risky to me to accept it into our audits, since I also have no reasonable way of ruling out that logos is unsound. In the particular case of wasm-wave, we should expect that strings accepted by the crate come from untrusted sources, so we want to be very rigorous on how they are parsed into values.

The particular bits of Lann's root wasm-wave repo this PR is upstreaming are found in https://github.com/lann/wasm-wave/tree/main/src/wasmtime. I was able to eliminate the struct FuncType definition in that code by defining component::Func::ty 5918a4b tso that theComponentFunc impl of WasmType is usable.

<!--
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.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Jun 26 2024 at 21:10):

pchickey edited PR #8872:

This PR is upstreaming instances of the wasm-wave crate's traits for wasmtime values. However, we won't be able to land this PR in wasmtime until wasm-wave is written entirely in safe rust, which it presently is not, because it uses logos to derive its lexer here: https://github.com/bytecodealliance/wasm-tools/blob/main/crates/wasm-wave/src/lex.rs#L11

This PR is blocked on the cargo-vet: I hadn't looked into how logos works prior to making this PR, but I have discovered that logos derive macro generates significant amounts of unsafe rust. While I don't have any evidence logos is unsound, but it seems too risky to me to accept it into our audits, since I also have no reasonable way of ruling out that logos is unsound. In the particular case of wasm-wave, we should expect that strings accepted by the crate come from untrusted sources, so we want to be very rigorous on how they are parsed into values.

The particular bits of Lann's root wasm-wave repo this PR is upstreaming are found in https://github.com/lann/wasm-wave/tree/main/src/wasmtime. I was able to eliminate the struct FuncType definition in that code by defining component::Func::ty 5918a4b tso that theComponentFunc impl of WasmType is usable.

<!--
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.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Jun 26 2024 at 21:12):

pchickey edited PR #8872:

This PR is upstreaming instances of the wasm-wave crate's traits for wasmtime values. However, we won't be able to land this PR in wasmtime until wasm-wave is written entirely in safe rust, which it presently is not, because it uses logos to derive its lexer here: https://github.com/bytecodealliance/wasm-tools/blob/main/crates/wasm-wave/src/lex.rs#L11

I hadn't looked into how logos works prior to making this PR, but when I got to the cargo vet for the logos crate, I discovered that logos derive macro generates significant amounts of unsafe rust. While I don't have any evidence logos is unsound, but it seems too risky to me to accept it into our audits, since I also have no reasonable way of ruling out that logos is unsound. In the particular case of wasm-wave, we should expect that strings accepted by the crate come from untrusted sources, so we want to be very rigorous on how they are parsed into values.

The particular bits of Lann's root wasm-wave repo this PR is upstreaming are found in https://github.com/lann/wasm-wave/tree/main/src/wasmtime. I was able to eliminate the struct FuncType definition in that code by defining component::Func::ty 5918a4b tso that theComponentFunc impl of WasmType is usable.

<!--
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.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Oct 19 2024 at 22:08):

Kmeakin commented on PR #8872:

I'd be interested in contributing to this, but I'll have to check with my employer first since it is under a separate repo.

In the meantime, you can use the forbid-unsafe feature from Logos: https://logos.maciej.codes/unsafe.html

Wasmtime GitHub notifications bot (Oct 21 2024 at 04:05):

pchickey commented on PR #8872:

Thanks! Yes, the plan is to merge this by enabling forbid-unsafe logos over in wasm tools, however I just haven't taken the time to get back to this because it fell off my radar. I don't think we should write a new lexer.

Wasmtime GitHub notifications bot (Oct 21 2024 at 18:59):

pchickey commented on PR #8872:

Once https://github.com/bytecodealliance/wasm-tools/pull/1874 lands and is in a wasm-tools release, I'll update this PR to use it.

Wasmtime GitHub notifications bot (Nov 12 2024 at 23:08):

pchickey updated PR #8872.

Wasmtime GitHub notifications bot (Nov 12 2024 at 23:28):

pchickey updated PR #8872.

Wasmtime GitHub notifications bot (Nov 12 2024 at 23:28):

pchickey commented on PR #8872:

This PR is now a single commit on top of #9601

Wasmtime GitHub notifications bot (Nov 13 2024 at 20:45):

pchickey updated PR #8872.

Wasmtime GitHub notifications bot (Nov 13 2024 at 20:46):

pchickey edited a comment on PR #8872:

~This PR is now a single commit on top of #9601 ~

Wasmtime GitHub notifications bot (Nov 13 2024 at 20:46):

pchickey edited a comment on PR #8872:

This PR is now a single commit on top of #9601 update and now, 9601 has landed, so its a single commit on main

Wasmtime GitHub notifications bot (Nov 13 2024 at 21:00):

pchickey updated PR #8872.

Wasmtime GitHub notifications bot (Nov 13 2024 at 21:01):

pchickey commented on PR #8872:

@alexcrichton Can you please double-check my safe-to-deploy audit of https://diff.rs/beef/0.5.2/0.5.2 as part of reviewing this PR?

Wasmtime GitHub notifications bot (Nov 13 2024 at 21:01):

pchickey requested alexcrichton for a review on PR #8872.

Wasmtime GitHub notifications bot (Nov 13 2024 at 21:01):

pchickey has marked PR #8872 as ready for review.

Wasmtime GitHub notifications bot (Nov 13 2024 at 21:01):

pchickey requested wasmtime-core-reviewers for a review on PR #8872.

Wasmtime GitHub notifications bot (Nov 13 2024 at 21:01):

pchickey requested wasmtime-default-reviewers for a review on PR #8872.

Wasmtime GitHub notifications bot (Nov 13 2024 at 21:01):

pchickey edited a comment on PR #8872:

Once https://github.com/bytecodealliance/wasm-tools/pull/1874 lands and is in a wasm-tools release, I'll update this PR to use it.
update that PR was released in wasm-tools 220

Wasmtime GitHub notifications bot (Nov 18 2024 at 17:48):

alexcrichton submitted PR review:

Looks good to me :+1:

To help with version alignment of the wasm_wave crate, how about:

• Reexporting the wasm_wave crate somewhere (e.g. wasmtime::component::wasm_wave)
• Adding some top-level conveniences (e.g. Val::{from_wave,to_wave})

and the docs for those helper methods could point to the wasm_wave reexport and additionally to the wave docs?

Wasmtime GitHub notifications bot (Nov 18 2024 at 18:57):

pchickey updated PR #8872.

Wasmtime GitHub notifications bot (Nov 18 2024 at 18:58):

pchickey commented on PR #8872:

Suggestions done. Thanks!

Wasmtime GitHub notifications bot (Nov 18 2024 at 18:58):

pchickey has enabled auto merge for PR #8872.

Wasmtime GitHub notifications bot (Nov 18 2024 at 20:01):

pchickey updated PR #8872.

Wasmtime GitHub notifications bot (Nov 18 2024 at 20:27):

pchickey merged PR #8872.

Last updated: Dec 23 2024 at 12:05 UTC