Skip to content

node-api: do not crash on module version mismatch - #66019

Open
krassx wants to merge 3 commits into
nodejs:mainfrom
krassx:node-api-throw-instead-of-crash-on-version-mismatch
Open

krassx wants to merge 3 commits into
nodejs:mainfrom
krassx:node-api-throw-instead-of-crash-on-version-mismatch

Conversation

@krassx

@krassx krassx commented Sep 14, 2026

Copy link
Copy Markdown

Problem

node_napi_env__::New() validates the Node-API version an add-on declares. When
the add-on requires a version this binary does not support, it throws a
descriptive error and returns nullptr:

} else if (module_api_version > NODE_API_SUPPORTED_VERSION_MAX &&
           module_api_version != NAPI_VERSION_EXPERIMENTAL) {
  node::Environment* node_env = node::Environment::GetCurrent(context);
  CHECK_NOT_NULL(node_env);
  v8impl::ThrowNodeApiVersionError(
      node_env, module_filename.c_str(), module_api_version);
  return nullptr;
}

napi_module_register_by_symbol() then dereferences that nullptr without
checking it:

napi_env env =
    node_napi_env__::New(context, module_filename, module_api_version);

napi_value _exports = nullptr;
env->CallIntoModule([&](napi_env env) {   // <-- nullptr

So loading such an add-on segfaults, and the error Node already built — "…
requires Node-API version N, but this version of Node.js only supports version M
add-ons."
— is never delivered. require() cannot catch it, because the process
is gone.

Reproduction

A ten-line add-on, no third-party tooling, plain node-gyp:

#define NAPI_VERSION 99
#include <node_api.h>
static napi_value Init(napi_env env, napi_value exports) { return exports; }
NAPI_MODULE(NODE_GYP_MODULE_NAME, Init)
$ node -e "try { require('./build/Release/hello.node') } catch (e) { console.log('caught:', e.message) }"
Segmentation fault

Confirmed on v20.x, v22.x, v24.x and v26.8.2, and main, v22.x-staging and
v24.x-staging all still lack the check.

Fix

Return when New() returns nullptr; the error it threw then propagates
normally. Verified against a local build of this branch:

$ ./out/Release/node -e "try { require('.../test_module_version_mismatch.node') }
                         catch (e) { console.log('caught:', e.message) }"
caught: file:///.../test_module_version_mismatch.node requires Node-API version
2147483646, but this version of Node.js only supports version 10 add-ons.

Removing the four added lines and relinking the same tree turns that back into
exit=139 with the catch never reached, so the new test fails without the fix
and passes with it.

node-api and js-native-api both run clean on the patched build: 102 tests,
0 failures.

Test

The error path had no coverage at all — "requires Node-API version" appears in
exactly one file in the repository, src/node_api.cc, which is how it could be
unreachable without anyone noticing. The new test is modelled on
test/node-api/test_null_init, which covers the sibling early return in the same
function.

Notes for reviewers

This is not a reprise of the Node-API versioning discussion in #57233. That
issue was closed on the grounds that the add-on had been miscompiled by a
third-party tool, which is fair — but it leaves the crash in place. The version
policy is not in question here; only that a check which already detects the
problem and writes an error should be able to deliver it. The reproduction above
involves no external tooling.

#63740 reports a superficially similar segfault in napi_register_module_v1 from
a libnode.so ABI mismatch. That is a different root cause and this change does
not address it.

Closest precedent: #58459 added CHECK_NOT_NULL(node_env) to this same function
for the node::Environment pointer. This change covers the napi_env returned
by New() twelve lines below, which is the one that is nullable by design.

Refs: #57233

AI use disclosure

Per doc/contributing/ai-guidelines.md:
this change was prepared with assistance from Claude Code. The commit
message refers to it only as "a closed-source coding agent", following the
guidance in Naming AI tools in disclosures to keep commercial brand names out
of the commit log and name them here instead.

The design, review and validation are my own. Specifically, what I verified
personally rather than taking on trust:

  • The mechanism, read directly in src/node_api.ccnode_napi_env__::New()
    returns nullptr on the version-mismatch branch, and
    napi_module_register_by_symbol() dereferences it with no check. Confirmed
    under a debugger: the faulting register holds 0.
  • That the fix works, against a local build of this branch, by observing the
    intended error reach a try/catch around require().
  • That the new test can fail. I removed the four added lines, relinked the
    same tree, and confirmed the test fails there (exit=139, the catch never
    reached) and passes with them. A test that cannot fail would not be worth
    adding.
  • That the test asserts the intended behaviour rather than the implementation's
    incidental shape: it matches the error text the version check produces, with
    the supported-version number left as \d+ so it does not need updating each
    time NODE_API_SUPPORTED_VERSION_MAX moves.
  • No regressions: node-api and js-native-api run clean on the patched
    build, 102 tests, 0 failures.

I can explain and defend the change during review, and will respond to feedback
myself.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/node-api

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to Node-API. labels Sep 14, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Welcome to Node.js, and thank you for your first contribution!

Before review, please take a moment to read:

Please make sure every commit is signed off. For a first pull request, GitHub Actions require collaborator approval and Jenkins CI must be started by a collaborator or triager, so an initial wait is normal.

`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: nodejs#57233
Signed-off-by: Alexey Karimov <krassx@gmail.com>
@krassx
krassx force-pushed the node-api-throw-instead-of-crash-on-version-mismatch branch from 614013e to 089103e Compare September 14, 2026 10:45
@codecov

codecov Bot commented Sep 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.21%. Comparing base (e5778f7) to head (089103e).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #66019      +/-   ##
==========================================
- Coverage   90.22%   90.21%   -0.01%     
==========================================
  Files         785      785              
  Lines      269357   269369      +12     
  Branches    51511    51507       -4     
==========================================
- Hits       243016   243010       -6     
- Misses      16853    16882      +29     
+ Partials     9488     9477      -11     
Files with missing lines Coverage Δ
src/node_api.cc 77.07% <100.00%> (+1.73%) ⬆️

... and 33 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread test/node-api/test_module_version_mismatch/test_module_version_mismatch.c Outdated

// An add-on that requires a newer Node-API version than this binary supports
// must be rejected with an error that `require()` can catch. The version check
// in `node_napi_env__::New()` already produces that error, but its nullptr

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto, please refrain from including impl internals in the test comments.

Comment thread src/node_api.cc Outdated
krassx and others added 2 commits September 15, 2026 10:35
…_mismatch.c

Co-authored-by: Chengzhong Wu <legendecas@gmail.com>
Co-authored-by: Chengzhong Wu <legendecas@gmail.com>
@krassx

krassx commented Sep 15, 2026

Copy link
Copy Markdown
Author

@legendecas Thank you for the review. I've applied the proposed changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to Node-API.

Projects

Status: Need Triage

Development

Successfully merging this pull request may close these issues.

3 participants