Fix integer underflow in Chakra napi_get_value_string_* zero-bufsize handling

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();