Skip to content

Commit 2ff325e

Browse files
Node-API (Chakra): fix heap corruption when an ObjectWrap constructor throws (#199)
## What Fixes a heap corruption in the ChakraCore Node-API implementation when an `ObjectWrap`-based constructor throws. Split out of #198 as an independent fix, per review. ## Problem When a wrapped (`napi_wrap`) constructor throws, the C++ `ObjectWrap` instance is destroyed by stack unwinding inside the callback, but the wrap finalizer registered by `napi_wrap()` stays attached to `this`. `~ObjectWrap()` cannot detach it, because `napi_get_reference_value()` returns null for the wrap's weak (refcount 0) reference. A later GC then runs the finalizer on the freed native instance — a use-after-free that surfaces as non-deterministic `STATUS_HEAP_CORRUPTION` (`0xC0000374`). ## Fix `Core/Node-API/Source/js_native_api_chakra.cc`: - In `ExternalCallback`, after a construct call that left a pending JS exception, detach the wrap (`napi_remove_wrap`) before returning, then re-set the exception so it is preserved across the cleanup. - Guard `napi_remove_wrap` against a `nullptr` `result` out-param (the finalizer-cleanup call above passes `nullptr`), so it no longer dereferences a null pointer. ## Test Adds a regression test (`Tests/UnitTests/Scripts/tests.ts`) that throws from a wrapped constructor 100× to create many dangling wraps, then allocates/decodes to exercise the heap and surface any corruption within the run. It uses `TextDecoder` as a convenient throwing wrapped constructor; the test passes on `main`'s existing `TextDecoder` behavior and is independent of the TextDecoder label change in #198. ## Verification Reliably reproduced as `STATUS_HEAP_CORRUPTION` on Chakra (Win32 x64/x86) before the fix; passes after. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 272f6a9 commit 2ff325e

2 files changed

Lines changed: 39 additions & 2 deletions

File tree

Core/Node-API/Source/js_native_api_chakra.cc

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,27 @@ class ExternalCallback {
172172

173173
napi_value result = externalCallback->_cb(
174174
externalCallback->_env, reinterpret_cast<napi_callback_info>(&cbInfo));
175+
176+
// If a constructor (construct call) left a pending JS exception, the C++
177+
// ObjectWrap instance was already destroyed by stack unwinding inside the
178+
// callback, but the wrap finalizer registered by napi_wrap() is still
179+
// attached to `this`. The addon-api ~ObjectWrap() cannot detach it here
180+
// because napi_get_reference_value() returns null for the wrap's weak
181+
// (refcount 0) reference. Detach the wrap now so a later GC does not run
182+
// the finalizer on the freed native instance (use-after-free / heap
183+
// corruption). The pending exception is preserved across the cleanup.
184+
if (isConstructCall) {
185+
bool hasException = false;
186+
if (JsHasException(&hasException) == JsNoError && hasException) {
187+
JsValueRef exception = JS_INVALID_REFERENCE;
188+
if (JsGetAndClearException(&exception) == JsNoError) {
189+
napi_remove_wrap(externalCallback->_env,
190+
reinterpret_cast<napi_value>(arguments[0]), nullptr);
191+
JsSetException(exception);
192+
}
193+
}
194+
}
195+
175196
return reinterpret_cast<JsValueRef>(result);
176197
}
177198

@@ -1748,9 +1769,11 @@ napi_status napi_remove_wrap(napi_env env, napi_value js_object, void** result)
17481769
CHECK_JSRT(env, JsSetExternalData(wrapper, nullptr));
17491770

17501771
if (externalData != nullptr) {
1751-
*result = externalData->Data();
1772+
if (result != nullptr) {
1773+
*result = externalData->Data();
1774+
}
17521775
delete externalData;
1753-
} else {
1776+
} else if (result != nullptr) {
17541777
*result = nullptr;
17551778
}
17561779

Tests/UnitTests/Scripts/tests.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1490,6 +1490,20 @@ describe("TextDecoder", function () {
14901490
expect(result).to.equal("H\0i");
14911491
expect(result.length).to.equal(3);
14921492
});
1493+
1494+
it("throwing from the constructor repeatedly does not corrupt native state", function () {
1495+
// Regression for a Chakra N-API ObjectWrap bug: when a wrapped
1496+
// constructor throws, the native instance is destroyed during stack
1497+
// unwinding but the wrap finalizer stayed attached to `this`, so a
1498+
// later GC ran the finalizer on freed memory (heap corruption). Throw
1499+
// many times to create many dangling wraps, then allocate/decode to
1500+
// exercise the heap and surface any corruption within this test run.
1501+
for (let i = 0; i < 100; ++i) {
1502+
expect(() => new TextDecoder("utf-16")).to.throw();
1503+
}
1504+
const decoder = new TextDecoder("utf-8");
1505+
expect(decoder.decode(new Uint8Array([79, 75]))).to.equal("OK");
1506+
});
14931507
});
14941508

14951509
describe("TextEncoder", function () {

0 commit comments

Comments
 (0)