From 21ae41c035b68d655185957fb7b24d1f283943bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Larch?= Date: Wed, 21 Oct 2026 21:01:01 +0200 Subject: [PATCH 1/3] Null-terminate PooledBuffer UTF-8 strings PooledBuffer.FromStringUtf8 and FromSpanUtf8 reserved a byte for the terminator but never wrote it. On .NET the array comes from ArrayPool.Shared, which is not zeroed, so native functions that take a null-terminated const char* without a length read stale bytes past the string (or past the array): napi_throw_error / napi_throw_type_error / napi_throw_range_error / node_api_throw_syntax_error (code and msg), node_embedding_runtime_config_add_module and node_embedding_last_error_message_set. For example JSError.ThrowError("x") produced a JS error message of "x" followed by garbage, and a linked module could be registered under a corrupted name. - Write the terminator explicitly in both helpers. - Encode an empty string as just the terminator instead of returning PooledBuffer.Empty, which pinned to a null pointer (ThrowError("") passed msg = NULL, so napi_throw_error failed and nothing was thrown). A null string still maps to a null pointer. - Return the module-name buffer to the pool in EmbeddingRuntimeConfigAddModule (Node.js copies the name). Fixes #508. Co-Authored-By: Claude Opus 5.5 --- .../Runtime/NodejsRuntime.Embedding.cs | 2 +- src/NodeApi/Runtime/PooledBuffer.cs | 17 ++++-- test/NodejsEmbeddingTests.cs | 28 +++++++++ test/PooledBufferTests.cs | 61 +++++++++++++++++++ test/TestUtils.cs | 29 +++++++++ 5 files changed, 132 insertions(+), 5 deletions(-) create mode 100644 test/PooledBufferTests.cs diff --git a/src/NodeApi/Runtime/NodejsRuntime.Embedding.cs b/src/NodeApi/Runtime/NodejsRuntime.Embedding.cs index 4c3c5da7..4ff15168 100644 --- a/src/NodeApi/Runtime/NodejsRuntime.Embedding.cs +++ b/src/NodeApi/Runtime/NodejsRuntime.Embedding.cs @@ -681,7 +681,7 @@ public override NodeEmbeddingStatus EmbeddingRuntimeConfigAddModule( node_embedding_data_release_callback release_init_module_data, int module_node_api_version) { - PooledBuffer moduleNameBuffer = PooledBuffer.FromSpanUtf8(module_name); + using PooledBuffer moduleNameBuffer = PooledBuffer.FromSpanUtf8(module_name); fixed (byte* moduleNamePtr = moduleNameBuffer) return Import(ref node_embedding_runtime_config_add_module)( runtime_config, diff --git a/src/NodeApi/Runtime/PooledBuffer.cs b/src/NodeApi/Runtime/PooledBuffer.cs index 597f1235..0e8328ed 100644 --- a/src/NodeApi/Runtime/PooledBuffer.cs +++ b/src/NodeApi/Runtime/PooledBuffer.cs @@ -68,17 +68,21 @@ public void Dispose() [EditorBrowsable(EditorBrowsableState.Never)] public readonly ref byte GetPinnableReference() => ref Span.GetPinnableReference(); + // Native callers may read these buffers as null-terminated C strings. A null string + // produces an empty buffer (null pointer); an empty string produces just the terminator. public static unsafe PooledBuffer FromStringUtf8(string? value) { - if (string.IsNullOrEmpty(value)) + if (value == null) { return Empty; } int byteLength = Encoding.UTF8.GetByteCount(value); PooledBuffer buffer = new(byteLength, byteLength + 1); - Encoding.UTF8.GetBytes(value, 0, value!.Length, buffer.Buffer, 0); + Encoding.UTF8.GetBytes(value, 0, value.Length, buffer.Buffer, 0); + // Pooled arrays are not zeroed. + buffer.Buffer[byteLength] = 0; return buffer; } @@ -86,7 +90,9 @@ public static unsafe PooledBuffer FromSpanUtf8(ReadOnlySpan value) { if (value.IsEmpty) { - return Empty; + PooledBuffer emptyString = new(0, 1); + emptyString.Buffer[0] = 0; + return emptyString; } fixed (char* valuePtr = value) @@ -94,7 +100,10 @@ public static unsafe PooledBuffer FromSpanUtf8(ReadOnlySpan value) int byteLength = Encoding.UTF8.GetByteCount(valuePtr, value.Length); PooledBuffer buffer = new(byteLength, byteLength + 1); fixed (byte* bufferPtr = buffer.Span) - Encoding.UTF8.GetBytes(valuePtr, value.Length, bufferPtr, byteLength + 1); + Encoding.UTF8.GetBytes(valuePtr, value.Length, bufferPtr, byteLength); + + // Pooled arrays are not zeroed. + buffer.Buffer[byteLength] = 0; return buffer; } } diff --git a/test/NodejsEmbeddingTests.cs b/test/NodejsEmbeddingTests.cs index 6e4191c8..1785decc 100644 --- a/test/NodejsEmbeddingTests.cs +++ b/test/NodejsEmbeddingTests.cs @@ -260,6 +260,34 @@ public void ErrorPropagation() $"at {typeof(NodejsEmbeddingTests).FullName}.", StringComparison.Ordinal)); } + [Theory] + [InlineData("x")] + [InlineData("")] + public void ThrowErrorFromCallback(string message) + { + using NodeEmbeddingThreadRuntime nodejs = CreateNodeEmbeddingThreadRuntime(); + + nodejs.Run(() => + { + JSValue.Global.SetProperty("throwError", JSValue.CreateFunction("throwError", (args) => + { + // Rented buffers that are not zeroed must not leak into the error message. + FillArrayPoolWithGarbage(); + JSError.ThrowError((string)args[0]); + return JSValue.Undefined; + })); + JSValue.Global.SetProperty("message", message); + + JSValue result = JSValue.RunScript(""" + (() => { + try { throwError(message); } catch (e) { return e.message; } + return 'not thrown'; + })() + """); + Assert.Equal(message, (string)result); + }); + } + [Fact] public async Task WorkerIsMainThread() { diff --git a/test/PooledBufferTests.cs b/test/PooledBufferTests.cs new file mode 100644 index 00000000..900eaeb2 --- /dev/null +++ b/test/PooledBufferTests.cs @@ -0,0 +1,61 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +#pragma warning disable CA1822 // Mark members as static + +using System; +using System.Text; +using Microsoft.JavaScript.NodeApi.Runtime; +using Xunit; +using static Microsoft.JavaScript.NodeApi.Test.TestUtils; + +namespace Microsoft.JavaScript.NodeApi.Test; + +public class PooledBufferTests +{ + public static TheoryData Strings { get; } = new() + { + "", + "x", + "module-name", + "\u00e4\u00f6\u00fc \u4e2d\u6587 \U0001F600", + }; + + [Theory] + [MemberData(nameof(Strings))] + public void FromStringUtf8IsNullTerminated(string value) + { + FillArrayPoolWithGarbage(); + using PooledBuffer buffer = PooledBuffer.FromStringUtf8(value); + AssertNullTerminatedUtf8(value, buffer); + } + + [Theory] + [MemberData(nameof(Strings))] + public void FromSpanUtf8IsNullTerminated(string value) + { + FillArrayPoolWithGarbage(); + using PooledBuffer buffer = PooledBuffer.FromSpanUtf8(value.AsSpan()); + AssertNullTerminatedUtf8(value, buffer); + } + + [Fact] + public void FromStringUtf8NullIsEmpty() + { + // A null string must still produce a null pointer when pinned. + using PooledBuffer buffer = PooledBuffer.FromStringUtf8(null); + Assert.Equal(0, buffer.Length); + Assert.True(buffer.Span.IsEmpty); + } + + private static void AssertNullTerminatedUtf8(string expected, PooledBuffer buffer) + { + int byteLength = Encoding.UTF8.GetByteCount(expected); + Assert.Equal(byteLength, buffer.Length); + + // The buffer must hold the terminator so that pinning it never yields a null pointer. + Assert.True(buffer.Span.Length > byteLength); + Assert.Equal(0, buffer.Span[byteLength]); + Assert.Equal(expected, Encoding.UTF8.GetString(buffer.Buffer, 0, byteLength)); + } +} diff --git a/test/TestUtils.cs b/test/TestUtils.cs index b52c09cf..3ca96420 100644 --- a/test/TestUtils.cs +++ b/test/TestUtils.cs @@ -2,6 +2,10 @@ // Licensed under the MIT License. using System; +#if !NETFRAMEWORK +using System.Buffers; +#endif +using System.Collections.Generic; using System.Diagnostics; using System.IO; using System.Runtime.InteropServices; @@ -142,6 +146,31 @@ public static void CopyIfNewer(string sourceFilePath, string targetFilePath) } } + /// + /// Fills arrays in the shared byte array pool (as seen by the calling thread) with non-zero + /// bytes, so that code relying on rented buffers being zeroed fails deterministically. + /// + public static void FillArrayPoolWithGarbage() + { +#if !NETFRAMEWORK + List rented = new(); + for (int size = 1; size <= 1024; size *= 2) + { + for (int i = 0; i < 8; i++) + { + byte[] array = ArrayPool.Shared.Rent(size); + array.AsSpan().Fill(0xAA); + rented.Add(array); + } + } + + foreach (byte[] array in rented) + { + ArrayPool.Shared.Return(array); + } +#endif + } + public static Task RunInThread(Action action) { TaskCompletionSource threadCompletion = new(); From ab9d812ecc1a10bb237e4fbcaf138a4d0baa39e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Larch?= Date: Wed, 21 Oct 2026 21:06:48 +0200 Subject: [PATCH 2/3] Initialize linked modules within the current JS value scope Calling process._linkedBinding() for a module registered through NodeEmbeddingRuntimeSettings.Modules failed or crashed the process (#464): - From a script run via NodeEmbeddingThreadRuntime.Run: "A Root scope cannot be created within another scope." - From MainScript: the nested root scope dropped its parent on dispose, so the thread was left without a current scope ("There is no active JS value scope", reported as a stack overflow in the issue). - From an ES module imported via ImportAsync: native assertion "(v8_scope_data_->nest_level()) == (nest_level)" in CloseV8Scope. ModuleInitializeCallbackAdapter always opened a new root scope for the napi_env that Node.js creates for each linked module. Modules are initialized lazily, while the runtime's scope is normally current on the JS thread, so open a callback scope within it instead and only fall back to a root scope when no scope is current. The module's exports then share the runtime's context, and later calls from JS into the exported .NET callbacks find a matching parent scope. Fixes #464. Co-Authored-By: Claude Opus 5.5 --- src/NodeApi/Runtime/NodeEmbedding.cs | 9 ++- test/NodejsEmbeddingTests.cs | 97 ++++++++++++++++++++++++++++ test/test-linked-module.mjs | 2 + 3 files changed, 107 insertions(+), 1 deletion(-) create mode 100644 test/test-linked-module.mjs diff --git a/src/NodeApi/Runtime/NodeEmbedding.cs b/src/NodeApi/Runtime/NodeEmbedding.cs index 7c9b74d1..fb0e06df 100644 --- a/src/NodeApi/Runtime/NodeEmbedding.cs +++ b/src/NodeApi/Runtime/NodeEmbedding.cs @@ -433,7 +433,14 @@ internal static unsafe napi_value ModuleInitializeCallbackAdapter( nint module_name, napi_value exports) { - using var jsValueScope = new JSValueScope(JSValueScopeType.Root, env, JSRuntime); + // Linked modules are initialized lazily by process._linkedBinding(), so the runtime's + // scope is usually current on this thread. Node.js passes a separate napi_env for each + // module; nesting a root scope for it would fail or, on dispose, leave the thread + // without a current scope. Initialize the module within the runtime's scope instead, + // so that its exports share the runtime context and later callbacks into them work. + using var jsValueScope = JSValueScope.CurrentOrNull != null + ? new JSValueScope(JSValueScopeType.Callback) + : new JSValueScope(JSValueScopeType.Root, env, JSRuntime); try { var callback = (InitializeModuleCallback)GCHandle.FromIntPtr(cb_data).Target!; diff --git a/test/NodejsEmbeddingTests.cs b/test/NodejsEmbeddingTests.cs index 1785decc..62c0077c 100644 --- a/test/NodejsEmbeddingTests.cs +++ b/test/NodejsEmbeddingTests.cs @@ -4,6 +4,7 @@ #pragma warning disable CA1822 // Mark members as static using System; +using System.Collections.Generic; using System.IO; using System.Linq; using System.Text; @@ -288,6 +289,102 @@ public void ThrowErrorFromCallback(string message) }); } + private static NodeEmbeddingThreadRuntime CreateRuntimeWithLinkedModule( + string mainScript = "") + { + NodeEmbeddingModuleInfo linkedModule = new() + { + Name = "test-linked", + NodeApiVersion = NodeEmbedding.NodeApiVersion, + OnInitialize = (runtime, moduleName, exports) => + { + exports.SetProperty("moduleName", moduleName); + exports.SetProperty("add", JSValue.CreateFunction( + "add", (args) => (int)args[0] + (int)args[1])); + return exports; + }, + }; + + // Modules are registered on the runtime thread while it is configured, so dirty the + // array pool lazily from there, right before the module name is marshalled. + static IEnumerable WithGarbageInArrayPool( + NodeEmbeddingModuleInfo module) + { + FillArrayPoolWithGarbage(); + yield return module; + } + + return NodejsPlatform.CreateThreadRuntime( + Path.Combine(GetRepoRootDirectory(), "test"), + new NodeEmbeddingRuntimeSettings + { + MainScript = MainScript + mainScript, + Modules = WithGarbageInArrayPool(linkedModule), + }); + } + + private static void AssertJSThreadIsUsable(NodeEmbeddingThreadRuntime nodejs) + { + nodejs.Run(() => + { + Assert.Equal(3, (int)JSValue.RunScript("1 + 2")); + }); + } + + [Fact] + public void LinkedModuleFromScript() + { + using NodeEmbeddingThreadRuntime nodejs = CreateRuntimeWithLinkedModule(); + + nodejs.Run(() => + { + JSValue linked = JSValue.RunScript("process._linkedBinding('test-linked')"); + Assert.Equal("test-linked", (string)linked["moduleName"]); + Assert.Equal(3, (int)linked.CallMethod("add", 1, 2)); + + // The scope that was current before the module was initialized must be restored. + Assert.Equal(3, (int)JSValue.RunScript("1 + 2")); + }); + + AssertJSThreadIsUsable(nodejs); + nodejs.Dispose(); + Assert.Equal(0, nodejs.ExitCode); + } + + [Fact] + public void LinkedModuleFromMainScript() + { + using NodeEmbeddingThreadRuntime nodejs = CreateRuntimeWithLinkedModule( + "globalThis.linked = process._linkedBinding('test-linked');\n"); + + nodejs.Run(() => + { + Assert.Equal(3, (int)JSValue.RunScript("globalThis.linked.add(1, 2)")); + }); + + AssertJSThreadIsUsable(nodejs); + nodejs.Dispose(); + Assert.Equal(0, nodejs.ExitCode); + } + + [Fact] + public async Task LinkedModuleFromESModule() + { + using NodeEmbeddingThreadRuntime nodejs = CreateRuntimeWithLinkedModule(); + + int result = await nodejs.RunAsync(async () => + { + JSValue testModule = await nodejs.ImportAsync( + "./test-linked-module.mjs", null, esModule: true); + return (int)testModule["result"]; + }); + Assert.Equal(3, result); + + AssertJSThreadIsUsable(nodejs); + nodejs.Dispose(); + Assert.Equal(0, nodejs.ExitCode); + } + [Fact] public async Task WorkerIsMainThread() { diff --git a/test/test-linked-module.mjs b/test/test-linked-module.mjs new file mode 100644 index 00000000..0d0e31e2 --- /dev/null +++ b/test/test-linked-module.mjs @@ -0,0 +1,2 @@ +const { add } = process._linkedBinding('test-linked'); +export const result = add(1, 2); From 0e133ff4f62065b19300bdb1fc94e48bf004fd5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Larch?= Date: Wed, 21 Oct 2026 22:28:44 +0200 Subject: [PATCH 3/3] Report linked module init errors to the module's env Addresses review feedback on #512: - Node-API tracks pending exceptions per napi_env, and Node.js only reports those of the module's env to the caller of process._linkedBinding(). Errors thrown by the initializer within the runtime's scope were only logged as "Uncaught N-API callback exception" (DEP0168). Move a pending exception from the scope's env to the module's env before returning. - A callback scope cannot be nested in a handle or escapable scope, so initializing a module while one is current threw outside the adapter's try/catch. Use the current scope as is in that case. Co-Authored-By: Claude Opus 5.5 --- src/NodeApi/Runtime/NodeEmbedding.cs | 24 +++++++++-- test/NodejsEmbeddingTests.cs | 59 ++++++++++++++++++++++++++-- 2 files changed, 77 insertions(+), 6 deletions(-) diff --git a/src/NodeApi/Runtime/NodeEmbedding.cs b/src/NodeApi/Runtime/NodeEmbedding.cs index fb0e06df..5224f9ba 100644 --- a/src/NodeApi/Runtime/NodeEmbedding.cs +++ b/src/NodeApi/Runtime/NodeEmbedding.cs @@ -438,9 +438,14 @@ internal static unsafe napi_value ModuleInitializeCallbackAdapter( // module; nesting a root scope for it would fail or, on dispose, leave the thread // without a current scope. Initialize the module within the runtime's scope instead, // so that its exports share the runtime context and later callbacks into them work. - using var jsValueScope = JSValueScope.CurrentOrNull != null - ? new JSValueScope(JSValueScopeType.Callback) - : new JSValueScope(JSValueScopeType.Root, env, JSRuntime); + // A callback scope cannot be nested in a handle scope, so that is used as is. + using JSValueScope? jsValueScope = JSValueScope.CurrentOrNull?.ScopeType switch + { + null => new JSValueScope(JSValueScopeType.Root, env, JSRuntime), + JSValueScopeType.Handle or JSValueScopeType.Escapable => null, + _ => new JSValueScope(JSValueScopeType.Callback), + }; + napi_env scopeEnv = JSValueScope.Current.UncheckedEnvironmentHandle; try { var callback = (InitializeModuleCallback)GCHandle.FromIntPtr(cb_data).Target!; @@ -455,6 +460,19 @@ internal static unsafe napi_value ModuleInitializeCallbackAdapter( JSError.ThrowError(ex); return napi_value.Null; } + finally + { + // Pending exceptions are tracked per napi_env, and Node.js only reports those of the + // module's env to the caller of process._linkedBinding(). + if (scopeEnv != env && + JSRuntime.IsExceptionPending(scopeEnv, out bool isPending) == napi_status.napi_ok && + isPending && + JSRuntime.GetAndClearLastException(scopeEnv, out napi_value error) == + napi_status.napi_ok) + { + JSRuntime.Throw(env, error); + } + } } #if UNMANAGED_DELEGATES diff --git a/test/NodejsEmbeddingTests.cs b/test/NodejsEmbeddingTests.cs index 62c0077c..fafe5ee0 100644 --- a/test/NodejsEmbeddingTests.cs +++ b/test/NodejsEmbeddingTests.cs @@ -290,19 +290,20 @@ public void ThrowErrorFromCallback(string message) } private static NodeEmbeddingThreadRuntime CreateRuntimeWithLinkedModule( - string mainScript = "") + string mainScript = "", + NodeEmbedding.InitializeModuleCallback? onInitialize = null) { NodeEmbeddingModuleInfo linkedModule = new() { Name = "test-linked", NodeApiVersion = NodeEmbedding.NodeApiVersion, - OnInitialize = (runtime, moduleName, exports) => + OnInitialize = onInitialize ?? ((runtime, moduleName, exports) => { exports.SetProperty("moduleName", moduleName); exports.SetProperty("add", JSValue.CreateFunction( "add", (args) => (int)args[0] + (int)args[1])); return exports; - }, + }), }; // Modules are registered on the runtime thread while it is configured, so dirty the @@ -385,6 +386,58 @@ public async Task LinkedModuleFromESModule() Assert.Equal(0, nodejs.ExitCode); } + [Fact] + public void LinkedModuleFromHandleScope() + { + using NodeEmbeddingThreadRuntime nodejs = CreateRuntimeWithLinkedModule(); + + nodejs.Run(() => + { + using JSValueScope handleScope = new(); + Assert.Equal( + "test-linked", + (string)JSValue.RunScript("process._linkedBinding('test-linked').moduleName")); + }); + + AssertJSThreadIsUsable(nodejs); + nodejs.Dispose(); + Assert.Equal(0, nodejs.ExitCode); + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public void LinkedModuleInitializationError(bool throwJSError) + { + using NodeEmbeddingThreadRuntime nodejs = CreateRuntimeWithLinkedModule( + onInitialize: (runtime, moduleName, exports) => + { + if (throwJSError) + { + JSError.ThrowError("init failed"); + return exports; + } + + throw new InvalidOperationException("init failed"); + }); + + nodejs.Run(() => + { + // The error must reach the caller of process._linkedBinding(). + JSValue result = JSValue.RunScript(""" + (() => { + try { process._linkedBinding('test-linked'); } catch (e) { return e.message; } + return 'not thrown'; + })() + """); + Assert.Equal("init failed", (string)result); + }); + + AssertJSThreadIsUsable(nodejs); + nodejs.Dispose(); + Assert.Equal(0, nodejs.ExitCode); + } + [Fact] public async Task WorkerIsMainThread() {