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/2] 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 21fcb80c16ec3a702d80420b3488b79173e6002f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Larch?= Date: Wed, 21 Oct 2026 21:03:28 +0200 Subject: [PATCH 2/2] Throw the named error type from JSError.Throw*Error(string) JSError.ThrowTypeError, ThrowRangeError and ThrowSyntaxError (with and without a code) all called JSRuntime.ThrowError, so JS received a plain Error instead of a TypeError / RangeError / SyntaxError. Call the matching runtime method instead. Fixes #509. Co-Authored-By: Claude Opus 5.5 --- src/NodeApi/JSError.cs | 12 ++++---- test/NodejsEmbeddingTests.cs | 55 ++++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 6 deletions(-) diff --git a/src/NodeApi/JSError.cs b/src/NodeApi/JSError.cs index 33437ae9..74cf119f 100644 --- a/src/NodeApi/JSError.cs +++ b/src/NodeApi/JSError.cs @@ -290,22 +290,22 @@ public static void ThrowError(string code, string message) => JSValue.GetCurrentRuntime(out napi_env env).ThrowError(env, code, message); public static void ThrowTypeError(string message) - => JSValue.GetCurrentRuntime(out napi_env env).ThrowError(env, code: null, message); + => JSValue.GetCurrentRuntime(out napi_env env).ThrowTypeError(env, code: null, message); public static void ThrowTypeError(string code, string message) - => JSValue.GetCurrentRuntime(out napi_env env).ThrowError(env, code, message); + => JSValue.GetCurrentRuntime(out napi_env env).ThrowTypeError(env, code, message); public static void ThrowRangeError(string message) - => JSValue.GetCurrentRuntime(out napi_env env).ThrowError(env, code: null, message); + => JSValue.GetCurrentRuntime(out napi_env env).ThrowRangeError(env, code: null, message); public static void ThrowRangeError(string code, string message) - => JSValue.GetCurrentRuntime(out napi_env env).ThrowError(env, code, message); + => JSValue.GetCurrentRuntime(out napi_env env).ThrowRangeError(env, code, message); public static void ThrowSyntaxError(string message) - => JSValue.GetCurrentRuntime(out napi_env env).ThrowError(env, code: null, message); + => JSValue.GetCurrentRuntime(out napi_env env).ThrowSyntaxError(env, code: null, message); public static void ThrowSyntaxError(string code, string message) - => JSValue.GetCurrentRuntime(out napi_env env).ThrowError(env, code, message); + => JSValue.GetCurrentRuntime(out napi_env env).ThrowSyntaxError(env, code, message); /// /// Gets a JS error stack trace that also includes a .NET stack trace, diff --git a/test/NodejsEmbeddingTests.cs b/test/NodejsEmbeddingTests.cs index 1785decc..cc5fbc78 100644 --- a/test/NodejsEmbeddingTests.cs +++ b/test/NodejsEmbeddingTests.cs @@ -288,6 +288,61 @@ public void ThrowErrorFromCallback(string message) }); } + [Theory] + [InlineData("Error", null)] + [InlineData("Error", "ERR_TEST")] + [InlineData("TypeError", null)] + [InlineData("TypeError", "ERR_TEST")] + [InlineData("RangeError", null)] + [InlineData("RangeError", "ERR_TEST")] + [InlineData("SyntaxError", null)] + [InlineData("SyntaxError", "ERR_TEST")] + public void ThrowTypedError(string errorType, string? code) + { + using NodeEmbeddingThreadRuntime nodejs = CreateNodeEmbeddingThreadRuntime(); + + nodejs.Run(() => + { + JSValue.Global.SetProperty("throwError", JSValue.CreateFunction("throwError", (_) => + { + Action throwWithoutCode = errorType switch + { + "Error" => JSError.ThrowError, + "TypeError" => JSError.ThrowTypeError, + "RangeError" => JSError.ThrowRangeError, + "SyntaxError" => JSError.ThrowSyntaxError, + _ => throw new ArgumentOutOfRangeException(nameof(errorType)), + }; + Action throwWithCode = errorType switch + { + "Error" => JSError.ThrowError, + "TypeError" => JSError.ThrowTypeError, + "RangeError" => JSError.ThrowRangeError, + "SyntaxError" => JSError.ThrowSyntaxError, + _ => throw new ArgumentOutOfRangeException(nameof(errorType)), + }; + + if (code == null) throwWithoutCode("test"); + else throwWithCode(code, "test"); + return JSValue.Undefined; + })); + + JSValue error = JSValue.RunScript(""" + (() => { + try { throwError(); } catch (e) { return e; } + return null; + })() + """); + Assert.True(error.IsObject()); + Assert.Equal(errorType, (string)error["name"]); + Assert.True((bool)JSValue.RunScript( + $"(e) => e instanceof {errorType}").Call(JSValue.Undefined, error)); + Assert.Equal("test", (string)error["message"]); + if (code == null) Assert.True(error["code"].IsUndefined()); + else Assert.Equal(code, (string)error["code"]); + }); + } + [Fact] public async Task WorkerIsMainThread() {