foster/denoland-deno
1
0
Fork 0
mirror of https://github.com/denoland/deno.git synced 2024-11-24 15:19:26 -05:00
Code Issues Packages 1 Wiki Activity

fix(napi): clear currently registering module slot (#19249)

Browse source 
This commit fixes problem with loading N-API modules that use 
the "old" way of registration (using "napi_module_register" API).
The slot was not cleared after loading modules, causing subsequent
calls that use the new way of registration (using 
"napi_register_module_v1" API) to try and load the previous module.

Ref https://github.com/denoland/deno/issues/16460

---------

Co-authored-by: Divy Srivastava <dj.srivastava23@gmail.com>
This commit is contained in:
Bartek Iwańczuk 2023-05-26 06:40:17 +02:00 committed by GitHub
parent 7ae55e75d8
commit 512d5337c4
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
9 changed files with 186 additions and 91 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
.github/workflows/ci.generate.ts vendored
View file

@ -17,7 +17,7 @@ const Runners = (() => {
})();
// bump the number at the start when you want to purge the cache
const prCacheKeyPrefix =
"29-cargo-target-${{ matrix.os }}-${{ matrix.profile }}-${{ matrix.job }}-";
"31-cargo-target-${{ matrix.os }}-${{ matrix.profile }}-${{ matrix.job }}-";
const installPkgsCommand =
"sudo apt-get install --no-install-recommends debootstrap clang-15 lld-15";
@ -480,7 +480,7 @@ const ci = {
"~/.cargo/git/db",
].join("\n"),
key:
"29-cargo-home-${{ matrix.os }}-${{ hashFiles('Cargo.lock') }}",
"31-cargo-home-${{ matrix.os }}-${{ hashFiles('Cargo.lock') }}",
},
},
{

6
.github/workflows/ci.yml vendored
View file

@ -293,7 +293,7 @@ jobs:
~/.cargo/registry/index
~/.cargo/registry/cache
~/.cargo/git/db
key: '29-cargo-home-${{ matrix.os }}-${{ hashFiles(''Cargo.lock'') }}'
key: '31-cargo-home-${{ matrix.os }}-${{ hashFiles(''Cargo.lock'') }}'
if: '!(github.event_name == ''pull_request'' && matrix.skip_pr)'
- name: Restore cache build output (PR)
uses: actions/cache/restore@v3
@ -305,7 +305,7 @@ jobs:
!./target/*/*.zip
!./target/*/*.tar.gz
key: never_saved
restore-keys: '29-cargo-target-${{ matrix.os }}-${{ matrix.profile }}-${{ matrix.job }}-'
restore-keys: '31-cargo-target-${{ matrix.os }}-${{ matrix.profile }}-${{ matrix.job }}-'
- name: Apply and update mtime cache
if: '!(github.event_name == ''pull_request'' && matrix.skip_pr) && (!startsWith(github.ref, ''refs/tags/''))'
uses: ./.github/mtime_cache
@ -589,7 +589,7 @@ jobs:
!./target/*/gn_out
!./target/*/*.zip
!./target/*/*.tar.gz
key: '29-cargo-target-${{ matrix.os }}-${{ matrix.profile }}-${{ matrix.job }}-${{ github.sha }}'
key: '31-cargo-target-${{ matrix.os }}-${{ matrix.profile }}-${{ matrix.job }}-${{ github.sha }}'
publish-canary:
name: publish canary
runs-on: ubuntu-22.04

5
cli/napi/env.rs
View file

@ -136,9 +136,10 @@ fn node_api_get_module_file_name(
#[napi_sym::napi_sym]
fn napi_module_register(module: *const NapiModule) -> napi_status {
MODULE.with(|cell| {
MODULE_TO_REGISTER.with(|cell| {
let mut slot = cell.borrow_mut();
slot.replace(module);
let prev = slot.replace(module);
assert!(prev.is_none());
});
napi_ok
}

140
ext/napi/lib.rs
View file

@ -81,9 +81,7 @@ pub const napi_would_deadlock: napi_status = 21;
pub const NAPI_AUTO_LENGTH: usize = usize::MAX;
thread_local! {
pub static MODULE: RefCell<Option<*const NapiModule>> = RefCell::new(None);
pub static ASYNC_WORK_SENDER: RefCell<Option<mpsc::UnboundedSender<PendingNapiAsyncWork>>> = RefCell::new(None);
pub static THREAD_SAFE_FN_SENDER: RefCell<Option<mpsc::UnboundedSender<ThreadSafeFunctionStatus>>> = RefCell::new(None);
pub static MODULE_TO_REGISTER: RefCell<Option<*const NapiModule>> = RefCell::new(None);
}
type napi_addon_register_func =
@ -346,15 +344,6 @@ impl Env {
>,
tsfn_ref_counters: Arc<Mutex<ThreadsafeFunctionRefCounters>>,
) -> Self {
let sc = sender.clone();
ASYNC_WORK_SENDER.with(|s| {
s.replace(Some(sc));
});
let ts = threadsafe_function_sender.clone();
THREAD_SAFE_FN_SENDER.with(|s| {
s.replace(Some(ts));
});
Self {
isolate_ptr,
context: context.into_raw(),
@ -559,7 +548,6 @@ where
{
let permissions = op_state.borrow_mut::<NP>();
permissions.check(Some(&PathBuf::from(&path)))?;
let (
async_work_sender,
tsfn_sender,
@ -622,77 +610,67 @@ where
Err(e) => return Err(type_error(e.to_string())),
};
MODULE.with(|cell| {
let slot = *cell.borrow();
let obj = match slot {
Some(nm) => {
// SAFETY: napi_register_module guarantees that `nm` is valid.
let nm = unsafe { &*nm };
assert_eq!(nm.nm_version, 1);
// SAFETY: we are going blind, calling the register function on the other side.
let maybe_exports = unsafe {
(nm.nm_register_func)(
env_ptr,
std::mem::transmute::<v8::Local<v8::Value>, napi_value>(
exports.into(),
),
)
};
let maybe_module = MODULE_TO_REGISTER.with(|cell| {
let mut slot = cell.borrow_mut();
slot.take()
});
let exports = maybe_exports
.as_ref()
.map(|_| unsafe {
// SAFETY: v8::Local is a pointer to a value and napi_value is also a pointer
// to a value, they have the same layout
std::mem::transmute::<napi_value, v8::Local<v8::Value>>(
maybe_exports,
)
})
.unwrap_or_else(|| {
// If the module didn't return anything, we use the exports object.
exports.into()
});
Ok(serde_v8::Value { v8_value: exports })
}
None => {
// Initializer callback.
// SAFETY: we are going blind, calling the register function on the other side.
unsafe {
let init = library
.get::<unsafe extern "C" fn(
env: napi_env,
exports: napi_value,
) -> napi_value>(b"napi_register_module_v1")
.expect("napi_register_module_v1 not found");
let maybe_exports = init(
env_ptr,
std::mem::transmute::<v8::Local<v8::Value>, napi_value>(
exports.into(),
),
);
let exports = maybe_exports
.as_ref()
.map(|_| {
// SAFETY: v8::Local is a pointer to a value and napi_value is also a pointer
// to a value, they have the same layout
std::mem::transmute::<napi_value, v8::Local<v8::Value>>(
maybe_exports,
)
})
.unwrap_or_else(|| {
// If the module didn't return anything, we use the exports object.
exports.into()
});
Ok(serde_v8::Value { v8_value: exports })
}
}
if let Some(module_to_register) = maybe_module {
// SAFETY: napi_register_module guarantees that `module_to_register` is valid.
let nm = unsafe { &*module_to_register };
assert_eq!(nm.nm_version, 1);
// SAFETY: we are going blind, calling the register function on the other side.
let maybe_exports = unsafe {
(nm.nm_register_func)(
env_ptr,
std::mem::transmute::<v8::Local<v8::Value>, napi_value>(exports.into()),
)
};
let exports = if maybe_exports.is_some() {
// SAFETY: v8::Local is a pointer to a value and napi_value is also a pointer
// to a value, they have the same layout
unsafe {
std::mem::transmute::<napi_value, v8::Local<v8::Value>>(maybe_exports)
}
} else {
exports.into()
};
// NAPI addons can't be unloaded, so we're going to "forget" the library
// object so it lives till the program exit.
std::mem::forget(library);
obj
})
return Ok(serde_v8::Value { v8_value: exports });
}
// Initializer callback.
// SAFETY: we are going blind, calling the register function on the other side.
let maybe_exports = unsafe {
let init = library
.get::<unsafe extern "C" fn(
env: napi_env,
exports: napi_value,
) -> napi_value>(b"napi_register_module_v1")
.expect("napi_register_module_v1 not found");
init(
env_ptr,
std::mem::transmute::<v8::Local<v8::Value>, napi_value>(exports.into()),
)
};
let exports = if maybe_exports.is_some() {
// SAFETY: v8::Local is a pointer to a value and napi_value is also a pointer
// to a value, they have the same layout
unsafe {
std::mem::transmute::<napi_value, v8::Local<v8::Value>>(maybe_exports)
}
} else {
exports.into()
};
// NAPI addons can't be unloaded, so we're going to "forget" the library
// object so it lives till the program exit.
std::mem::forget(library);
Ok(serde_v8::Value { v8_value: exports })
}

5
test_napi/.gitignore vendored
View file

@ -1,4 +1,7 @@
package-lock.json
# Test generated artifacts
.swc
.swc
*.dylib
*.so
*.dll

6
test_napi/common.js
View file

@ -9,17 +9,19 @@ export {
export { fromFileUrl } from "../test_util/std/path/mod.ts";
const targetDir = Deno.execPath().replace(/[^\/\\]+$/, "");
const [libPrefix, libSuffix] = {
export const [libPrefix, libSuffix] = {
darwin: ["lib", "dylib"],
linux: ["lib", "so"],
windows: ["", "dll"],
}[Deno.build.os];
const ops = Deno[Deno.internal].core.ops;
export function loadTestLibrary() {
const specifier = `${targetDir}/${libPrefix}test_napi.${libSuffix}`;
// Internal, used in ext/node
return Deno[Deno.internal].core.ops.op_napi_open(specifier, {
return ops.op_napi_open(specifier, {
Buffer: {},
});
}

14
test_napi/init_test.js Normal file
View file

@ -0,0 +1,14 @@
// Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
import { assert, libSuffix } from "./common.js";
const ops = Deno[Deno.internal].core.ops;
Deno.test("ctr initialization (napi_module_register)", {
ignore: Deno.build.os == "windows",
}, function () {
const path = new URL(`./module.${libSuffix}`, import.meta.url).pathname;
const obj = ops.op_napi_open(path, {});
assert(obj != null);
assert(typeof obj === "object");
});

68
test_napi/module.c Normal file
View file

@ -0,0 +1,68 @@
typedef struct napi_module {
int nm_version;
unsigned int nm_flags;
const char* nm_filename;
void* nm_register_func;
const char* nm_modname;
void* nm_priv;
void* reserved[4];
} napi_module;
#ifdef _WIN32
#define NAPI_EXTERN __declspec(dllexport)
#define NAPI_CDECL __cdecl
#else
#define NAPI_EXTERN __attribute__((visibility("default")))
#define NAPI_CDECL
#endif
NAPI_EXTERN void NAPI_CDECL
napi_module_register(napi_module* mod);
#if defined(_MSC_VER)
#if defined(__cplusplus)
#define NAPI_C_CTOR(fn) \
static void NAPI_CDECL fn(void); \
namespace { \
struct fn##_ { \
fn##_() { fn(); } \
} fn##_v_; \
} \
static void NAPI_CDECL fn(void)
#else // !defined(__cplusplus)
#pragma section(".CRT$XCU", read)
// The NAPI_C_CTOR macro defines a function fn that is called during CRT
// initialization.
// C does not support dynamic initialization of static variables and this code
// simulates C++ behavior. Exporting the function pointer prevents it from being
// optimized. See for details:
// https://docs.microsoft.com/en-us/cpp/c-runtime-library/crt-initialization?view=msvc-170
#define NAPI_C_CTOR(fn) \
static void NAPI_CDECL fn(void); \
__declspec(dllexport, allocate(".CRT$XCU")) void(NAPI_CDECL * fn##_)(void) = \
fn; \
static void NAPI_CDECL fn(void)
#endif // defined(__cplusplus)
#else
#define NAPI_C_CTOR(fn) \
static void fn(void) __attribute__((constructor)); \
static void fn(void)
#endif
#define NAPI_MODULE_TEST(modname, regfunc) \
static napi_module _module = { \
1, \
0, \
__FILE__, \
regfunc, \
#modname, \
0, \
{0}, \
}; \
NAPI_C_CTOR(_register_##modname) { napi_module_register(&_module); } \
void* init(void* env __attribute__((unused)), void* exports) {
return exports;
}
NAPI_MODULE_TEST(TEST_NAPI_MODULE_NAME, init)

29
test_napi/tests/napi_tests.rs
View file

@ -18,6 +18,35 @@ fn build() {
}
let build_plugin_output = build_plugin.output().unwrap();
assert!(build_plugin_output.status.success());
// cc module.c -undefined dynamic_lookup -shared -Wl,-no_fixup_chains -dynamic -o module.dylib
#[cfg(not(target_os = "windows"))]
{
let out = if cfg!(target_os = "macos") {
"module.dylib"
} else {
"module.so"
};
let mut cc = Command::new("cc");
#[cfg(not(target_os = "macos"))]
let c_module = cc.arg("module.c").arg("-shared").arg("-o").arg(out);
#[cfg(target_os = "macos")]
let c_module = {
cc.arg("module.c")
.arg("-undefined")
.arg("dynamic_lookup")
.arg("-shared")
.arg("-Wl,-no_fixup_chains")
.arg("-dynamic")
.arg("-o")
.arg(out)
};
let c_module_output = c_module.output().unwrap();
assert!(c_module_output.status.success());
}
}
#[test]