Merge pull request #907 from neon-bindings/kv/type-tag

safety(neon): Improve safety of JsBox on Node-API >= 8

crates/neon/Cargo.toml

@@ -20,6 +20,7 @@ anyhow = "1.0.58" # used for a doc example
 nodejs-sys = "0.13.0"
 
 [dependencies]
+getrandom = { version = "0.2.7", optional = true }
 libloading = "0.7.3"
 semver = "1"
 smallvec = "1.4.2"
@@ -55,7 +56,7 @@ napi-4 = ["napi-3"]
 napi-5 = ["napi-4"]
 napi-6 = ["napi-5"]
 napi-7 = ["napi-6"]
-napi-8 = ["napi-7"]
+napi-8 = ["napi-7", "getrandom"]
 napi-latest = ["napi-8"]
 napi-experimental = ["napi-8"]
 

crates/neon/src/lib.rs

@@ -98,3 +98,26 @@ pub use neon_macros::*;
 
 #[cfg(feature = "napi-6")]
 mod lifecycle;
+
+#[cfg(feature = "napi-8")]
+static MODULE_TAG: once_cell::sync::Lazy<crate::sys::TypeTag> = once_cell::sync::Lazy::new(|| {
+    let mut lower = [0; std::mem::size_of::<u64>()];
+
+    // Generating a random module tag at runtime allows Neon builds to be reproducible. A few
+    //  alternativeswere considered:
+    // * Generating a random value at build time; this reduces runtime dependencies but, breaks
+    //   reproducible builds
+    // * A static random value; this solves the previous issues, but does not protect against ABI
+    //   differences across Neon and Rust versions
+    // * Calculating a variable from the environment (e.g. Rust version); this theoretically works
+    //   but, is complicated and error prone. This could be a future optimization.
+    getrandom::getrandom(&mut lower).expect("Failed to generate a Neon module type tag");
+
+    // We only use 64-bits of the available 128-bits. The rest is reserved for future versioning and
+    // expansion of implementation.
+    let lower = u64::from_ne_bytes(lower);
+
+    // Note: `upper` must be non-zero or `napi_check_object_type_tag` will always return false
+    // https://github.com/nodejs/node/blob/5fad0b93667ffc6e4def52996b9529ac99b26319/src/js_native_api_v8.cc#L2455
+    crate::sys::TypeTag { lower, upper: 1 }
+});

crates/neon/src/sys/bindings/functions.rs

@@ -335,6 +335,13 @@ mod napi8 {
         extern "C" {
             fn object_freeze(env: Env, object: Value) -> Status;
             fn object_seal(env: Env, object: Value) -> Status;
+            fn type_tag_object(env: Env, object: Value, tag: *const TypeTag) -> Status;
+            fn check_object_type_tag(
+                env: Env,
+                object: Value,
+                tag: *const TypeTag,
+                result: *mut bool,
+            ) -> Status;
         }
     );
 }

crates/neon/src/sys/bindings/types.rs

@@ -226,3 +226,11 @@ pub struct Deferred__ {
 }
 
 pub type Deferred = *mut Deferred__;
+
+#[cfg(feature = "napi-8")]
+#[repr(C)]
+#[derive(Debug, Copy, Clone)]
+pub struct TypeTag {
+    pub(crate) lower: u64,
+    pub(crate) upper: u64,
+}

crates/neon/src/sys/external.rs

@@ -35,14 +35,19 @@ pub unsafe fn deref<T: Send + 'static>(env: Env, local: Local) -> Option<*const
 
     let result = result.assume_init();
 
-    // Note: This only validates it is an external, not that it was created by
-    // this module. In this future, this can be improved with type tagging:
-    // https://nodejs.org/api/n-api.html#n_api_napi_type_tag
-    // https://github.com/neon-bindings/neon/issues/591
+    // Ensure we have an external
     if result != napi::ValueType::External {
         return None;
     }
 
+    // As a future improvement, this could be done with a dynamic symbol check instead of
+    // relying on the Node-API version compatibility at compile time.
+    #[cfg(feature = "napi-8")]
+    // Check the external came from this module
+    if !super::tag::check_object_type_tag(env, local, &crate::MODULE_TAG) {
+        return None;
+    }
+
     let mut result = MaybeUninit::uninit();
     let status = napi::get_value_external(env, local, result.as_mut_ptr());
 
@@ -70,5 +75,11 @@ pub unsafe fn create<T: Send + 'static>(env: Env, v: T, finalizer: fn(Env, T)) -
     // or shutting down.
     assert_eq!(status, napi::Status::Ok);
 
-    result.assume_init()
+    let external = result.assume_init();
+
+    #[cfg(feature = "napi-8")]
+    // Tag the object as coming from this module
+    super::tag::type_tag_object(env, external, &crate::MODULE_TAG);
+
+    external
 }

crates/neon/src/sys/no_panic.rs

@@ -253,6 +253,7 @@ unsafe fn panic_msg(panic: &Panic) -> Option<&str> {
 }
 
 unsafe fn external_from_panic(env: Env, panic: Panic) -> Local {
+    let fail = || fatal_error("Failed to create a neon::types::JsBox from a panic");
     let mut result = MaybeUninit::uninit();
     let status = napi::create_external(
         env,
@@ -263,10 +264,17 @@ unsafe fn external_from_panic(env: Env, panic: Panic) -> Local {
     );
 
     if status != napi::Status::Ok {
-        fatal_error("Failed to create a neon::types::JsBox from a panic");
+        fail();
     }
 
-    result.assume_init()
+    let external = result.assume_init();
+
+    #[cfg(feature = "napi-8")]
+    if napi::type_tag_object(env, external, &*crate::MODULE_TAG) != napi::Status::Ok {
+        fail();
+    }
+
+    external
 }
 
 extern "C" fn finalize_panic(_env: Env, data: *mut c_void, _hint: *mut c_void) {

crates/neon/src/sys/tag.rs

@@ -114,3 +114,21 @@ pub unsafe fn is_promise(env: Env, val: Local) -> bool {
     );
     result
 }
+
+#[cfg(feature = "napi-8")]
+pub unsafe fn type_tag_object(env: Env, object: Local, tag: &super::TypeTag) {
+    assert_eq!(
+        napi::type_tag_object(env, object, tag as *const _),
+        napi::Status::Ok
+    );
+}
+
+#[cfg(feature = "napi-8")]
+pub unsafe fn check_object_type_tag(env: Env, object: Local, tag: &super::TypeTag) -> bool {
+    let mut result = false;
+    assert_eq!(
+        napi::check_object_type_tag(env, object, tag as *const _, &mut result as *mut _),
+        napi::Status::Ok
+    );
+    result
+}