diff --git a/src/NodeApi/Runtime/NodeEmbedding.cs b/src/NodeApi/Runtime/NodeEmbedding.cs index 7c9b74d1..5224f9ba 100644 --- a/src/NodeApi/Runtime/NodeEmbedding.cs +++ b/src/NodeApi/Runtime/NodeEmbedding.cs @@ -433,7 +433,19 @@ 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. + // 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!; @@ -448,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/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/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; } } } diff --git a/test/NodejsEmbeddingTests.cs b/test/NodejsEmbeddingTests.cs index 6e4191c8..fafe5ee0 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; @@ -260,6 +261,183 @@ 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); + }); + } + + private static NodeEmbeddingThreadRuntime CreateRuntimeWithLinkedModule( + string mainScript = "", + NodeEmbedding.InitializeModuleCallback? onInitialize = null) + { + NodeEmbeddingModuleInfo linkedModule = new() + { + Name = "test-linked", + NodeApiVersion = NodeEmbedding.NodeApiVersion, + 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 + // 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 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() { 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)); + } +} 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);