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/2] 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/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/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)); + } +}