[weak-refs] UAF in cli-generated JS shims when dealing with by-value receivers

[danielhenrymantilla]

Describe the Bug

When the --weak-refs feature is enabled, the JS shim code that wasmbindgen-cli generates (out_dir/libname_bg.js file) is incorrect and leads to UAFs (e.g., double free) when a decorated [wasm_bindgen] method uses a by-value receiver.

The very short technical description of the bug is that in the JS glue wrapping such a method is missing a call to ThatClassFinalization.unregister(this);.

Steps to Reproduce

1. //! src/lib.rs
   use ::wasm_bindgen::prelude::*;
   
   #[wasm_bindgen]
   pub struct Foo { /* … */ }
   
   #[wasm_bindgen]
   impl Foo {
       #[wasm_bindgen(constructor)]
       pub fn new(/* … */) -> Self {
           Self { /* … */ }
       }
   
       pub fn identity(self) -> Self {
           self
       }
   }

2. # Cargo.toml
   [lib]
   crate-type = ["cdylib"]
   
   [package]
   name = "example"
   version = "0.0.0"
   edition = "2018"
   
   [dependencies]
   wasm-bindgen = "0.2.70"

3. cargo build --target wasm32-unknown-unknown
   wasm-bindgen --out-dir out_dir --out-refs ./target/wasm32-unknown-unknown/debug/example.wasm

   The generated out_dir/example_bg.js will then contain, among other things:

   const FooFinalization = new FinalizationRegistry(ptr => wasm.__wbg_foo_free(ptr));
   /**
   */
   export class Foo {
       static __wrap(ptr) {
           const obj = Object.create(Foo.prototype);
           obj.ptr = ptr;
           FooFinalization.register(obj, obj.ptr, obj);
           return obj;
       }
   
       free() {
           const ptr = this.ptr;
           this.ptr = 0;
           FooFinalization.unregister(this);
           wasm.__wbg_foo_free(ptr);
       }
       /**
       */
       constructor() {
           var ret = wasm.foo_new();
           return Foo.__wrap(ret);
       }
       /**
       * @returns {Foo}
       */
       identity() {
           var ptr = this.ptr;
           this.ptr = 0;
           var ret = wasm.foo_identity(ptr);
           return Foo.__wrap(ret);
       }
   }

4. Now, somewhere within JS code interacting with this wasm library, do:

   let foo = new Foo();
   let foo2 = foo.identity();

Expected behavior

Everything is fine

Actual behavior

Some use-after-free or double-free error occurs, such as a borrow_mut() error, once garbage collection disposes of both foo and foo2.

Screenshot of a debugged situation

The following is not exactly a screenshot of the minimal repro, but rather, of a personal debugging session that was conducted to identify the cause of the borrow_mut error. In that debugging session, all WasmRefCells were monkey-patched to log whenever a borrow occurred or one ended, with a global ever incrementing "uid" to distinguish between each WasmRefCell instance:

As you can see, we have a wbg_…_free called twice on the same instance #4.

Explanation

When foo is GC-ed, wasm.__wbg_foo_free(ptr) will be called by the Finalization registry, but that ptr was actually given in an owned fashion to wasm.foo_identity(ptr), which deallocates that ptr when it does Box::from_raw:

• Code for wasm.foo_identity

  /// `wasm.foo_identity`
  pub extern "C" fn __wasm_bindgen_generated_Foo_identity(
      me: u32,
  ) -> <Foo as wasm_bindgen::convert::ReturnWasmAbi>::Abi {
      let _ret = {
          // let me = unsafe { *Box::from_raw(me) }; /* frees `me` */
          let me = unsafe { <Foo as wasm_bindgen::convert::FromWasmAbi>::from_abi(me) };
          me.identity()
      };
      <Foo as wasm_bindgen::convert::ReturnWasmAbi>::return_abi(_ret)
  }

Solution

Do the same thing that the glue for JS free currently does (in that regard, free() is not that special, it could simply be the glue generated for a dummy drop-ping method, such as impl Foo { pub fn free (self) {} }.

Fix implemented in #2448

[mikialex]

This fix #2448 only fix self consumed case not the parameter's. For example if you have

#[wasm-bindgen]
pub fn test(foo: A){...}

foo will used after free when --weak-refs config is on.

Should we reopen this issue or file another one?