Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upstream wasm-wave instances in wasmtime #8872

Merged
merged 7 commits into from
Nov 18, 2024
Merged

Upstream wasm-wave instances in wasmtime #8872

merged 7 commits into from
Nov 18, 2024

Conversation

pchickey
Copy link
Contributor

@pchickey pchickey commented Jun 25, 2024

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.

@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Jun 25, 2024
@Kmeakin
Copy link
Contributor

Kmeakin commented Oct 19, 2024

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

@pchickey
Copy link
Contributor Author

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.

@pchickey
Copy link
Contributor Author

pchickey commented Oct 21, 2024

Once bytecodealliance/wasm-tools#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

@pchickey pchickey force-pushed the pch/upstream_wave branch 2 times, most recently from 7f37566 to 9b718b1 Compare November 12, 2024 23:28
@pchickey
Copy link
Contributor Author

pchickey commented Nov 12, 2024

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

@pchickey
Copy link
Contributor Author

@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?

@pchickey pchickey marked this pull request as ready for review November 13, 2024 21:01
@pchickey pchickey requested review from a team as code owners November 13, 2024 21:01
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

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?

@pchickey
Copy link
Contributor Author

Suggestions done. Thanks!

@pchickey pchickey enabled auto-merge November 18, 2024 18:58
@pchickey pchickey added this pull request to the merge queue Nov 18, 2024
Merged via the queue into main with commit 4c6739f Nov 18, 2024
42 checks passed
@pchickey pchickey deleted the pch/upstream_wave branch November 18, 2024 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants