-
Notifications
You must be signed in to change notification settings - Fork 3
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
base: main
Are you sure you want to change the base?
Changes from all commits
123382a
e118b09
182d1e2
5af7585
1deb61c
57fdbf3
4245470
6a6231f
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 |
|---|---|---|
|
|
@@ -100,13 +100,12 @@ pub fn doubleBigInt(n: BigInt) !BigInt { | |
| return BigInt.from(val * 2); | ||
| } | ||
|
|
||
| /// Read a BigInt's first u64 word via `getValueBigintWords` passing `null` for `sign_bit`. | ||
| /// Read a single-word BigInt as a u64 via `getValueBigintWords` passing `null` for `sign_bit`. | ||
| /// | ||
| /// Throws if word_count > 1. | ||
| pub fn bigIntFirstWord(n: BigInt) !Number { | ||
| /// Throws `Overflow` if the BigInt needs more than one word. | ||
| pub fn bigIntSingleWord(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,16 @@ pub fn typeTag(comptime T: type) napi.c.napi_type_tag { | |
| }; | ||
| } | ||
|
|
||
| /// After a successful wrap, N-API owns `native_object` and frees it via the | ||
| /// finalizer when the JS object is GC'd. On a later error the caller frees it | ||
| /// instead, so the finalizer is detached first — it must not also fire, or the | ||
| /// object is freed twice. | ||
| 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 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 {}; | ||
|
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 |
||
| if (!(try env.checkObjectTypeTag(object, tag))) { | ||
| try env.typeTagObject(object, tag); | ||
| } | ||
|
|
@@ -68,9 +72,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 |
||
| }; | ||
|
|
||
| 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.