fix: harden N-API boundary against JS-triggerable memory bugs by nazarhussain · Pull Request #60 · ChainSafe/zapi

nazarhussain · 2026-07-22T05:26:31Z

Fixes JS-triggerable memory-safety bugs found in a security audit of src/js and the N-API layer.

Fixes

BigInt word OOB read (getValueBigintWords): napi reports the required word count, which can exceed the caller's buffer; slicing by it read out of bounds. Now returns error.Overflow.
BigInt.toI128 overflow: out-of-range magnitudes and the valid -2^127 both tripped @intCast. Now range-checked.
Unknown napi enum values (Status, ValueType, TypedarrayType): exhaustive enums from napi out-params were invalid-enum UB (e.g. Float16Array, newer statuses). Made non-exhaustive; unknown values map to errors.
Constructor without new: wrapped native state onto globalThis. Now throws TypeError.
Error-path memory bugs (not reachable from the test harness): double-free in materializeClassInstance, use-after-free in registerClass list linking, catch unreachable in module registration.
Bonus: implemented expectType/expectTypedArrayOfType, which every js.Value.as*() method called but were never defined (compile error on use, hidden by lazy analysis).

Impact depends on the consumer's optimize mode — zapi ships as source, so the build mode is chosen downstream. lodestar-z builds ReleaseSafe, where these surface as safety-check panics → process abort (DoS). They would be UB only under ReleaseFast/ReleaseSmall (safety checks elided), which lodestar-z doesn't use.

Tests

zig build test:napi / test:zapi, pnpm test:js all green
New regression tests: i128 boundaries, out-of-range BigInts, no-new calls, value narrowing
examples/targets has one pre-existing, unrelated failure (musl detection on macOS)

🤖 Generated with Claude Code

@intcast

napi sets the out word_count to the required count, which can exceed the caller's buffer; slicing by it read out of bounds for BigInts wider than the buffer (panic in safe builds, UB in ReleaseFast). getValueBigintWords returns error.Overflow instead. toI128 now range-checks the magnitude and handles -2^127, which previously tripped @intcast before negation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Status, ValueType, and TypedarrayType were exhaustive enums populated straight from napi out-params; a status or array type this binding doesn't know (e.g. Float16Array, statuses added in newer Node) was invalid-enum UB in ReleaseFast and a panic in safe builds. Make them non-exhaustive and map unknown values to errors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Without new, V8 hands the global proxy as the receiver, so the generated constructor wrapped native state and a finalizer onto globalThis and type-tagged it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

- wrapTaggedObject destroyed the native object on tagging failure while materializeClassInstance's errdefer destroyed it again (double free); ownership now stays with the caller on error - registerClass linked the entry before addEnvCleanupHook, so a hook failure freed the list head while still reachable (use-after-free) and leaked the ctor ref - module registration aborted via catch unreachable when throwError failed with an exception already pending None of these paths are reachable from the JS test harness (they need napi calls failing mid-sequence), hence no regression tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

expectType and expectTypedArrayOfType were called by every as*() narrowing method but never defined; Zig's lazy analysis hid it until a consumer instantiated one, which then failed to compile. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

spiral-ladder

safe builds panic (process abort / DoS); ReleaseFast — the usual production build — gets UB.

This is not true, we're using ReleaseSafe for zapi. I don't think we should use ReleaseSafe anywhere accept for bls (the rust bindings is compiled with -O3 which is == ReleaseFast)

spiral-ladder · 2026-07-23T08:19:25Z

 /// Read a BigInt's first u64 word via `getValueBigintWords` passing `null` for `sign_bit`.
 ///
-/// Throws if word_count > 1.
+/// Throws `Overflow` if the BigInt needs more than one word.


If this is the intended behaviour, maybe rename to bigIntSingleWord? bigIntFirstWord seems to imply there's more than one word and we're taking only the first

Good call — renamed to bigIntSingleWord (6a6231f). It reads a single-word BigInt and throws otherwise, so "first" was misleading.

spiral-ladder · 2026-07-23T08:20:27Z

+}
+
+/// Narrow an untyped value to a Uint8Array and return its length.
+pub fn narrowToUint8ArrayLen(v: Value) !Number {


do we need the Len suffix?

Kept it — the fn returns the length (a Number), not the array, so the suffix names what comes back. Happy to drop it if you'd rather; it's just example code.

spiral-ladder · 2026-07-23T10:10:16Z

 }

+/// On error the caller keeps ownership of `native_object`: the wrap is
+/// detached so the finalizer cannot fire, but the object is not destroyed.


i think maybe it's worth documenting why we don't want the finalizer to fire for now (because consumer cleans it up), this comment in itself isn't helpful w/o context imo

Expanded it (4245470): wrap() installs a finalizer that frees the object on GC; on error the caller owns the free instead, so we detach the finalizer first — otherwise both free it.

spiral-ladder · 2026-07-23T10:15:33Z

    bigint64 = c.napi_bigint64_array,
    biguint64 = c.napi_biguint64_array,
+    // Non-exhaustive: engines already ship array types napi doesn't name yet
+    // (e.g. Float16Array); unknown values must be representable, not UB.


question: i know the _ is to account for future upgrades adding new types, but do we necessarily want to silently accept and not UB?

It's not silent — _ just makes an unknown value representable (reading an unknown into an exhaustive enum is the UB). We then reject it: elementSize() → null → error.GenericFailure → TypeMismatch thrown to JS. An exhaustive enum would instead be UB/panic at @enumFromInt, before we ever get to reject. So this is what lets us turn an unknown type into a clean error instead of UB.

spiral-ladder · 2026-07-23T10:24:07Z

also, it appears some of BLS code use these codepaths, we could perhaps have regression tests aimed at what they used?

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

nazarhussain · 2026-07-24T09:23:30Z

Looked at blst.zig — it exercises typed-array narrowing (isTypedarray/getTypedarrayInfo, Uint8Array.toSlice/fromExternal) and class construction/unwrap, but no BigInt and no no-new calls. Both of the paths it hits are already covered by the regression tests here: Uint8Array narrowing by exact subtype, and class construction/cross-class binding. The BigInt fixes aren't on BLS's path. Happy to add a test mirroring a specific BLS pattern if there's one you want pinned down.

The helper reads a single-word BigInt and errors otherwise; "first" wrongly implied there are more words to follow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

nazarhussain · 2026-07-24T09:32:20Z

safe builds panic (process abort / DoS); ReleaseFast — the usual production build — gets UB.

This is not true, we're using ReleaseSafe for zapi. I don't think we should use ReleaseSafe anywhere accept for bls (the rust bindings is compiled with -O3 which is == ReleaseFast)

You're right — and since zapi ships as source, the build mode is the consumer's call, not zapi's. lodestar-z builds ReleaseSafe, so these surface as safety-check panics → process abort (DoS), not UB; UB would only apply to a ReleaseFast/ReleaseSmall consumer, which we don't use. Corrected the description to frame it that way.

spiral-ladder

code changes lgtm but i personally prefer not having an ai reply to me

nazarhussain · 2026-07-24T11:42:07Z

code changes lgtm but i personally prefer not having an ai reply to me

It was the first attempt for that experiment, but none of those replies were automated.

GrapeBaBa · 2026-07-24T15:13:00Z

-    } else |_| {};
+    // Assumed infallible right after a successful wrap; if removeWrap ever
+    // failed the finalizer would stay live and both it and the caller would free.
+    errdefer _ = env.removeWrap(T, object) catch {};


What is the behavior when catch {} triggered, is it possible double free or memory leak? Is it possible testing this case?

This comment also confused me it should infallible after a successful wrap, but actually below statements still can throw error, why not make the env.wrap after these checks?

GrapeBaBa · 2026-07-24T15:30:49Z


                    const init_result = callInit(init_fn, args) orelse return null;

                    const obj_ptr = std.heap.c_allocator.create(T) catch {


This fail seems probably leak the init_result, reorder obj_ptr and init_result may more simple fix

GrapeBaBa · 2026-07-24T16:07:24Z

    no_external_buffers_allowed = c.napi_no_external_buffers_allowed,
    cannot_run_js = c.napi_cannot_run_js,
+    // Non-exhaustive: future Node versions may add statuses.
+    _,


this still panic

nazarhussain and others added 6 commits July 22, 2026 10:19

fix(dsl): reject class constructor calls without new

182d1e2

Without new, V8 hands the global proxy as the receiver, so the generated constructor wrapped native state and a finalizer onto globalThis and type-tagged it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

docs: note the removeWrap infallibility invariant

57fdbf3

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

nazarhussain marked this pull request as ready for review July 23, 2026 07:09

nazarhussain requested review from GrapeBaBa and spiral-ladder July 23, 2026 07:10

spiral-ladder reviewed Jul 23, 2026

View reviewed changes

docs: explain wrapTaggedObject finalizer ownership

4245470

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

refactor(example): rename bigIntFirstWord to bigIntSingleWord

6a6231f

The helper reads a single-word BigInt and errors otherwise; "first" wrongly implied there are more words to follow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

nazarhussain requested a review from spiral-ladder July 24, 2026 09:32

spiral-ladder approved these changes Jul 24, 2026

View reviewed changes

GrapeBaBa reviewed Jul 24, 2026

View reviewed changes


		const init_result = callInit(init_fn, args) orelse return null;

		const obj_ptr = std.heap.c_allocator.create(T) catch {

Uh oh!

Conversation

nazarhussain commented Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fixes

Tests

Uh oh!

spiral-ladder left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spiral-ladder commented Jul 23, 2026

Uh oh!

nazarhussain commented Jul 24, 2026

Uh oh!

nazarhussain commented Jul 24, 2026

Uh oh!

spiral-ladder left a comment

Choose a reason for hiding this comment

Uh oh!

nazarhussain commented Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nazarhussain commented Jul 22, 2026 •

edited

Loading

spiral-ladder left a comment •

edited

Loading

nazarhussain commented Jul 24, 2026 •

edited

Loading