Skip to content

PooledBuffer.FromStringUtf8/FromSpanUtf8 never write the NUL terminator → garbage in JS error messages and linked module names on .NET #508

Closed

Description

@r-Larch

PooledBuffer.FromStringUtf8 and PooledBuffer.FromSpanUtf8 (src/NodeApi/Runtime/PooledBuffer.cs) reserve byteLength + 1 bytes but never write the terminating \0:

PooledBuffer buffer = new(byteLength, byteLength + 1);              // reserves the terminator byte...
Encoding.UTF8.GetBytes(value, 0, value!.Length, buffer.Buffer, 0); // ...but never writes it

On .NET the array comes from ArrayPool<byte>.Shared.Rent, which is not zeroed. (On .NET Framework / netstandard it is a fresh new byte[], so the bug only shows on .NET.) Several call sites pass the pointer to Node-API functions that take a NUL-terminated const char* without a length:

Managed API Native function Argument
JSError.ThrowError / ThrowTypeError / ThrowRangeError / ThrowSyntaxError (string overloads) napi_throw_error, napi_throw_type_error, napi_throw_range_error, node_api_throw_syntax_error code and msg
NodeEmbeddingRuntimeSettings.Modules node_embedding_runtime_config_add_module module_name
exceptions in the embedding callback adapters (TaskRunCallbackAdapter, …) node_embedding_last_error_message_set message

Once the pool is warm, native code reads stale bytes after the string, or past the end of the array if it contains no zero byte (out-of-bounds read).

Call sites that pass an explicit length (CreateFunction, DefineClass, GetSymbolFor, FatalError) are not affected.

What we saw

  • In production (0.9.21, net10.0): a module registered via NodeEmbeddingRuntimeSettings.Modules as gaia was linked under a corrupted name, and process._linkedBinding('gaia') failed with No such binding was linked: gaia. It happened intermittently, only after the app had been running for a while.
  • JSError.ThrowError("x") from a .NET callback → in JS, e.message is "x" followed by 15 garbage bytes (x���������������).

Repro

Make it deterministic by dirtying the shared pool on the JS thread first:

static void FillArrayPoolWithGarbage()
{
    List<byte[]> rented = new();
    for (int size = 1; size <= 1024; size *= 2)
        for (int i = 0; i < 8; i++)
        {
            byte[] array = ArrayPool<byte>.Shared.Rent(size);
            array.AsSpan().Fill(0xAA);
            rented.Add(array);
        }
    foreach (byte[] array in rented) ArrayPool<byte>.Shared.Return(array);
}

nodejs.Run(() =>
{
    JSValue.Global.SetProperty("throwError", JSValue.CreateFunction("throwError", (args) =>
    {
        FillArrayPoolWithGarbage();
        JSError.ThrowError((string)args[0]);
        return JSValue.Undefined;
    }));

    string message = (string)JSValue.RunScript(
        "(() => { try { throwError('x'); } catch (e) { return e.message; } })()");
    // Expected "x"; actual "x" + garbage.
});

Related

Suggested fix

Write the terminator explicitly in both helpers (buffer.Buffer[byteLength] = 0). Encode "" as a 1-byte buffer holding only the terminator, and keep null → null pointer (used for the optional code). Add using in EmbeddingRuntimeConfigAddModule; Node.js copies the name into a std::string, so disposing right after the call is safe. I have a PR ready.

Versions: reproduced on main (54d2b74) and in 0.9.21; the code is unchanged in 0.9.28. Windows x64, net10.0, Microsoft.JavaScript.LibNode 20.1800.203.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions