diff --git a/Core/Node-API/Source/js_native_api_chakra.cc b/Core/Node-API/Source/js_native_api_chakra.cc index 53ef2af9..297be2aa 100644 --- a/Core/Node-API/Source/js_native_api_chakra.cc +++ b/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; diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index 86f52b17..2c9e8e80 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/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(buf), bufsize, result); diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index f4fa3c52..a920fa1f 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/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 zeroSafe; + std::promise 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(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(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();