Fix integer underflow in Chakra napi_get_value_string_* zero-bufsize handling (#197)

## Summary

Fixes an integer underflow in the Chakra Node-API string getters when a
caller passes a non-null buffer with `bufsize == 0`.

`napi_get_value_string_utf16` forwarded `bufsize - 1` to
`JsCopyStringUtf16` as the destination capacity and stored the null
terminator at `buf[bufsize - 1]`. With `bufsize == 0`, `bufsize - 1`
underflows to `SIZE_MAX`, so:
- the **entire** JS string is copied into the zero-length buffer, and
- the terminator is written at `buf[SIZE_MAX]`.

That's an attacker-sized out-of-bounds write reachable from any native
caller that passes `(buf != nullptr, bufsize == 0)` (CWE-191 →
CWE-787/CWE-120).

## Fix

- `napi_get_value_string_utf16`: gate the copy on `bufsize != 0`
(mirroring the upstream Node implementation). A non-null buffer with
`bufsize == 0` now reports zero copied and writes nothing.
- `napi_get_value_string_latin1` / `napi_get_value_string_utf8`: these
shared a related, milder bug — with `bufsize == 0` they took the slow
path (a needless allocation) and stored the terminator at `buf[0]`, one
byte past a zero-length buffer. Both now short-circuit `bufsize == 0`
identically.

## Test

Adds a native regression test
(`NodeApi.GetValueStringUtf16HandlesZeroBufsize`) — the path isn't
reachable from JS, so it's exercised through the C API. It passes a
sentinel-filled buffer with `bufsize == 0` and asserts nothing is
written and the reported length is zero, while a normally-sized buffer
still copies and null-terminates. Guarded off the V8JSI backend (which
doesn't expose `napi_get_value_string_utf16`), same as the
`napi_create_dataview` regression test from #181.

**Verified (Win32/Chakra):** builds clean; the new test and the full
UnitTests suite pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: bkaradzic-microsoft

Core/Node-API/Source/js_native_api_chakra.cc

@@ -1405,6 +1405,14 @@ napi_status napi_get_value_string_latin1(napi_env env,
     CHECK_JSRT_EXPECTED(env,
       JsCopyString(jsValue, nullptr, 0, result, CP_LATIN1),
       napi_string_expected);
+  } else if (bufsize == 0) {
+    // A non-null buffer with bufsize == 0 has no room for the null terminator;
+    // report zero and write nothing. The slow path below would otherwise run a
+    // needless allocation and store the terminator at buf[0], one byte past the
+    // zero-length buffer.
+    if (result != nullptr) {
+      *result = 0;
+    }
   } else {
     size_t count = 0;
     CHECK_JSRT_EXPECTED(env,
@@ -1489,6 +1497,14 @@ napi_status napi_get_value_string_utf8(napi_env env,
     CHECK_JSRT_EXPECTED(env,
       JsCopyString(jsValue, nullptr, 0, result),
       napi_string_expected);
+  } else if (bufsize == 0) {
+    // A non-null buffer with bufsize == 0 has no room for the null terminator;
+    // report zero and write nothing. The slow path below would otherwise run a
+    // needless allocation and store the terminator at buf[0], one byte past the
+    // zero-length buffer.
+    if (result != nullptr) {
+      *result = 0;
+    }
   } else {
     size_t count = 0;
     CHECK_JSRT_EXPECTED(env,
@@ -1574,7 +1590,7 @@ napi_status napi_get_value_string_utf16(napi_env env,
     CHECK_JSRT_EXPECTED(env,
       JsCopyStringUtf16(jsValue, nullptr, 0, result),
       napi_string_expected);
-  } else {
+  } else if (bufsize != 0) {
     size_t copied = 0;
     CHECK_JSRT_EXPECTED(env,
       JsCopyStringUtf16(
@@ -1593,6 +1609,12 @@ napi_status napi_get_value_string_utf16(napi_env env,
     if (result != nullptr) {
       *result = copied;
     }
+  } else if (result != nullptr) {
+    // A non-null buffer with bufsize == 0 has no room for any character or the
+    // null terminator. Report zero copied and write nothing, instead of letting
+    // bufsize - 1 underflow to SIZE_MAX (which would copy the whole string into
+    // the zero-length buffer and store the terminator at buf[SIZE_MAX]).
+    *result = 0;
   }
 
   return napi_ok;

Core/Node-API/Source/js_native_api_javascriptcore.cc

@@ -1815,6 +1815,12 @@ napi_status napi_get_value_string_latin1(napi_env env,
 
   if (buf == nullptr) {
     *result = string.LengthLatin1();
+  } else if (bufsize == 0) {
+    // No room for any character or the null terminator; report zero and write
+    // nothing rather than letting the copy helper underflow bufsize - 1.
+    if (result != nullptr) {
+      *result = 0;
+    }
   } else {
     string.CopyToLatin1(buf, bufsize, result);
   }
@@ -1844,6 +1850,12 @@ napi_status napi_get_value_string_utf8(napi_env env,
 
   if (buf == nullptr) {
     *result = string.LengthUTF8();
+  } else if (bufsize == 0) {
+    // No room for any byte or the null terminator; report zero and write
+    // nothing rather than letting the copy helper underflow bufsize - 1.
+    if (result != nullptr) {
+      *result = 0;
+    }
   } else {
     string.CopyToUTF8(buf, bufsize, result);
   }
@@ -1873,6 +1885,12 @@ napi_status napi_get_value_string_utf16(napi_env env,
 
   if (buf == nullptr) {
     *result = string.Length();
+  } else if (bufsize == 0) {
+    // No room for any code unit or the null terminator; report zero and write
+    // nothing rather than letting the copy helper underflow bufsize - 1.
+    if (result != nullptr) {
+      *result = 0;
+    }
   } else {
     static_assert(sizeof(char16_t) == sizeof(JSChar));
     string.CopyTo(reinterpret_cast<JSChar*>(buf), bufsize, result);

Tests/UnitTests/Shared/Shared.cpp

@@ -351,6 +351,58 @@ TEST(NodeApi, CreateDataViewRejectsOverflowingRange)
 }
 #endif
 
+// The V8JSI Node-API shim does not expose napi_get_value_string_utf16, so this
+// native test only builds on the Chakra, V8, and JavaScriptCore backends.
+#if !defined(JSRUNTIMEHOST_NAPI_ENGINE_JSI)
+TEST(NodeApi, GetValueStringUtf16HandlesZeroBufsize)
+{
+    // Regression: napi_get_value_string_utf16 with a non-null buffer and
+    // bufsize == 0 must not evaluate bufsize - 1. On the Chakra backend the
+    // pre-fix code forwarded bufsize - 1 (== SIZE_MAX) to JsCopyStringUtf16 as
+    // the destination capacity, copying the entire JS string into the
+    // zero-length buffer, and then stored the terminator at buf[bufsize - 1]
+    // (== buf[SIZE_MAX]). The call must instead write nothing and report zero.
+    Babylon::AppRuntime runtime{};
+
+    std::promise<bool> zeroSafe;
+    std::promise<bool> normalWorks;
+
+    runtime.Dispatch([&zeroSafe, &normalWorks](Napi::Env env) {
+        napi_env nenv{env};
+
+        napi_value strValue{Napi::String::New(env, "hello world")};
+
+        // Sentinel-filled buffer. With bufsize == 0 nothing may be written, so
+        // every element must survive unchanged (a SIZE_MAX-capacity copy would
+        // clobber it / overflow).
+        char16_t guard[8];
+        for (auto& c : guard)
+        {
+            c = static_cast<char16_t>(0x7FFF);
+        }
+
+        size_t copied{0xDEAD};
+        napi_status status{napi_get_value_string_utf16(nenv, strValue, guard, 0, &copied)};
+
+        bool safe{status == napi_ok && copied == 0};
+        for (auto c : guard)
+        {
+            safe = safe && (c == static_cast<char16_t>(0x7FFF));
+        }
+        zeroSafe.set_value(safe);
+
+        // A sufficiently-sized buffer must still copy and null-terminate.
+        char16_t buf[32];
+        size_t copied2{0};
+        napi_status status2{napi_get_value_string_utf16(nenv, strValue, buf, 32, &copied2)};
+        normalWorks.set_value(status2 == napi_ok && copied2 == 11 && buf[copied2] == 0);
+    });
+
+    EXPECT_TRUE(zeroSafe.get_future().get());
+    EXPECT_TRUE(normalWorks.get_future().get());
+}
+#endif
+
 int RunTests()
 {
     testing::InitGoogleTest();