Stream: general

Topic: ✔ wasmparser assumptions about Resources with static func...

Frank Rehwinkel (May 03 2024 at 16:33):

I've found a block of code that bails when a resource that is being imported has a static function.

https://github.com/bytecodealliance/wasm-tools/blob/df542a839b1fc6c1345c68abf5da21ca9ef5f9fe/crates/wasmparser/src/validator/component.rs#L3162-L3170

If that doesn't expand here, let me do it:

            // Static methods don't have much validation beyond that they must
            // be a function and the resource name referred to must already be
            // in this context.
            ComponentNameKind::Static(name) => {
                func()?;
                if !self.all_resource_names.contains(name.resource().as_str()) {
                    bail!(offset, "static resource name is not known in this context");
                }
            }

An issue that was created against cargo-component yesterday, https://github.com/bytecodealliance/cargo-component/issues/284
has a small example that fails.
The first WIT is

package test:exporter@0.1.0;

interface exports {
    resource my-resource {
        get-static: static func() -> string;
    }
}

world example {
    export exports;
}

and the second that wants to use it but fails is

package test:importer@0.1.0;

world my-world {
    import test:exporter/exports@0.1.0;
}

I've traced enough to know that it fails because the all_resource_names IndexSet is still empty. The "my-resource" string can't be found in it.

Does anyone here have an idea why it might be empty? I can keep digging in a few hours if the problem isn't immediately obvious to someone. But the two WIT files are very small and the validation assumptions made by the wasmparser validator are still probably fresh in someone's mind.

Thanks.

Alex Crichton (May 03 2024 at 17:04):

Given this error, do you have a wasm on hand which should be valid but the validator is rejecting it? If so can you share the wasm? (and perhaps how to build the wasm as well)

Frank Rehwinkel (May 03 2024 at 17:10):

exporter.wasm

Alex Crichton (May 03 2024 at 17:11):

as in, that's considered invalid? If I run wasm-tools validate on it then it looks like it validates successfully

Frank Rehwinkel (May 03 2024 at 17:12):

Agreed. It is valid when I run was-tools validate on it too.

Alex Crichton (May 03 2024 at 17:12):

but through a different means it's hitting an error in the validator?

Frank Rehwinkel (May 03 2024 at 17:12):

But it fails when the second component is being built and wants to import it.

Alex Crichton (May 03 2024 at 17:12):

do you have a wasm for the second component?

Frank Rehwinkel (May 03 2024 at 17:13):

Well, I do if you count the case where I commented out that check that bails. You want that one?

Alex Crichton (May 03 2024 at 17:13):

sure yeah

Alex Crichton (May 03 2024 at 17:13):

(still trying to catch up on the context here myself)

Alex Crichton (May 03 2024 at 17:14):

this may also be a bug in wasm-compose where it's not generating a resource type when it should

Frank Rehwinkel (May 03 2024 at 17:18):

Well, that second wasm binary is built, even when the cargo component build fails with the error message. I guess it is checking things after the fact.

importer $ cargo component build --release
   Compiling wit-bindgen-rt v0.24.0
   Compiling importer v0.1.0 (/private/tmp/try/min-ex/importer)
    Finished `release` profile [optimized] target(s) in 0.32s
    Creating component target/wasm32-wasi/release/importer.wasm
error: failed to validate component output

Caused by:
    export name `[static]my-resource.get-static` is not valid
    static resource name is not known in this context (at offset 0xb)
 importer $ l $(find . -name '*.wasm')
-rwxr-xr-x  1 frank  wheel   1.6M May  3 13:16 ./target/wasm32-wasi/release/deps/importer-cc9d5b41ee4288ab.wasm
-rwxr-xr-x  1 frank  wheel   1.6M May  3 13:16 ./target/wasm32-wasi/release/importer.wasm

It also passes the wasm-tools validate on its own.

Joel Dice (May 03 2024 at 17:19):

I can confirm (as the one who added it) that wasm-compose's resource support is... less than robust. I've got some patches lying around for other issues I've run into, but haven't prioritized getting them upstreamed, partly because it seems like wac is where the focus is these days. But sounds like there's a problem even before wasm-compose is involved?

Joel Dice (May 03 2024 at 17:23):

Or does cargo-component use wasm-compose behind the scenes?

Ryan Levick (rylev) (May 03 2024 at 17:29):

I believe cargo-component uses wasm-compose for component dependencies. I believe @Peter Huene was hoping to move it over to use wac at some point in the nearish future.

Frank Rehwinkel (May 03 2024 at 17:31):

It's true that the second wasm crate, the one named importer is using the first crate named exporter. But now I see the second wasm crate build fails with the same message, even when the first one's crate has just had 'cargo clean' run it so there is no target in it, no ".wasm" to pick up. So I don't think this is a compose issue - at least not the composing of two components.

Alex Crichton (May 03 2024 at 17:34):

can you get a copy of importer.wasm perhaps? The raw output of rustc before cargo-component turns it into a component?

Frank Rehwinkel (May 03 2024 at 17:34):

It is the cargo component build step in the second crate that fails because the wasmparser has found the name of the resource isn't found in the list it thinks it setup for validation. The comment around the check for static functions of a resource says there isn't much to check except that the resource name was known. Well, here it isn't known. I don't know if it should have been known or if the logic behind the check is wrong.

I don't see something like this being tested in the cargo-component crate.

Alex Crichton (May 03 2024 at 17:34):

this may also be a bug in wit-component where it's producing an invalid component

Peter Huene (May 03 2024 at 17:35):

Ryan Levick (rylev) said:

I believe cargo-component uses wasm-compose for component dependencies. I believe Peter Huene was hoping to move it over to use wac at some point in the nearish future.

There's no use of wasm-compose for component dependencies; cargo-component just synthesizes imports for each component's exports in the target world for bindings generation.

I haven't looked too closely at this issue, but there might be something wrong with what it's giving wit-bindgen.

Frank Rehwinkel (May 03 2024 at 17:36):

Alex Crichton said:

can you get a copy of importer.wasm perhaps? The raw output of rustc before cargo-component turns it into a component?

I would be happy too. But I don't know the command sequence for that.

Peter Huene (May 03 2024 at 17:36):

run cargo build --target wasm32-wasi, the .wasm in targets/wasm32-wasi/debug will be a module

Frank Rehwinkel (May 03 2024 at 17:39):

Hmm. That created exactly the same wasm files. I ran sum on them.

Peter Huene (May 03 2024 at 17:40):

If it's failing to compentize with cargo component build, the original .wasm will still be present too

Frank Rehwinkel (May 03 2024 at 17:40):

Maybe the one I already uploaded was the first step? When it is a raw module?

Peter Huene (May 03 2024 at 17:41):

i think you uploaded exporter.wasm and Alex was requesting importer.wasm, but maybe I missed it

Frank Rehwinkel (May 03 2024 at 17:42):

You are right. I don't see the second one in the stream now either. Thought I had uploaded it. Here it is again. And then the zip file that has both crates in it that came with the original issue.

Frank Rehwinkel (May 03 2024 at 17:43):

importer.wasm

Frank Rehwinkel (May 03 2024 at 17:43):

min-ex.zip

Alex Crichton (May 03 2024 at 17:47):

ok I can reproduce the error with wasm-tools component new ~/Downloads/importer.wasm --adapt ./wasi_snapshot_preview1.reactor.wasm

Alex Crichton (May 03 2024 at 17:47):

would you mind opening an issue on the wasm-tools repo with importer.wasm and that comand line for this?

Alex Crichton (May 03 2024 at 17:48):

I'll try to get to investigating what's going on soon

Frank Rehwinkel (May 03 2024 at 17:49):

Sure. No problem. Glad you don't think it is something obviously wrong with the WIT files or the Cargo.toml files. Those are always the first places someone can go wrong when it is actually operator error.

Alex Crichton (May 03 2024 at 17:51):

no yeah I suspect this is a bug in wit-component

Peter Huene (May 03 2024 at 17:51):

Definitely not operator error and thanks for reporting it!

Alex Crichton (May 03 2024 at 17:51):

I think I have a grasp on the shape of the bug too just need some time later to dig in properly

Frank Rehwinkel (May 03 2024 at 18:06):

Here is the issue.

Alex Crichton (May 03 2024 at 18:32):

thanks!

Alex Crichton (May 03 2024 at 19:40):

ok turned out to be a pretty easy fix -- https://github.com/bytecodealliance/wasm-tools/pull/1530 -- thanks again for the report!

Peter Huene (May 03 2024 at 19:41):

Thanks alex!

Notification Bot (May 04 2024 at 22:09):

Frank Rehwinkel has marked this topic as resolved.

Last updated: Nov 22 2024 at 17:03 UTC