Skip to content

Commit

Permalink
fix(napi): typed arrays ref shouldn't use offset. (#1376)
Browse files Browse the repository at this point in the history
Notice from the n-api docs that the data returned from
`napi_get_typedarray_info` is already adjusted by the byte offset.
https://nodejs.org/api/n-api.html#napi_get_typedarray_info

This means that when `as_ref`/`as_mut` apply the byte offset, the
offset is in practice applied twice.
This wasn't caught in tests because no test tried to modify a typed
array with a byte offset, and the test didn't us the typed array
structs, only `JsTypedArray`. If you want, I can modify the rest of the
functions in examples/napi-compt-mode/src/arraybuffers.rs
and the matching tests, to test all typed arrays.

IMO the `byte_offset` field can be removed entirely from the struct,
but I wanted to submit a minimal PR.
  • Loading branch information
nihohit committed Nov 30, 2022
1 parent 573f67b commit 1cf3263
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 6 deletions.
4 changes: 0 additions & 4 deletions crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs
Expand Up @@ -207,16 +207,12 @@ macro_rules! impl_typed_array {
impl AsRef<[$rust_type]> for $name {
fn as_ref(&self) -> &[$rust_type] {
unsafe { std::slice::from_raw_parts(self.data, self.length) }
.split_at(self.byte_offset)
.1
}
}

impl AsMut<[$rust_type]> for $name {
fn as_mut(&mut self) -> &mut [$rust_type] {
unsafe { std::slice::from_raw_parts_mut(self.data, self.length) }
.split_at_mut(self.byte_offset)
.1
}
}

Expand Down
7 changes: 7 additions & 0 deletions examples/napi-compat-mode/__test__/arraybuffer.spec.ts
Expand Up @@ -17,6 +17,13 @@ test('should be able to mutate Uint8Array', (t) => {
t.is(fixture[0], 42)
})

test('should be able to mutate Uint8Array in its middle', (t) => {
const fixture = new Uint8Array([0, 1, 2])
const view = new Uint8Array(fixture.buffer, 1, 1)
bindings.mutateUint8Array(view)
t.is(fixture[1], 42)
})

test('should be able to mutate Uint16Array', (t) => {
const fixture = new Uint16Array([0, 1, 2])
bindings.mutateUint16Array(fixture)
Expand Down
7 changes: 5 additions & 2 deletions examples/napi-compat-mode/src/arraybuffer.rs
@@ -1,7 +1,10 @@
use std::f64::consts::PI;
use std::str;

use napi::{CallContext, JsArrayBuffer, JsNumber, JsObject, JsTypedArray, JsUndefined, Result};
use napi::{
bindgen_prelude::Uint8Array, CallContext, JsArrayBuffer, JsNumber, JsObject, JsTypedArray,
JsUndefined, Result,
};

#[js_function(1)]
pub fn get_arraybuffer_length(ctx: CallContext) -> Result<JsNumber> {
Expand All @@ -11,7 +14,7 @@ pub fn get_arraybuffer_length(ctx: CallContext) -> Result<JsNumber> {

#[js_function(1)]
pub fn mutate_uint8_array(ctx: CallContext) -> Result<JsUndefined> {
let mut buffer = ctx.get::<JsTypedArray>(0)?.into_value()?;
let mut buffer = ctx.get::<Uint8Array>(0)?;
let buffer_mut_ref: &mut [u8] = buffer.as_mut();
buffer_mut_ref[0] = 42;
ctx.env.get_undefined()
Expand Down

0 comments on commit 1cf3263

Please sign in to comment.