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

Error on creating a resource with a list of tuples in variant #817

Closed
Exotik850 opened this issue Jan 15, 2024 · 13 comments · Fixed by #846
Closed

Error on creating a resource with a list of tuples in variant #817

Exotik850 opened this issue Jan 15, 2024 · 13 comments · Fixed by #846
Labels
gen-rust Related to bindings for Rust-compiled-to-WebAssembly

Comments

@Exotik850
Copy link

Working on this pr and whenever I create a resource with a list of tuples inside of a variant like so:

the wit representation of the resource:

interface toml {
  /// The handle for a `TomlValue`
  resource toml {
    /// Creates a value in table and returns the handle
    constructor(value: toml-value);
    /// Clones value from table
    get: func() -> toml-value;
    /// Sets value in table
    set: func(value: toml-value);
    /// Clones the value and makes a new handle
    clone: func() -> toml;
  }

  variant toml-value {
    %string(string),
    integer(s64),
    float(float64),
    boolean(bool),
    datetime(datetime),
    %array(array),
    %table(table),
  }

  record datetime {
    date: option<date>,
    time: option<time>,
    offset: option<offset>,
  }

  record date {
    year: u16,
    month: u8,
    day: u8,
  }

  record time {
    hour: u8,
    minute: u8, 
    second: u8,
    nanosecond: u32,
  }

  variant offset {
    z,
    custom(tuple<s8,u8>),
  }

  type array = list<toml>;
  type table = list<tuple<string, toml>>; // The problem child
}

This is the implementation of the resource, the bindings just call these functions:

impl PluginRuntimeState {
    pub fn get_toml(&self, value: Resource<Toml>) -> TomlValue {
        self.tomls
            .get(value.rep() as usize)
            .expect("Resource gaurantees existence")
            .clone()
    }

    pub fn set_toml(&mut self, key: Resource<Toml>, value: TomlValue) {
        *self
            .tomls
            .get_mut(key.rep() as usize)
            .expect("Resource gaurantees existence") = value;
    }

    pub fn insert_toml(&mut self, value: TomlValue) -> usize {
        self.tomls.insert(value)
    }

    pub fn new_toml(&mut self, value: TomlValue) -> Resource<Toml> {
        Resource::new_own(self.insert_toml(value) as u32)
    }

    pub fn clone_handle(&mut self, handle: &Resource<Toml>) -> Resource<Toml> {
        let new_toml = self.get_toml(Resource::new_own(handle.rep()));
        self.new_toml(new_toml)
    }
}

It throws this error when calling the constructor of the function with table values like so:

struct Plugin;

impl Guest for Plugin {
    fn get_default_config() -> Toml {
        let values = vec![(
            "Test".into(),
            Toml::new(TomlValue::String("THIS A TEST".into())),
        )];

        let table = TomlValue::Table(values);
        let toml = Toml::new(table); // The error occurs here

        let value = toml.get();
        log(&format!("{value:?}"));
        toml
    }
    ...

The error isn't very helpful as to what went wrong:

[WARN] Couldn't get default config from plugin: TestPlugin : error while executing at wasm backtrace:
   0: 0x2a0f8a - wit-component:shim!indirect-plugins:main/toml-[constructor]toml
   1: 0x979c - test_plugin::plugins::main::toml::Toml::new::he2c438c40e24e4ec
                   at D:\Code\Rust\dioxus\packages\cli-plugin\src\export_plugin.rs:4:9
   2: 0x168e2 - <test_plugin::Plugin as test_plugin::exports::plugins::main::definitions::Guest>::get_default_config::h924bfaae2c70a627
                   at D:\Code\Rust\dioxus\packages\cli-plugin\examples\test-plugin\src\lib.rs:21:20
   3: 0x11b99 - plugins:main/definitions#get-default-config
                   at D:\Code\Rust\dioxus\packages\cli-plugin\src\export_plugin.rs:4:9

I thought it was because it was a list of resources at first, but when it's initialized with an array object it works just fine, and
even when the implementation and signature are changed to be a list<tuple<string, string>>, it will cause the same error.

I would like to try and fix this if I can, but I don't even know where to start 😅

@Exotik850 Exotik850 changed the title Error on creating a resource with a list of strings in variant Error on creating a resource with a list of tuples in variant Jan 15, 2024
@alexcrichton
Copy link
Member

Thanks for the report, but there's quite a lot behind this issue which isn't here and might be where the issue is from reading over this. Nothing looks awry from reading over this but I'm not sure for example where the [WARN] is coming from. Should the [WARN] not be printed? If so, could you show the logic that's printing it?

If you could provide a reproducible example for this that'd be a great start!

@Exotik850
Copy link
Author

The [WARN] is just our own debugging when calling the binding, which is being called here

I've made a repo that reproduces the issue in as little code I could: https://github.com/Exotik850/wit-resource-fail
Just running the test-host example should do it, but all the other parts of the process are in there as well,
Thank you for your quick reply!

@alexcrichton
Copy link
Member

How did you build test.wasm? I can reproduce with test.wasm as-is in the repository but if I run cargo build -p test-plugin --target wasm32-wasi and rebuild test.wasm myself with wasm-tools component new ./target/wasm32-wasi/debug/test_plugin.wasm --adapt wasi_snapshot_preview1.wasm -o test.wasm then the issue doesn't reproduce.

Also, to confirm, is the error you're seeing this:

thread 'main' panicked at examples/test-host/src/main.rs:101:10:
called `Result::unwrap()` on an `Err` value: error while executing at wasm backtrace:
    0: 0x259cbd - wit-component:shim!indirect-plugins:main/toml-[constructor]toml
    1: 0x6404 - <unknown>!test_plugin::plugins::main::toml::Toml::new::h40eba8924dad2660
    2: 0x137c - <unknown>!<test_plugin::Plugin as test_plugin::exports::plugins::main::definitions::Guest>::run::h82681d419b0799ec
    3: 0x1576 - <unknown>!plugins:main/definitions#run
note: using the `WASMTIME_BACKTRACE_DETAILS=1` environment variable may show more debugging information

Caused by:
    invalid utf-8 sequence of 1 bytes from index 0

If so that indicates what's probably a bug in the guest and/or corruption there because invalid utf-8 is being passed somewhere by accident.

@Exotik850
Copy link
Author

Exotik850 commented Jan 18, 2024

Hmm, that's quite strange, It works on my work laptop but not my home computer, both running windows 11.
I've added a readme that shows what we've done to make the wasm files and run it, so I'll dig a bit deeper once I get home and see if it isn't just an issue on my computer,
Thanks for all your help!

@Exotik850
Copy link
Author

Exotik850 commented Jan 18, 2024

After a bit more testing I can't find what the exact difference would be in how each is being run, setting all of the toolchains to be the same, both have the latest version of wasm-tools, both have the same operating system version and everything, but when the exact same commands are run they both have different results:
Laptop output
image

Desktop output
image

With both building everything from scratch beforehand and making the wasm file into a component

Edit: It is also throwing the same error as my windows desktop when ran on mac

@Exotik850
Copy link
Author

Exotik850 commented Jan 18, 2024

Turns out I forgot to have $env:CARGO_INCREMENTAL=true set on the desktop, once that was set it started building it correctly, not really sure how that would affect this particular issue but if it works it works 🤷‍♂️

@alexcrichton
Copy link
Member

Hm I wonder if you're perhaps running into a rustc issue in that case? I'm also not sure how, though. Do you have a way to build test.wasm from source that reproduces the issue though?

@Exotik850
Copy link
Author

After fiddling with it for a while I think I'm getting close to something, if I initialize the string that would go into the tomlvalue beforehand like this:

let val = "THIS A TEST".to_string();
log(&val);
let val = TomlValue::String(val);
let key = "Test".to_string();
log(&key);
let values: Vec<(String, Toml)> = vec![(
    key,
    Toml::new(val),
)];
let table = TomlValue::Table(values);
let toml = Toml::new(table);
let value = toml.get();
log(&format!("{value:?}"));
toml

it jumbles it before the variant is sent over to the host:

THIS A TEST
Test
INSERTING TOML : TomlValue::String("l \u{10}\0l \u{10}\0\u{10}\0\0")
New Resource : 0
INSERTING TOML : TomlValue::Table([("@\"\u{10}\0", Resource { rep: 0, state: "own (not in table)" })])
New Resource : 1
GETTING TOML Resource { rep: 1, state: "borrow" }
 Slab { len: 2, cap: 4 }
TomlValue::Table([("@\"\u{10}\0", Toml { handle: Resource { handle: 1 } })])
TomlValue::Table([("@\"\u{10}\0", Resource { rep: 0, state: "own (not in table)" })])

But when initialized in the same line as the tomlvalue like this:

let val = TomlValue::String("THIS A TEST".to_string());

It doesn't let the toml for the string be initialized at all:

Test
thread 'main' panicked at examples\test-host\src\main.rs:103:10:
called `Result::unwrap()` on an `Err` value: error while executing at wasm backtrace:
    0: 0x240ddf - wit-component:shim!indirect-plugins:main/toml-[constructor]toml
    1: 0x9dd5 - test_plugin::plugins::main::toml::Toml::new::hedffd76ba3fde429
                    at D:\Code\Rust\wit-resource-fail\src\export_plugin.rs:4:9
    2: 0x5c6c - <test_plugin::Plugin as test_plugin::exports::plugins::main::definitions::Guest>::run::h7c3d96060d2a60a7
                    at D:\Code\Rust\wit-resource-fail\examples\test-plugin\src\lib.rs:21:13
    3: 0x6010 - plugins:main/definitions#run
                    at D:\Code\Rust\wit-resource-fail\src\export_plugin.rs:4:9

Caused by:
    invalid utf-8 sequence of 1 bytes from index 0

This happens on both my laptop and my desktop, and the most recent changes have been pushed to the MRE

@alexcrichton
Copy link
Member

Can you provide details on how to build this all from source to reproduce the error you're seeing on both systems? Basically what repos at what commits are used and what commands are done to run this.

@Exotik850
Copy link
Author

Exotik850 commented Jan 23, 2024

There's a readme in this repo that has all of the commands to build the wasm and then run the host. Swapping between the commented out lines in the test-plugin example lib.rs file reproduces the errors on both systems

@alexcrichton alexcrichton added the gen-rust Related to bindings for Rust-compiled-to-WebAssembly label Jan 24, 2024
@alexcrichton
Copy link
Member

Ah ok this is definitely a bug in the generated bindings and what's amounting to UB/use-after-free. Looking at the generated code I see:

 91             impl Toml {
 92                 #[allow(unused_unsafe, clippy::all)]
 93                 /// Creates a value in table and returns the handle
 94                 pub fn new(value: TomlValue) -> Self {
 95                     #[allow(unused_imports)]
 96                     use wit_bindgen::rt::{alloc, string::String, vec::Vec};
 97                     unsafe {
 98                         let mut cleanup_list = Vec::new();
 99                         let (result4_0, result4_1, result4_2) = match value {
100                             TomlValue::String(e) => {
101                                 let vec0 = e;
102                                 let ptr0 = vec0.as_ptr() as i32;
103                                 let len0 = vec0.len() as i32;
104                                 (0i32, ptr0, len0)
105                             }
...

where the string e is dropped before the ptr/len are ready by the imported function. This I think additionally applies to the other case as well.

alexcrichton added a commit to alexcrichton/witx-bindgen that referenced this issue Feb 17, 2024
This commit fixes an soundness issue in the Rust code generator's generated
code. Previously unsound code was generated when:

* An import had a parameter
* That had either a list or a variant of an aggregate where:
* Where one field was a `list<T>` or `string`
* And another field was an owned resource

In this situation the Rust generator uses an "owned" type for the argument,
such as `MyStruct`. This is done to reflect how ownership of the resource in
the argument is lost when the function is called. The problem with this,
however, is that the rest of bindings generation assumes that imported
arguments are all "borrowed" meaning that raw pointers from lists/strings can
be passed through to the canonical ABI. This is not the case here because the
argument is owned, meaning that the generated code would record an argument to
be passed to the canonical ABI and then promptly deallocate the argument.

The fix here is preceded by a refactoring to how Rust manages owned types to
make the fix possible. The general idea for the fix though is that while `x:
MyStruct` is the argument to the function all references internally are through
`&x` to ensure that it remains rooted as an argument, preserving all pointers
to lists and such. This unfortunately means that ownership can no longer model
movement of resources and instead interior mutability must be used (since we
have to move out of `&Resource<T>` since everything is borrowed). Fixing that,
however, is probably not worth the complexity at this time.

Closes bytecodealliance#817
Closes bytecodealliance/wasmtime#7951
@alexcrichton
Copy link
Member

Ok I have gotten around to really digging into this and I've confirmed with your repository that it's fixed by #846. Thank you again for taking the time to open this issue and work with me debugging it, I very much appreciate it!

@Exotik850
Copy link
Author

Absolutely! Glad I could be of help 😊

alexcrichton added a commit to alexcrichton/witx-bindgen that referenced this issue Feb 20, 2024
This commit fixes an soundness issue in the Rust code generator's generated
code. Previously unsound code was generated when:

* An import had a parameter
* That had either a list or a variant of an aggregate where:
* Where one field was a `list<T>` or `string`
* And another field was an owned resource

In this situation the Rust generator uses an "owned" type for the argument,
such as `MyStruct`. This is done to reflect how ownership of the resource in
the argument is lost when the function is called. The problem with this,
however, is that the rest of bindings generation assumes that imported
arguments are all "borrowed" meaning that raw pointers from lists/strings can
be passed through to the canonical ABI. This is not the case here because the
argument is owned, meaning that the generated code would record an argument to
be passed to the canonical ABI and then promptly deallocate the argument.

The fix here is preceded by a refactoring to how Rust manages owned types to
make the fix possible. The general idea for the fix though is that while `x:
MyStruct` is the argument to the function all references internally are through
`&x` to ensure that it remains rooted as an argument, preserving all pointers
to lists and such. This unfortunately means that ownership can no longer model
movement of resources and instead interior mutability must be used (since we
have to move out of `&Resource<T>` since everything is borrowed). Fixing that,
however, is probably not worth the complexity at this time.

Closes bytecodealliance#817
Closes bytecodealliance/wasmtime#7951
github-merge-queue bot pushed a commit that referenced this issue Feb 20, 2024
* Refactor how Rust handles ownership

This commit is the next in a long line of refactors to try to model how
the Rust generator handles ownership. The Rust generator has an
`ownership` knob for deciding how to render types. The Rust generator
additionally models ownership of resources/lists in params/results of
imports/exports. Putting all of this together has led to many attempts
to wrangle this in a form that's understandable but every time a bug
comes up I feel like I don't understand what's going on. I've made
another attempt in this commit.

Here the goal is to centralize knowledge about types as much as
possible. Instead of being spread out over a few methods everything is
hopefully now centralized in a single type and a single "main method".
All other little pieces stem from these two and are modeled to be
relatively simple compared to the "main method". Whether or not this
stands the test of time we'll see.

This does change generated code in some situations as can be seen by the
test that was updated. Nothing major should be changed, but a few more
borrows are now required in places which probably should have been
borrows before.

More comments are found in the code about specific changes made here.

* Fix unsoundness in generated Rust code

This commit fixes an soundness issue in the Rust code generator's generated
code. Previously unsound code was generated when:

* An import had a parameter
* That had either a list or a variant of an aggregate where:
* Where one field was a `list<T>` or `string`
* And another field was an owned resource

In this situation the Rust generator uses an "owned" type for the argument,
such as `MyStruct`. This is done to reflect how ownership of the resource in
the argument is lost when the function is called. The problem with this,
however, is that the rest of bindings generation assumes that imported
arguments are all "borrowed" meaning that raw pointers from lists/strings can
be passed through to the canonical ABI. This is not the case here because the
argument is owned, meaning that the generated code would record an argument to
be passed to the canonical ABI and then promptly deallocate the argument.

The fix here is preceded by a refactoring to how Rust manages owned types to
make the fix possible. The general idea for the fix though is that while `x:
MyStruct` is the argument to the function all references internally are through
`&x` to ensure that it remains rooted as an argument, preserving all pointers
to lists and such. This unfortunately means that ownership can no longer model
movement of resources and instead interior mutability must be used (since we
have to move out of `&Resource<T>` since everything is borrowed). Fixing that,
however, is probably not worth the complexity at this time.

Closes #817
Closes bytecodealliance/wasmtime#7951
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gen-rust Related to bindings for Rust-compiled-to-WebAssembly
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants