feat(core, value): add as_bytes_mut() to array buffer and typed array #329
Conversation
| pub fn as_bytes_mut(&self) -> Option<&mut [u8]> { | ||
| let raw = Self::get_raw(self.as_value())?; | ||
| Some(unsafe { slice::from_raw_parts_mut(raw.ptr.as_ptr(), raw.len) }) | ||
| } |
There was a problem hiding this comment.
This is unsound. It allows you to call as_bytes_mut twice on the same object resulting in aliasing mutable references which is undefined behavior. This also cannot be avoided by making the function take a mutable reference to self as you can have two separate ArrayBuffer values point to the same slice.
In order to make this sound, there needs to be some kind of RefCell/Mutex like lock to prevent aliasing mutable references.
There was a problem hiding this comment.
Thanks @DelSkayn that's a great point. I'll take some time to rework it, unless there's already a way to manipulate the bytes directly without copying that I'm missing?
There was a problem hiding this comment.
Just to chime in, this is a valid API even if unsafe.
I use it in llrt_modules to access the buffer.
The undefined behaviour is what is expected from some APIs like filehandle.write in node, see the discussion I had with node maintainers.
So I would still allow it but I would mark the function function as unsafe.
There was a problem hiding this comment.
It is unsafe and also very prone to unsoundness, especially with a scripting language which might call functions in any order this function could quickly be used wrong. Even just creating an aliasing mutable reference to the same object is unsound, even if the mutable reference is never used.
I am fine with allowing having some unsafe way to access the bytes internally but it should then be via a raw pointer, not a mutable reference. Raw pointers force the user of the API to think about how such a buffer is accessed and makes it clear that mutable access, not properly coordinated, is unsound.
There was a problem hiding this comment.
In that sense the raw interface that is already present is good enough since I was able to use it for my usecase.
I am just not sure what the worst case scenario is really is if we do pub unsafe fn as_bytes_mut(&self) -> Option<&mut [u8]>. We know the slice will be be valid since the GC wont run until self is dropped at minimum.
Yes it can be unsound if you have two concurrent access to it (even though that seems to be the expectation for Node), that is why it is marked unsafe and we should add a proper documentation.
This is not a problem unique to rquickjs, deno is in the same boat here. They allow DerefMut from it, which is even worst.
- V8Slice: https://github.com/denoland/deno_core/blob/665a61d733d4c9cb18aa81c050c135b51e9b51f5/serde_v8/magic/v8slice.rs#L59-L78
- Safety: https://github.com/denoland/deno_core/blob/665a61d733d4c9cb18aa81c050c135b51e9b51f5/serde_v8/magic/v8slice.rs#L180-L186
Their safety comment is "interesting" to say the least.
Overview
There's currently not a way to modify the bytes of an
ArrayBufferor a typed array likeUint8Array, so this PR adds newas_bytes_mut()methods that follows the typical convention in Rust.