JsArrayBuffer::external soundness hole

[Cassy343]

Undefined behavior can be triggered with a misuse of JsArrayBuffer::external using safe code.

Rust:

use neon::prelude::*;
use std::{
    mem,
    sync::{Arc, Mutex},
    thread,
    time::Duration,
};

#[neon::main]
fn main(mut cx: ModuleContext) -> NeonResult<()> {
    let buffer = Arc::new(Mutex::new(vec![0u8, 1, 2, 3]));

    cx.export_function("get_buf", {
        let buffer = Arc::clone(&buffer);
        move |mut cx| {
            Ok(JsArrayBuffer::external(&mut cx, &mut **buffer.lock().unwrap()))
        }
    })?;

    thread::spawn(move || {
        thread::sleep(Duration::from_secs(3));
        drop(mem::take(&mut *buffer.lock().unwrap()));
        println!("Buffer dropped");
        // `buffer` is also implicitly dropped here, so the explicit drop isn't strictly necessary
    });

    Ok(())
}

JavaScript:

const rust = require('.');
let buf = rust.get_buf();
setTimeout(() => console.log(buf), 5000);

Result:

$ node index.js 
Buffer dropped
ArrayBuffer { [Uint8Contents]: <d0 b3 53 06>, byteLength: 4 }

[kjvalencik]

@Cassy343 Good find! It looks like both JsArrayBuffer::external and JsBuffer::external are missing the 'static bound on T.

A more minimal example demonstrating the soundness hole:

pub fn soundness_hole(mut cx: FunctionContext) -> JsResult<JsArrayBuffer> {
    let mut data = vec![0u8, 1, 2, 3];
    let buf = JsArrayBuffer::external(&mut cx, data.as_mut_slice());

    Ok(buf)
    // `data` is dropped here, but `buf` is holding a reference!
}

A longer example that clearly shows the data dropping while a reference is still held:

struct BytesMut(Vec<u8>);

impl Drop for BytesMut {
    fn drop(&mut self) {
        eprintln!("Dropping `BytesMut`!");
    }
}

impl AsMut<[u8]> for BytesMut {
    fn as_mut(&mut self) -> &mut [u8] {
        self.0.as_mut_slice()
    }
}

pub fn soundness_hole(mut cx: FunctionContext) -> JsResult<JsArrayBuffer> {
    let mut data = BytesMut(vec![0, 1, 2, 3]);
    let buf = JsArrayBuffer::external(&mut cx, data.as_mut());

    drop(data);

    Ok(buf)
}

With the bound, this is properly a compile error:

169 |     let buf = JsArrayBuffer::external(&mut cx, data.as_mut_slice());
    |                                                ^^^^^^^^^^^^^^^^^^^
    |                                                |
    |                                                borrowed value does not live long enough
    |                                                argument requires that `data` is borrowed for `'static`
...
172 | }
    | - `data` dropped here while still borrowed

I have not directly tested your example, but it should be identical since &mut **buffer.lock().unwrap() will deref to a &mut [u8]. Ultimately this is the problem since there is a blanket implementation of AsMut<Self> for all &mut T.

[Cassy343]

There's another more subtle error (bug?) that shows up when you only add 'static. From my testing, the v8 engine will crash if you create two array buffers with overlapping regions in memory, but the 'static bound allows you to violate that like so:

use neon::prelude::*;

#[neon::main]
fn main(mut cx: ModuleContext) -> NeonResult<()> {
    cx.export_function("empty_buf", move |mut cx| {
        let buffer = JsArrayBuffer::external(
            &mut cx,
            &mut []
        );
        Ok(buffer)
    })?;

    Ok(())
}

Result:

$ node
Welcome to Node.js v16.13.0.
Type ".help" for more information.
> const rust = require('.');
undefined
> rust.empty_buf()
ArrayBuffer { [Uint8Contents]: <>, byteLength: 0 }
> rust.empty_buf()


#
# Fatal error in , line 0
# Check failed: result.second.
#
#
#
#FailureMessage Object: 0x7ffc8cf945f0
 1: 0xb6f151  [node]
 2: 0x1bf56f4 V8_Fatal(char const*, ...) [node]
 3: 0xfc3f61 v8::internal::GlobalBackingStoreRegistry::Register(std::shared_ptr<v8::internal::BackingStore>) [node]
 4: 0xd151c8 v8::ArrayBuffer::GetBackingStore() [node]
 5: 0xadd964  [node]
 6: 0xd4a18e  [node]
 7: 0xd4b5af v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [node]
 8: 0x15e7959  [node]
Trace/breakpoint trap (core dumped)

I'm not sure if the right choice here is to just clearly document this behavior (if it's expected), or bite the bullet and require an owned value (perhaps T: Into<Box<[u8]>>.

[kjvalencik]

@Cassy343 That's interesting because it seems to be related to internal behavior of V8 that expects unique references.

Nothing about this is unsafe from a Rust perspective. This is only possible with an empty array. Rust's ownership rules would prevent this as soon as there is data (wouldn't be able to get multiple &'static mut references without unsafe).

I'm more comfortable filing that as a separate bug and taking time to identify the problem since it's not a potential security issue (like the current problem).

[Cassy343]

Sounds good, I'll leave you to file that since I think you have a better idea of how to categorize/characterize the problem.

[kjvalencik]

It actually looks like it might be a Node issue: Node #32463. It used to work and no longer does.

Interestingly, I've only been able to make it happen in the REPL.

[kjvalencik]

I did a little more testing and the issue you found with duplicate of the same (empty) pointer is fixed in Node 17 and Node 18.

[kjvalencik]

@Cassy343 Thanks so much for reporting this issue!

This has been fixed and published in Neon 0.10.1 and on main. A rustsec advisory is pending at rustsec/advisory-db#1256.