From 6fdd2e4b5bca21b1b1f83863947bc57730306b89 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Mon, 25 Sep 2017 11:37:18 -0400 Subject: [PATCH 1/3] n-api: add check for large strings n-api uses size_t for the size of strings when specifying string lengths. V8 only supports a size of int. Add a check so that an error will be returned if the user passes in a string with a size larger than will fit into an int. --- src/node_api.cc | 4 ++++ test/addons-napi/test_string/test.js | 4 ++++ test/addons-napi/test_string/test_string.c | 8 ++++++++ 3 files changed, 16 insertions(+) diff --git a/src/node_api.cc b/src/node_api.cc index fbf20878abd702..ad7b74fe0a04a3 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -10,6 +10,7 @@ #include #include +#include // INT_MAX #include #include #include @@ -125,6 +126,9 @@ struct napi_env__ { do { \ static_assert(static_cast(NAPI_AUTO_LENGTH) == -1, \ "Casting NAPI_AUTO_LENGTH to int must result in -1"); \ + RETURN_STATUS_IF_FALSE((env), \ + (len == NAPI_AUTO_LENGTH) || len <= INT_MAX, \ + napi_generic_failure); \ auto str_maybe = v8::String::NewFromUtf8( \ (env)->isolate, (str), v8::NewStringType::kInternalized, \ static_cast(len)); \ diff --git a/test/addons-napi/test_string/test.js b/test/addons-napi/test_string/test.js index 6e4962d129a6a4..e92d3bf0323eba 100644 --- a/test/addons-napi/test_string/test.js +++ b/test/addons-napi/test_string/test.js @@ -69,3 +69,7 @@ assert.strictEqual(test_string.TestUtf8Insufficient(str6), str6.slice(0, 1)); assert.strictEqual(test_string.TestUtf16Insufficient(str6), str6.slice(0, 3)); assert.strictEqual(test_string.Utf16Length(str6), 5); assert.strictEqual(test_string.Utf8Length(str6), 14); + +assert.throws(() => { + test_string.TestLargeUtf8() +}, /^Error: Unknown failure$/); diff --git a/test/addons-napi/test_string/test_string.c b/test/addons-napi/test_string/test_string.c index 9b06740db69361..279967dbfd959a 100644 --- a/test/addons-napi/test_string/test_string.c +++ b/test/addons-napi/test_string/test_string.c @@ -201,6 +201,13 @@ napi_value Utf8Length(napi_env env, napi_callback_info info) { return output; } +napi_value TestLargeUtf8(napi_env env, napi_callback_info info) { + napi_value output; + NAPI_CALL(env, napi_create_string_utf8(env, "", 4294967296 + 1, &output)); + + return output; +} + napi_value Init(napi_env env, napi_value exports) { napi_property_descriptor properties[] = { DECLARE_NAPI_PROPERTY("TestLatin1", TestLatin1), @@ -211,6 +218,7 @@ napi_value Init(napi_env env, napi_value exports) { DECLARE_NAPI_PROPERTY("TestUtf16Insufficient", TestUtf16Insufficient), DECLARE_NAPI_PROPERTY("Utf16Length", Utf16Length), DECLARE_NAPI_PROPERTY("Utf8Length", Utf8Length), + DECLARE_NAPI_PROPERTY("TestLargeUtf8", TestLargeUtf8), }; NAPI_CALL(env, napi_define_properties( From d319160c7581a19fae7b251b6f30734f9fef39c3 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Tue, 26 Sep 2017 17:29:13 -0400 Subject: [PATCH 2/3] squash: address comments --- src/node_api.cc | 4 ++-- test/addons-napi/test_string/test.js | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/node_api.cc b/src/node_api.cc index ad7b74fe0a04a3..94e4e57bb483d3 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -128,7 +128,7 @@ struct napi_env__ { "Casting NAPI_AUTO_LENGTH to int must result in -1"); \ RETURN_STATUS_IF_FALSE((env), \ (len == NAPI_AUTO_LENGTH) || len <= INT_MAX, \ - napi_generic_failure); \ + napi_invalid_arg); \ auto str_maybe = v8::String::NewFromUtf8( \ (env)->isolate, (str), v8::NewStringType::kInternalized, \ static_cast(len)); \ @@ -870,7 +870,7 @@ void napi_module_register(napi_module* mod) { // Warning: Keep in-sync with napi_status enum const char* error_messages[] = {nullptr, - "Invalid pointer passed as argument", + "Invalid argument", "An object was expected", "A string was expected", "A string or symbol was expected", diff --git a/test/addons-napi/test_string/test.js b/test/addons-napi/test_string/test.js index e92d3bf0323eba..5ce3d739c7a941 100644 --- a/test/addons-napi/test_string/test.js +++ b/test/addons-napi/test_string/test.js @@ -71,5 +71,5 @@ assert.strictEqual(test_string.Utf16Length(str6), 5); assert.strictEqual(test_string.Utf8Length(str6), 14); assert.throws(() => { - test_string.TestLargeUtf8() -}, /^Error: Unknown failure$/); + test_string.TestLargeUtf8(); +}, /^Error: Invalid argument$/); From ff719da924abde9b38372d7e0f8814695578ed99 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Thu, 28 Sep 2017 17:25:44 -0400 Subject: [PATCH 3/3] squash: fix test failures on 32bit platforms --- test/addons-napi/test_string/test_string.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/test/addons-napi/test_string/test_string.c b/test/addons-napi/test_string/test_string.c index 279967dbfd959a..1dd9301a920c30 100644 --- a/test/addons-napi/test_string/test_string.c +++ b/test/addons-napi/test_string/test_string.c @@ -1,3 +1,4 @@ +#include // INT_MAX #include #include "../common.h" @@ -203,7 +204,13 @@ napi_value Utf8Length(napi_env env, napi_callback_info info) { napi_value TestLargeUtf8(napi_env env, napi_callback_info info) { napi_value output; - NAPI_CALL(env, napi_create_string_utf8(env, "", 4294967296 + 1, &output)); + if (SIZE_MAX > INT_MAX) { + NAPI_CALL(env, napi_create_string_utf8(env, "", ((size_t)INT_MAX) + 1, &output)); + } else { + // just throw the expected error as there is nothing to test + // in this case since we can't overflow + NAPI_CALL(env, napi_throw_error(env, NULL, "Invalid argument")); + } return output; }