From 86a07da13654bff923dbcc4263b91ac37076dab4 Mon Sep 17 00:00:00 2001 From: "K.J. Valencik" Date: Wed, 17 Jul 2024 17:37:40 -0400 Subject: [PATCH 1/2] fix(neon): Fix a panic when borrowing an empty buffer or typed array --- crates/neon/src/types_impl/buffer/lock.rs | 8 ++++-- crates/neon/src/types_impl/buffer/mod.rs | 8 ++++++ crates/neon/src/types_impl/buffer/types.rs | 32 ++++++++++++++-------- test/napi/lib/typedarrays.js | 14 ++++++++++ test/napi/src/js/typedarrays.rs | 29 ++++++++++++++++++++ test/napi/src/lib.rs | 2 ++ 6 files changed, 80 insertions(+), 13 deletions(-) diff --git a/crates/neon/src/types_impl/buffer/lock.rs b/crates/neon/src/types_impl/buffer/lock.rs index 420240b5c..c3c72fcf2 100644 --- a/crates/neon/src/types_impl/buffer/lock.rs +++ b/crates/neon/src/types_impl/buffer/lock.rs @@ -58,7 +58,9 @@ impl Ledger { ledger: &'a RefCell, data: &'a [T], ) -> Result, BorrowError> { - ledger.borrow_mut().try_add_borrow(data)?; + if !data.is_empty() { + ledger.borrow_mut().try_add_borrow(data)?; + } Ok(Ref { ledger, data }) } @@ -69,7 +71,9 @@ impl Ledger { ledger: &'a RefCell, data: &'a mut [T], ) -> Result, BorrowError> { - ledger.borrow_mut().try_add_borrow_mut(data)?; + if !data.is_empty() { + ledger.borrow_mut().try_add_borrow_mut(data)?; + } Ok(RefMut { ledger, data }) } diff --git a/crates/neon/src/types_impl/buffer/mod.rs b/crates/neon/src/types_impl/buffer/mod.rs index 247ff9d14..112906808 100644 --- a/crates/neon/src/types_impl/buffer/mod.rs +++ b/crates/neon/src/types_impl/buffer/mod.rs @@ -138,6 +138,10 @@ impl<'a, T> DerefMut for RefMut<'a, T> { impl<'a, T> Drop for Ref<'a, T> { fn drop(&mut self) { + if self.is_empty() { + return; + } + let mut ledger = self.ledger.borrow_mut(); let range = Ledger::slice_to_range(self.data); let i = ledger.shared.iter().rposition(|r| r == &range).unwrap(); @@ -148,6 +152,10 @@ impl<'a, T> Drop for Ref<'a, T> { impl<'a, T> Drop for RefMut<'a, T> { fn drop(&mut self) { + if self.is_empty() { + return; + } + let mut ledger = self.ledger.borrow_mut(); let range = Ledger::slice_to_range(self.data); let i = ledger.owned.iter().rposition(|r| r == &range).unwrap(); diff --git a/crates/neon/src/types_impl/buffer/types.rs b/crates/neon/src/types_impl/buffer/types.rs index 6b499d66c..fb4583534 100644 --- a/crates/neon/src/types_impl/buffer/types.rs +++ b/crates/neon/src/types_impl/buffer/types.rs @@ -5,7 +5,7 @@ use crate::{ handle::{internal::TransparentNoCopyWrapper, Handle}, object::Object, result::{JsResult, Throw}, - sys::{self, raw, TypedArrayType}, + sys::{self, raw, typedarray::TypedArrayInfo, TypedArrayType}, types_impl::{ buffer::{ lock::{Ledger, Lock}, @@ -549,7 +549,7 @@ where let value = self.to_local(); let info = sys::typedarray::info(env, value); - slice::from_raw_parts(info.data.cast(), info.length) + slice_from_info(info) } } @@ -562,7 +562,7 @@ where let value = self.to_local(); let info = sys::typedarray::info(env, value); - slice::from_raw_parts_mut(info.data.cast(), info.length) + slice_from_info_mut(info) } } @@ -579,10 +579,7 @@ where let info = sys::typedarray::info(env, value); // The borrowed data must be guarded by `Ledger` before returning - Ledger::try_borrow( - &lock.ledger, - slice::from_raw_parts(info.data.cast(), info.length), - ) + Ledger::try_borrow(&lock.ledger, slice_from_info(info)) } } @@ -599,10 +596,7 @@ where let info = sys::typedarray::info(env, value); // The borrowed data must be guarded by `Ledger` before returning - Ledger::try_borrow_mut( - &lock.ledger, - slice::from_raw_parts_mut(info.data.cast(), info.length), - ) + Ledger::try_borrow_mut(&lock.ledger, slice_from_info_mut(info)) } } @@ -778,6 +772,22 @@ where } } +unsafe fn slice_from_info<'a, T>(info: TypedArrayInfo) -> &'a [T] { + if info.length == 0 { + &[] + } else { + slice::from_raw_parts(info.data.cast(), info.length) + } +} + +unsafe fn slice_from_info_mut<'a, T>(info: TypedArrayInfo) -> &'a mut [T] { + if info.length == 0 { + &mut [] + } else { + slice::from_raw_parts_mut(info.data.cast(), info.length) + } +} + macro_rules! impl_typed_array { ($typ:ident, $etyp:ty, $($pattern:pat_param)|+, $tag:ident, $alias:ident, $two:expr$(,)?) => { impl private::Sealed for $etyp {} diff --git a/test/napi/lib/typedarrays.js b/test/napi/lib/typedarrays.js index 17795bdc7..ca38f0985 100644 --- a/test/napi/lib/typedarrays.js +++ b/test/napi/lib/typedarrays.js @@ -414,6 +414,20 @@ describe("Typed arrays", function () { assert.equal(b[3], 55); }); + it("copies from a source buffer to a destination with borrow API", function () { + for (const f of [addon.copy_buffer, addon.copy_buffer_with_borrow]) { + const a = Buffer.from([1, 2, 3]); + const b = Buffer.from([0, 0, 0, 4, 5, 6]); + + // Full + addon.copy_buffer(a, b); + assert.deepEqual([...b], [1, 2, 3, 4, 5, 6]); + + // Empty buffers + addon.copy_buffer(Buffer.alloc(0), Buffer.alloc(0)); + } + }); + it("zeroes the byteLength when an ArrayBuffer is detached", function () { var buf = new ArrayBuffer(16); assert.strictEqual(buf.byteLength, 16); diff --git a/test/napi/src/js/typedarrays.rs b/test/napi/src/js/typedarrays.rs index 23ac74ec1..bb8312f26 100644 --- a/test/napi/src/js/typedarrays.rs +++ b/test/napi/src/js/typedarrays.rs @@ -379,3 +379,32 @@ pub fn write_buffer_with_borrow_mut(mut cx: FunctionContext) -> JsResult JsResult { + let a = cx.argument::>(0)?; + let mut b = cx.argument::>(1)?; + let a_len = a.as_slice(&cx).len(); + let b_len = b.as_slice(&cx).len(); + let n = a_len.min(b_len); + let a = a.as_slice(&cx)[..n].to_vec(); + + b.as_mut_slice(&mut cx)[..n].copy_from_slice(&a); + + Ok(cx.undefined()) +} + +pub fn copy_buffer_with_borrow(mut cx: FunctionContext) -> JsResult { + let a = cx.argument::>(0)?; + let mut b = cx.argument::>(1)?; + let lock = cx.lock(); + let (Ok(a), Ok(mut b)) = (a.try_borrow(&lock), b.try_borrow_mut(&lock)) else { + return cx.throw_error("Borrow Error"); + }; + + let n = a.len().min(b.len()); + + b[..n].copy_from_slice(&a); + drop((a, b)); + + Ok(cx.undefined()) +} diff --git a/test/napi/src/lib.rs b/test/napi/src/lib.rs index 0065d5b77..627b3f80b 100644 --- a/test/napi/src/lib.rs +++ b/test/napi/src/lib.rs @@ -285,6 +285,8 @@ fn main(mut cx: ModuleContext) -> NeonResult<()> { cx.export_function("read_buffer_with_borrow", read_buffer_with_borrow)?; cx.export_function("write_buffer_with_lock", write_buffer_with_lock)?; cx.export_function("write_buffer_with_borrow_mut", write_buffer_with_borrow_mut)?; + cx.export_function("copy_buffer", copy_buffer)?; + cx.export_function("copy_buffer_with_borrow", copy_buffer_with_borrow)?; cx.export_function("byte_length", byte_length)?; cx.export_function("call_nullary_method", call_nullary_method)?; cx.export_function("call_unary_method", call_unary_method)?; From 70b5a6f5543e6b6144ca26830707083b1c928b85 Mon Sep 17 00:00:00 2001 From: "K.J. Valencik" Date: Thu, 18 Jul 2024 11:29:36 -0400 Subject: [PATCH 2/2] fix(ci): Temporarily remove 20.x from the build matrix to workaround npm bug --- .github/workflows/ci.yml | 4 ++-- .github/workflows/lint.yml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d2852f01a..f7dd1310d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -25,9 +25,9 @@ jobs: - name: Set Matrix id: set_matrix env: - FULL_NODE_VERSIONS: '["18.x", "20.x", "22.x"]' + FULL_NODE_VERSIONS: '["18.x", "20.x"]' FULL_RUST_TOOLCHAINS: '["stable", "nightly"]' - PARTIAL_NODE_VERSIONS: '["22.x"]' + PARTIAL_NODE_VERSIONS: '["20.x"]' PARTIAL_RUST_TOOLCHAINS: '["stable"]' HAS_FULL_MATRIX_LABEL: ${{ contains(github.event.pull_request.labels.*.name, 'full matrix') }} IS_PUSHED: ${{ github.event_name == 'push' }} diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index b84ba45e2..37866187d 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -20,7 +20,7 @@ jobs: strategy: matrix: - node-version: [22.x] + node-version: [20.x] rust-toolchain: [nightly] steps: