Skip to content

recursive use of an object detected which would lead to unsafe aliasing in rust #1578

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

Closed
dtysky opened this issue Jun 7, 2019 · 20 comments
Closed
Labels

Comments

@dtysky
Copy link

dtysky commented Jun 7, 2019

Summary

Hi, I am porting the "gl-matrix" to webassembly with wasm-bindgen. In "gl-matrix", we can find lots of those methods:

// module vec3

function add(out, a, b) {
xxx
}

In most time, arguments "out" and "a" are same variable.

I implement it in rust and have built it to wasm:

impl Vector3 {
    pub fn add(out: &mut Vector3, a: &Vector3, b: &Vector3) {
xxx
}
}

It seems lt work fine, but after running these code:

const a = Vector3.create();
const b = Vector3.create();
Vector3.add(a, a, b);

Rust thrown an error:

debug.js:21 Error: recursive use of an object detected which would lead to unsafe aliasing in rust

Do you have any good idea to solve this problem? Thanks!

@dtysky dtysky added the question label Jun 7, 2019
@alexcrichton
Copy link
Contributor

Thanks for the report! This is indeed a dynamic error detected by the runtime, and the issue here is that you can't have both & and &mut pointers to the same object in Rust. In this case for the add function both out and a have the same pointer, so that's disallowed by rustc at compile time but has to be detected at runtime in JS.

You'll either want to return a Vector3 or otherwise have a third object here, and that should do the trick!

@dtysky
Copy link
Author

dtysky commented Jun 7, 2019

Thanks for the report! This is indeed a dynamic error detected by the runtime, and the issue here is that you can't have both & and &mut pointers to the same object in Rust. In this case for the add function both out and a have the same pointer, so that's disallowed by rustc at compile time but has to be detected at runtime in JS.

You'll either want to return a Vector3 or otherwise have a third object here, and that should do the trick!

@alexcrichton

Thanks for your quick reply! I have guessed this issue is about "borrow", and have tried to change the function to these:

pub fn add(out: &mut Vector3, a: &mut Vector3, b: &Vector3) {
}

But still get the same error...I cannot understand it...

Return a Vector3 or a third object will make a copy, in my application I cannot afford the overhead...

Any ideas

@alexcrichton
Copy link
Contributor

I'd recomment exploring this example in Rust with the assistance of the Rust compiler. The idioms that work in Rust are what you'll want to transfer to JS.

@dtysky
Copy link
Author

dtysky commented Jun 8, 2019

@alexcrichton

It seems no better way to solve this problem, I had tried some unsafe code like these:

pub fn add(out: *mut Vector4, a: *mut Vector4, b: *mut Vector4) {
        unsafe {
            let out = &mut *out;
            let a = & *a;
            let b = & *b;
            out.0 = a.0 + b.0;
            out.1 = a.1 + b.1;
            out.2 = a.2 + b.2;
            out.3 = a.3 + b.3;
        }
    }

It works, but wasm-bindgen will generate js code that lose the type information and I had to pass ptr to rust myself:

math.Vector4.add((v1 as any).ptr + 4, (v1 as any).ptr + 4, (v2 as any).ptr + 4);

It's painful...If we cannot reuse out and a, the goals of performance will be lost. Rust is a great language and wasm-bindgen is a great toolchain, but in this application I cannot find any viable solutions.

Or are there any options to turn off the runtime checking?

Thanks.

@Pauan
Copy link
Contributor

Pauan commented Jun 8, 2019

I'm not 100% sure, but I'm pretty sure your code isn't sound.

Why not create a new method which treats out and a as the same?

    pub fn increment(a: &mut Vector4, b: &Vector4) {
        a.0 = a.0 + b.0;
        a.1 = a.1 + b.1;
        a.2 = a.2 + b.2;
        a.3 = a.3 + b.3;
    }

It's quite common in Rust to have different methods for different uses (e.g. new and new_with_extra_stuff), so this is quite idiomatic.

@dtysky
Copy link
Author

dtysky commented Jun 8, 2019

@Pauan

Yes your way is natural, but in my case, I must make sure my implements is as same as gl-matrix library.

We use gl-matrix in our web game engine, we want to replace the math library and ensure it could fallback to js version.

@dtysky
Copy link
Author

dtysky commented Jun 8, 2019

Emm...After all I solve this problem by some real tricks:

let fp = path.resolve(__dirname, './pkg/xxx.wasm');
const originBuffer = fs.readFileSync(fp);

const wasm = binaryen.readBinary(originBuffer);
const wast = wasm.emitText()
  .replace(/\(br_if \$label\$1[\s\n]+?\(i32.eq\n[\s\S\n]+?i32.const -1\)[\s\n]+\)[\s\n]+\)/g, '');
const distBuffer = binaryen.parseText(wast).emitBinary();

fs.writeFileSync(fp, distBuffer);

I convert the wasm file to wast, them remove borrow checking from it, at last convert it back to wasm, it works(in this case we do not need borrow checking).

@smolck
Copy link

smolck commented May 31, 2021

Is there a recommended way of debugging this issue and fixing it? Or documentation with examples of it happening that explain why it occurs in those examples? I'm running into this myself in this PR for a text editor I maintain, but I have no real idea why it's happening/how to fix it (I only ever call functions on a struct from a single reference to said struct, so no obvious violations of the borrow checker from the JS side), and this thread is the only real information on the topic I could find after searching around a bit.

@ghost
Copy link

ghost commented May 31, 2021

It seems like the underlying problem here is simply that we don't want references, we want pointers. @dtysky Would having Wasm-bindgen somehow support allowing pointers to/from the ABI fit your original use case?

@dtysky
Copy link
Author

dtysky commented Jun 1, 2021

It seems like the underlying problem here is simply that we don't want referencesz we want pointers. @dtysky Would having Wasm-bindgen somehow support allowing pointers to/from the ABI fit your original use case?

Yes, in my case i just expect the member functions could receive a pointer as self, and return a pointer as result: https://github.com/dtysky/gl-matrix-wasm

@theloni-monk
Copy link

I was able to get around this issue by creating an external function which takes a mutable reference to the object I want to modify and then performing whatever modification outside of the scope of the object in question. I think this stops the recursive error because isn't telling the object to borrow itself while JS has it already borrowed. I probably didn't explain that well but I can provide more info upon request.

@ghost
Copy link

ghost commented Jun 6, 2021

I was able to get around this issue by creating an external function which takes a mutable reference to the object I want to modify and then performing whatever modification outside of the scope of the object in question. I think this stops the recursive error because isn't telling the object to borrow itself while JS has it already borrowed. I probably didn't explain that well but I can provide more info upon request.

The last part of what you has said isn't very clear, can you show what you had done to work around this?

@theloni-monk
Copy link

theloni-monk commented Jun 6, 2021

So if we have rust code like this:

#[wasm_bindgen]
pub struct foo(u32, f64);
#[wasm_bindgen]
impl foo{
  #[wasm_bindgen]
  pub fn new() -> Result<foo, JsValue>{Ok(foo{17u32, 15f64})}
  #[wasm_bindgen]
  pub fn getter(&self ) -> Result<u32, JsValue> { Ok(self.0) }
  #[wasm_bindgen]
  pub fn setter(&mut self, in: u32){self.0 = in;}
}
#[wasm_bindgen]
pub fn setter_external(target: &mut foo, in: u32){ target.0 = in;}

This would be the corresponding JS:

let wasmLoader = import('path_to_pkg_dir/index');
wasmLoader.then((wasmInstance)=>{
let fooJs = wasmInstance.new();
fooJs.getter(); //17
//fooJs.setter(12); //ERROR recursive use of an object detected that would lead to unsave aliasing in rust
//but if we use the setter external to the object itself:
wasmInstance.setter_external(fooJs, 12); //no error
fooJs.getter(); // 12
});

Is that helpful? Sorry I don't often participate in issues and such but I though I would since I had just spent a while trying to figure this out.

@smolck
Copy link

smolck commented Jun 6, 2021

Is it intended for fooJs.setter(12); not to work in that example? It seems like any impld functions that take &mut self just don't work (or rarely work) when called from JS? Should this be documented (even if it is a bug)?

@theloni-monk Out of curiosity, if you don't call fooJs.getter() before calling fooJs.setter(12), does calling fooJs.setter(12) work?

@theloni-monk
Copy link

@smolck I'm not sure about in this specific example (I rlly don't want to set up a whole new rust + webpack + js environment just to test it) but in a larger scale project of mine, no fooJs.setter(12); would still not work. I don't think that the failure of &mut self functions is a bug, I think its just a limitation of the library(don't take my word for it tho). I definitely agree this should be documented though, as that documentation would have saved me(and others it would seem) a lot of time earlier.

@jthemphill
Copy link
Contributor

jthemphill commented Aug 25, 2023

This error definitely needs more documentation. The original example that aliases a mutable reference with another reference is clearly wrong according to Rust's rules, but @theloni-monk's code doesn't seem wrong to me at all.

I don't see why there should be any difference at runtime between a trait function that accepts &mut self or a free function that accepts the same object by mutable reference.

My project also has trait functions that accept &mut self, but my project usually doesn't throw these errors. Sometimes I get these errors nondeterministically, but I don't understand why this is happening or which references wasm-bindgen thinks is aliased. Can we print more information here?

@Liamolucko
Copy link
Collaborator

@theloni-monk's code works fine for me (although I did have to modify it slightly to fix some syntax errors):

use wasm_bindgen::prelude::*;

#[wasm_bindgen]
pub struct Foo(u32, f64);

#[wasm_bindgen]
impl Foo {
    pub fn new() -> Result<Foo, JsValue> {
        Ok(Foo(17, 15.0))
    }

    pub fn getter(&self) -> Result<u32, JsValue> {
        Ok(self.0)
    }

    pub fn setter(&mut self, value: u32) {
        self.0 = value;
    }
}

#[wasm_bindgen]
pub fn setter_external(target: &mut Foo, value: u32) {
    target.0 = value;
}
import { Foo, setter_external } from "./pkg/wasm_bindgen_playground.js";

const foo = Foo.new();
console.log(foo.getter()); // prints 17
foo.setter(12);
console.log(foo.getter()); // prints 12
setter_external(foo, 15);
console.log(foo.getter()); // prints 15

So I'm not sure what's going on there. There shouldn't be any difference between methods that take &mut self and free functions that take &mut T.

My project also has trait functions that accept &mut self, but my project usually doesn't throw these errors. Sometimes I get these errors nondeterministically, but I don't understand why this is happening or which references wasm-bindgen thinks is aliased. Can we print more information here?

It might be because you're calling those functions recursively. Something like this:

use js_sys::Function;
use wasm_bindgen::prelude::*;

#[wasm_bindgen]
pub struct Counter {
    count: u32,
}

#[wasm_bindgen]
impl Counter {
    #[wasm_bindgen(constructor)]
    pub fn new() -> Self {
        Self { count: 0 }
    }

    pub fn inc(&mut self, callback: Function) -> Result<(), JsValue> {
        self.count += 1;
        callback.call1(&JsValue::NULL, &self.count.into())?;
        Ok(())
    }
}
import { Counter } from "./pkg/wasm_bindgen_playground.js";

const counter = new Counter();
function incCallback(count) {
    if (count < 10) {
        counter.inc(incCallback) // Error: recursive use of an object detected which would lead to unsafe aliasing in rust
    }
};
counter.inc(incCallback)

This is a bit of a contrived example, but hopefully it gets the idea across. Because inc gets called again while the first inc is still running, the Counter is still mutably borrowed and can't get borrowed again. This is what the error means when it says 'recursive use of an object'.

@jthemphill
Copy link
Contributor

jthemphill commented Sep 12, 2023

It might be because you're calling those functions recursively.

I was getting this error when I was calling a recursive DFS function within Rust code. The problem went away, and I think this happened when I switched to an iterative DFS function. I can write a minimal repro to confirm.

The one thing I'm pretty sure about is that my JS code never saw more than one reference to my object. I wasn't doing any callback shenanigans, I just had a private &mut self member function in Rust, which called itself recursively. The borrow checker had no problem with it, and therefore the bindgen runtime should also accept it.

Looking at the generated webassembly, it seems there's a call to $wasm_bindgen::__rt::borrow_fail in every one of my Rust functions. That makes me think the WasmRefCell logic is getting inlined into every one of my functions, such that we grab a WasmRefCell on every function invocation. I'm still studying the webassembly to verify that.

But if so, I think this is incorrect... my understanding is that we should only be grabbing a WasmRefCell whenever we cross over from JS into Rust code.

@alivarastepour
Copy link

I really don't understand the use of the word 'recursive' in this error message, nor do I understand why the below code is encountering this error. I mean, this is a callback; how and why is it inferred that there are mutable and immutable references to the object at the same time?

  // the mutable ref
  component.register_effect(function (args) {
    console.log(component.state()); // use some getter (immutable ref)
   // or
    component.set_state((prev) => { // use another mutable ref
      return { ...prev, age: 2 };
    });
  });

@jthemphill
Copy link
Contributor

jthemphill commented May 13, 2024

@alivarastepour : Nothing is inferred; this exception is thrown at runtime.

Every type that you expose to the JS runtime is wrapped in a WasmRefCell<T>, which acts like a RefCell<T> does and follows the same rules. As with RefCell<T>, when a call to borrow() or borrow_mut() occurs within a call to borrow_mut(), an
error will be returned, which will be converted to a thrown JS exception. The error message that bindgen throws just doesn't explain this very well.

While WasmRefCell<T> exists to protect against the lack of a borrow checker in JS code, right now it interferes with correctly-written Rust code that otherwise passes the borrow checker. This is a bug, and this is what I was complaining about in my earlier comment. If Component::register_effect() calls one of Component's public methods, your generated code will still wind up calling WasmRefCell<Component>::borrow() and this error will still be thrown, even though your code follows all borrowing rules correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants