wasmtime icon indicating copy to clipboard operation
wasmtime copied to clipboard

Upstream wasm-wave instances in wasmtime

Open pchickey opened this issue 1 year ago • 1 comments

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.

pchickey avatar Jun 25 '24 17:06 pchickey

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

Kmeakin avatar Oct 19 '24 22:10 Kmeakin

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 avatar Oct 21 '24 04:10 pchickey

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

pchickey avatar Oct 21 '24 18:10 pchickey

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 avatar Nov 12 '24 23:11 pchickey

@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 avatar Nov 13 '24 21:11 pchickey

Suggestions done. Thanks!

pchickey avatar Nov 18 '24 18:11 pchickey