memory unsafety? about blake3 HOT 4 CLOSED

Good question. This was an interesting adventure into some of the internals which I haven't looked into before.

Uint8Array::view creates a Uint8Array the reference the memory of the passed slice, which is unsafe but avoids copying all the memory to a new structure.

wasm-bindgen has some docs on how they interop with JS objects. Effectively, there's a heap of objects (an array of 'stuff' in JS land) and it creates functions that copy data between the heap, as objects, and the wasm memory, as binary data/array buffers.

This is the "copy to heap" function in the generate assembly code. It takes a span of memory, and then copies that through an external JS function into a Uint8Array...

  // (memoryOffset, byteLength) -> heapIndex
  (func $js_sys::Uint8Array::view::h5952f6922d850fc5 (type $t6) (param $p0 i32) (param $p1 i32) (result i32)
    (local $l2 i32) (local $l3 i32)
    (local.set $p0
      // copy "$p1" bytes at "$p0" from the heap, and return the index of the new array
      (call $js_sys::Uint8Array::new_with_byte_offset_and_length::__wbg_newwithbyteoffsetandlength_6b93e5ed7d4086de::h969adee35a99416b
        (local.tee $l3
          // add and return ref to heap ArrayBuffer
          (call $js_sys::WebAssembly::Memory::buffer::__wbg_buffer_1bb127df6348017b::h0363ca360bd6b281
            (local.tee $l2
              (call $wasm_bindgen::memory::ha866361ff5e01285))))
        (local.get $p0)
        (local.get $p1)))

The implementation of newwithbyteoffsetandlength in JS is pretty simple:

module.exports.__wbg_newwithbyteoffsetandlength_6b93e5ed7d4086de = function(arg0, arg1, arg2) {
    // new Uint8Array(src, start, length) returns an array with a copy of the memory at the given location
    var ret = new Uint8Array(getObject(arg0) /* get heap memory */, arg1 >>> 0 /* cast to int32 */, arg2 >>> 0);
    // add this new array to the heap and return its index
    return addHeapObject(ret);
};

#[wasm_bindgen] does the magic to handle the return value, copying it to the JavaScript heap and causing the function to return the index of the copied memory in the heap. Here's the annotate wat of the hash function:

(func $hash (export "hash") (type $t6) (param $p0 i32) (param $p1 i32) (result i32)
  (local $l2 i32) (local $l3 i32)
  // allocate an area on the 'stack' for returned memory:
  (global.set $g0 
    (local.tee $l2
      (i32.sub
        (global.get $g0)
        (i32.const 32))))
  // call the hash function, with (returnLocation, heapOffset, length)
  (call $blake3::hash::h273370a0db68eed0
    (local.get $l2)
    (local.get $p0)
    (local.get $p1))
  // set l3 to the location on the heap
  (local.set $l3
    (call $js_sys::Uint8Array::view::h5952f6922d850fc5 // -> heap pointer
      (call $blake3::Hash::as_bytes::hcd433cd1201154e8
        (local.get $l2))
      (i32.const 32)))
  // deallocate if we're at the top of the stack (I think)
  (block $B0
    (br_if $B0
      (i32.eqz // $p1 == 0
        (local.get $p1)))
    (call $__rust_dealloc
      (local.get $p0)
      (local.get $p1)
      (i32.const 1)))
  (global.set $g0
    (i32.add
      (local.get $l2)
      (i32.const 32)))
  // return data loation on the JS heap
  (local.get $l3))

Then we take that object off the heap in the wrapping JS code:

module.exports.hash = function(data) {
    var ptr0 = passArray8ToWasm0(data, wasm.__wbindgen_malloc);
    var len0 = WASM_VECTOR_LEN;
    var ret = wasm.hash(ptr0, len0);
    return takeObject(ret);
};

tl;dr: this appears to be safe 🙂 I will add a test case to ensure the previous arrays aren't mutated to be extra sure, however

from blake3.

I was wrong, actually! Fixed in the linked commit

from blake3.

In the production build this was fine, but in the debug build the heap at the returned location happens to be overwritten.

Are you sure it was fine in the production build? This sounds like the kind of bug where optimizations might affect where it triggers, but where it's always going to be possible to trigger somehow.

from blake3.

Yea. I didn't dive into it in great detail, but it looked like extra work happening in the debug build caused the (now freed) heap space the Uint8Array referenced to be overwritten with other data before control was given back to JS. In the production build, less went on after the call to Uint8Array::from so the area of memory where the hash existed was never overwritten, allowing us to pull it out in JS land.

A classic use-after-free error, which would have been impossible in Rust if I was not a bad person using unsafe 😛

from blake3.