Repository navigation
fix: harden N-API boundary against JS-triggerable memory bugs #60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
123382a
e118b09
182d1e2
5af7585
1deb61c
57fdbf3
4245470
6a6231f
a5f59d7
a04a490
ab52f61
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -102,11 +102,10 @@ pub fn doubleBigInt(n: BigInt) !BigInt { | |
|
|
||
| /// 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. | ||
| pub fn bigIntFirstWord(n: BigInt) !Number { | ||
| var words: [1]u64 = .{0}; | ||
| const got = try n.toValue().getValueBigintWords(null, &words); | ||
| if (got.len > 1) return error.BigIntTooLarge; | ||
| _ = try n.toValue().getValueBigintWords(null, &words); | ||
| return Number.from(words[0]); | ||
| } | ||
|
|
||
|
|
@@ -118,6 +117,14 @@ pub fn bigIntSign(n: BigInt) !Number { | |
| return Number.from(@as(u32, sign)); | ||
| } | ||
|
|
||
| /// Convert a BigInt to an i128 and render it as a decimal string. | ||
| pub fn bigIntToI128String(n: BigInt) !String { | ||
| const v = try n.toI128(); | ||
| var buf: [48]u8 = undefined; | ||
| const s = std.fmt.bufPrint(&buf, "{d}", .{v}) catch return error.FormatError; | ||
| return String.from(s); | ||
| } | ||
|
|
||
| /// Add one day (86400000ms) to a Date. | ||
| pub fn tomorrow(d: Date) Date { | ||
| const ts = d.assertTimestamp(); | ||
|
|
@@ -309,6 +316,18 @@ pub const Buffer = struct { | |
| // Section 10: Mixed DSL + N-API | ||
| // ============================================================================ | ||
|
|
||
| /// Narrow an untyped value to a Number via the `js.Value` narrowing API. | ||
| pub fn narrowToNumber(v: Value) !Number { | ||
| return v.asNumber(); | ||
| } | ||
|
|
||
| /// Narrow an untyped value to a Uint8Array and return its length. | ||
| pub fn narrowToUint8ArrayLen(v: Value) !Number { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need the Len suffix?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kept it — the fn returns the length (a |
||
| const arr = try v.asUint8Array(); | ||
| const slice = try arr.toSlice(); | ||
| return Number.from(@as(u32, @intCast(slice.len))); | ||
| } | ||
|
|
||
| /// Return the JS typeof string for any value. | ||
| /// Demonstrates dropping down to low-level napi to call raw N-API methods. | ||
| pub fn getTypeOf(val: Value) !String { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,12 +8,13 @@ pub fn typeTag(comptime T: type) napi.c.napi_type_tag { | |
| }; | ||
| } | ||
|
|
||
| /// On error the caller keeps ownership of `native_object`: the wrap is | ||
| /// detached so the finalizer cannot fire, but the object is not destroyed. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| pub fn wrapTaggedObject(comptime T: type, env: napi.Env, object: napi.Value, native_object: *T) !void { | ||
| const tag = typeTag(T); | ||
| try env.wrap(object, T, native_object, defaultFinalize(T), null, null); | ||
| errdefer if (env.removeWrap(T, object)) |removed| { | ||
| destroyNativeObject(T, removed); | ||
| } else |_| {}; | ||
| // Assumed infallible: a failing removeWrap leaves the finalizer live and the caller double-frees. | ||
| errdefer _ = env.removeWrap(T, object) catch {}; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the behavior when This comment also confused me it should infallible after a successful wrap, but actually below statements still can throw error, why not make the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call — reordered so the tag/check runs first and |
||
| if (!(try env.checkObjectTypeTag(object, tag))) { | ||
| try env.typeTagObject(object, tag); | ||
| } | ||
|
|
@@ -68,9 +69,12 @@ pub fn registerClass(comptime T: type, env: napi.Env, ctor: napi.Value) !void { | |
| .ctor_ref = try env.createReference(ctor, 1), | ||
| .next = State.head, | ||
| }; | ||
| State.head = entry; | ||
| errdefer entry.ctor_ref.delete() catch {}; | ||
|
|
||
| // Link only after the cleanup hook is registered: a hook failure must not | ||
| // leave a freed entry reachable from the list head. | ||
| try env.addEnvCleanupHook(State.Entry, entry, State.cleanupHook); | ||
| State.head = entry; | ||
| } | ||
|
|
||
| /// Per-thread marker set by `materializeClassInstance` to tell the generated | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,8 @@ pub const Status = enum(c.napi_status) { | |
| would_deadlock = c.napi_would_deadlock, | ||
| 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. | ||
| _, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this still panic
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Switched to |
||
| }; | ||
|
|
||
| pub const NapiError = error{ | ||
|
|
@@ -80,6 +82,7 @@ pub fn check(code: c_uint) NapiError!void { | |
| .would_deadlock => return error.WouldDeadlock, | ||
| .no_external_buffers_allowed => return error.NoExternalBuffersAllowed, | ||
| .cannot_run_js => return error.CannotRunJS, | ||
| _ => return error.GenericFailure, | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -112,3 +115,9 @@ test "isNapiError identifies napi errors" { | |
| try std.testing.expect(isNapiError(error.GenericFailure)); | ||
| try std.testing.expect(!isNapiError(error.OutOfMemory)); | ||
| } | ||
|
|
||
| test "check maps unknown status codes to GenericFailure" { | ||
| // Future Node versions may return statuses this enum doesn't know about; | ||
| // they must map to an error, not invalid-enum UB. | ||
| try std.testing.expectError(error.GenericFailure, check(9999)); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,8 @@ pub const ValueType = enum(c.napi_valuetype) { | |
| function = c.napi_function, | ||
| external = c.napi_external, | ||
| bigint = c.napi_bigint, | ||
| // Non-exhaustive: future Node versions may add value types. | ||
| _, | ||
| }; | ||
|
|
||
| /// https://nodejs.org/api/n-api.html#napi_typedarray_type | ||
|
|
@@ -49,13 +51,18 @@ pub const TypedarrayType = enum(c.napi_typedarray_type) { | |
| float64 = c.napi_float64_array, | ||
| 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. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: i know the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not silent — |
||
| _, | ||
|
|
||
| pub fn elementSize(self: TypedarrayType) usize { | ||
| /// Returns null for typedarray types this binding doesn't know about. | ||
| pub fn elementSize(self: TypedarrayType) ?usize { | ||
| switch (self) { | ||
| .int8, .uint8, .uint8_clamped => return 1, | ||
| .int16, .uint16 => return 2, | ||
| .int32, .uint32, .float32 => return 4, | ||
| .float64, .bigint64, .biguint64 => return 8, | ||
| _ => return null, | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -72,10 +79,22 @@ pub const TypedarrayType = enum(c.napi_typedarray_type) { | |
| .float64 => return f64, | ||
| .bigint64 => return i64, | ||
| .biguint64 => return u64, | ||
| _ => @compileError("unknown typedarray type"), | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| test "elementSize returns null for unknown typedarray types" { | ||
| // e.g. Float16Array on newer engines: unknown values must not be UB. | ||
| const unknown: TypedarrayType = @enumFromInt(999); | ||
| try @import("std").testing.expect(unknown.elementSize() == null); | ||
| } | ||
|
|
||
| test "elementSize covers known typedarray types" { | ||
| try @import("std").testing.expectEqual(@as(?usize, 1), TypedarrayType.uint8.elementSize()); | ||
| try @import("std").testing.expectEqual(@as(?usize, 8), TypedarrayType.biguint64.elementSize()); | ||
| } | ||
|
|
||
| /// https://nodejs.org/api/n-api.html#napi_property_attributes | ||
| pub const PropertyAttributes = enum(c.napi_property_attributes) { | ||
| default = c.napi_default, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call — renamed to
bigIntSingleWord(6a6231f). It reads a single-word BigInt and throws otherwise, so "first" was misleading.