rusty_v8 icon indicating copy to clipboard operation
rusty_v8 copied to clipboard

Using the deref value of a shared BackingStore might be undefined behavior

Open andreubotella opened this issue 3 years ago • 2 comments

In the C++ v8 API, BackingStore simply exposes a pointer to the chunk of memory that belongs to the backing store and leaves it to the user to use it responsibly. In order to offer a safe API, rusty_v8 has BackingStore deref to a slice of Cell<u8>. But although this seems to be safe if the backing store isn't shared, for stores corresponding to SharedArrayBuffers, using this deref value seems to be undefined behavior.

The Javascript memory model allows two types of orderings on (Shared)ArrayBuffers: Unordered and SeqCst. The former is the ordering used with raw reads or writes, while the latter is the one you get by using the methods in the Atomics global. Although I'm not 100% sure that the SeqCst ordering is equivalent to using C++20/Rust atomics with Ordering::SeqCst, that seems likely to be the case.

However, the unordered ordering provides no guarantees other than a lack of UB – my understanding is that unordered access don't even have to be atomic. Since raw memory accesses in C++ or Rust would cause UB, it might be safe to use atomics with Ordering::Relaxed, since it's always okay to use a stronger ordering than needed – but it wasn't clear to me that that wouldn't interact with the JIT code in unexpected ways. However, I found this v8 issue which fixed a case of UB in accessing a SAB's memory by replacing it with relaxed atomics, so using that is presumably fine.


I propose to fix this by getting rid of the Deref implementation and instead add the following method:

impl BackingStore {
    pub fn as_slice<T: BackingStoreElement>(&self) -> BackingStoreSlice<'_, T> {
        todo!()
    }
}

enum BackingStoreSlice<'a, T: BackingStoreElement> {
    SingleThreaded(&'a [Cell<T>]),
    Shared(&'a [T::Atomic]),
}

trait BackingStoreElement {
    type Atomic;
}
impl BackingStoreElement for u8 {
    type Atomic = AtomicU8;
}
impl BackingStoreElement for i8 {
    type Atomic = AtomicI8;
}
impl BackingStoreElement for u16 {
    type Atomic = AtomicU16;
}
// ...

This API wouldn't force users to choose either Ordering::Relaxed or Ordering::SeqCst, but my understanding is that using other orderings might cause unexpected results but wouldn't be UB no matter what the JIT does.

andreubotella avatar Jun 22 '21 22:06 andreubotella

cc @piscisaureus

lucacasonato avatar Jun 22 '21 22:06 lucacasonato

I found another case where the Deref of a BackingStore might be UB, even in the non-shared case: for length-zero BackingStores, it seems like the pointer returned by bs.data() is always the null pointer – and creating a slice from the null pointer is UB, even for empty slices. This can be fixed ahead of the rest of this issue by creating a slice from NonNull::dangling() in those cases.

andreubotella avatar Oct 25 '21 08:10 andreubotella