From 4c2df00c41848288740b720ffd34922d96b4dc5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Larch?= Date: Wed, 21 Oct 2026 21:01:00 +0200 Subject: [PATCH 1/4] Write explicit null terminators in Utf8StringArray The string buffer is rented from ArrayPool.Shared on .NET, which is not zeroed, so the byte reserved for each terminator could hold stale data and native code read argv entries past their end. Same fix as #482 (fixes #481); needed here because the new tests dirty the shared pool before the Node.js platform is created. Co-Authored-By: Claude Opus 5.5 --- src/NodeApi/Runtime/Utf8StringArray.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/NodeApi/Runtime/Utf8StringArray.cs b/src/NodeApi/Runtime/Utf8StringArray.cs index f3b7b36c..04d0ad60 100644 --- a/src/NodeApi/Runtime/Utf8StringArray.cs +++ b/src/NodeApi/Runtime/Utf8StringArray.cs @@ -47,8 +47,10 @@ public unsafe Utf8StringArray(ReadOnlySpan strings) { Utf8Strings[i] = stringBufferPtr + offset; offset += Encoding.UTF8.GetBytes( - src, strings[i].Length, (byte*)(stringBufferPtr + offset), byteLength - offset) - + 1; // +1 for the string Null-terminator. + src, strings[i].Length, (byte*)(stringBufferPtr + offset), byteLength - offset); + + // Pooled arrays are not zeroed. + _stringBuffer[offset++] = 0; } } } From 19f7529b36e8c112905dd751a3167f8c79a01be0 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 2/4] 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/TestUtils.cs | 29 +++++++ test/Utf8BufferTests.cs | 76 +++++++++++++++++++ 5 files changed, 147 insertions(+), 5 deletions(-) create mode 100644 test/Utf8BufferTests.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/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(); diff --git a/test/Utf8BufferTests.cs b/test/Utf8BufferTests.cs new file mode 100644 index 00000000..5be9636d --- /dev/null +++ b/test/Utf8BufferTests.cs @@ -0,0 +1,76 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System; +using System.Runtime.InteropServices; +using System.Text; +using Microsoft.JavaScript.NodeApi.Runtime; +using Xunit; +using static Microsoft.JavaScript.NodeApi.Test.TestUtils; + +namespace Microsoft.JavaScript.NodeApi.Test; + +public class Utf8BufferTests +{ + 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); + } + +#if !NETFRAMEWORK + [Fact] + public void Utf8StringArrayIsNullTerminated() + { + string[] strings = ["node", "", "--expose-gc", "\u00e4\u00f6\u00fc \U0001F600"]; + + FillArrayPoolWithGarbage(); + using Utf8StringArray utf8Strings = new(strings); + + for (int i = 0; i < strings.Length; i++) + { + Assert.Equal(strings[i], Marshal.PtrToStringUTF8(utf8Strings.Utf8Strings[i])); + } + } +#endif + + 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)); + } +} From e598b31ef69b6c710dbe0504030552546cb07373 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 3/4] 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 824d31be195bbf64fca423ed8345534037c2e0ad 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 4/4] 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() {