Skip to content

Commit 089103e

Browse files
committed
node-api: do not crash on module version mismatch
`node_napi_env__::New()` returns nullptr after throwing when an add-on declares a Node-API version this binary does not support, but `napi_module_register_by_symbol()` dereferenced the result without checking it. Loading such an add-on segfaulted instead of surfacing the error the version check had already produced, so `require()` could not catch it. Reproduced on v20.x, v22.x, v24.x and v26.8.2 with a ten-line add-on whose only distinguishing content is a NAPI_VERSION above NODE_API_SUPPORTED_VERSION_MAX. `main`, `v22.x-staging` and `v24.x-staging` all lack the check. The error path had no test coverage: the message text appears in exactly one file in the repository, `src/node_api.cc`. A test is added next to `test_null_init`, which covers the sibling early return in the same function. Prepared with assistance from a closed-source coding agent, named in the pull request description. The design, review and validation are my own: I verified the fix and the test against a local build, including removing the four added lines and relinking to confirm the test fails without them. Refs: #57233 Signed-off-by: Alexey Karimov <krassx@gmail.com>
1 parent e5778f7 commit 089103e

4 files changed

Lines changed: 35 additions & 0 deletions

File tree

src/node_api.cc

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -770,6 +770,10 @@ void napi_module_register_by_symbol(v8::Local<v8::Object> exports,
770770
// Create a new napi_env for this specific module.
771771
napi_env env =
772772
node_napi_env__::New(context, module_filename, module_api_version);
773+
// `New()` returns nullptr after throwing when the add-on requires a
774+
// Node-API version this binary does not support. Returning here lets that
775+
// error surface; dereferencing `env` instead turns it into a segfault.
776+
if (env == nullptr) return;
773777

774778
napi_value _exports = nullptr;
775779
env->CallIntoModule([&](napi_env env) {
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
{
2+
'targets': [
3+
{
4+
'target_name': 'test_module_version_mismatch',
5+
'sources': [ 'test_module_version_mismatch.c' ],
6+
# One below NAPI_VERSION_EXPERIMENTAL, so it is always above
7+
# NODE_API_SUPPORTED_VERSION_MAX and never becomes a real version, but is
8+
# not the experimental value the version check deliberately allows.
9+
'defines': [ 'NAPI_VERSION=2147483646' ]
10+
}
11+
]
12+
}
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
'use strict';
2+
const common = require('../../common');
3+
const assert = require('assert');
4+
5+
// An add-on that requires a newer Node-API version than this binary supports
6+
// must be rejected with an error that `require()` can catch. The version check
7+
// in `node_napi_env__::New()` already produces that error, but its nullptr
8+
// return used to be dereferenced by `napi_module_register_by_symbol()`, so the
9+
// process segfaulted before the error could surface.
10+
assert.throws(
11+
() => require(`./build/${common.buildType}/test_module_version_mismatch`),
12+
/requires Node-API version 2147483646, but this version of Node\.js only supports version \d+ add-ons\./);
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
#include <node_api.h>
2+
3+
// This add-on declares a Node-API version that no build supports, so loading it
4+
// must fail with the error `node_napi_env__::New()` throws -- not a crash.
5+
NAPI_MODULE_INIT() {
6+
return exports;
7+
}

0 commit comments

Comments
 (0)