From 68f9e1fa9c2b8ad62a927ce8d08d3fc71b6a46b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20Martign=C3=A8ne?= Date: Mon, 15 Apr 2024 19:18:37 +0200 Subject: [PATCH] node-api: copy external type tags when they are set In order to adapt to V8 changes regarding storing private properties on Externals, ExternalWrapper objects were introduced in #51149. However, this new code stores the type tag pointer and not the 128-bit value inside. This breaks some pre-existing code that were making temporary tags. It also means that unloading the module will cause existing External objects to have a tag pointer that points nowhere (use-after-free bug). Change ExternalWrapper to store tags by value to fix this regression. PR-URL: https://github.com/nodejs/node/pull/52426 Reviewed-By: Gabriel Schulhof Reviewed-By: Chengzhong Wu Reviewed-By: Michael Dawson --- src/js_native_api_v8.cc | 13 +++++---- test/js-native-api/test_object/test_object.c | 29 ++++++++++++++++++-- 2 files changed, 33 insertions(+), 9 deletions(-) diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 3c5dc70330cd7f..2a5bb8f5bc5ae2 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -858,23 +858,24 @@ class ExternalWrapper { void* Data() { return data_; } bool TypeTag(const napi_type_tag* type_tag) { - if (type_tag_ != nullptr) { + if (has_tag_) { return false; } - type_tag_ = type_tag; + type_tag_ = *type_tag; + has_tag_ = true; return true; } bool CheckTypeTag(const napi_type_tag* type_tag) { - return type_tag == type_tag_ || - (type_tag_ && type_tag->lower == type_tag_->lower && - type_tag->upper == type_tag_->upper); + return has_tag_ && type_tag->lower == type_tag_.lower && + type_tag->upper == type_tag_.upper; } private: v8impl::Persistent persistent_; void* data_; - const napi_type_tag* type_tag_ = nullptr; + napi_type_tag type_tag_; + bool has_tag_ = false; }; } // end of namespace v8impl diff --git a/test/js-native-api/test_object/test_object.c b/test/js-native-api/test_object/test_object.c index eddf805187507e..b9b3e5c90ae906 100644 --- a/test/js-native-api/test_object/test_object.c +++ b/test/js-native-api/test_object/test_object.c @@ -629,12 +629,29 @@ TypeTaggedInstance(napi_env env, napi_callback_info info) { size_t argc = 1; uint32_t type_index; napi_value instance, which_type; + napi_type_tag tag; + + // Below we copy the tag before setting it to prevent bugs where a pointer + // to the tag (instead of the 128-bit tag value) is stored. NODE_API_CALL(env, napi_get_cb_info(env, info, &argc, &which_type, NULL, NULL)); NODE_API_CALL(env, napi_get_value_uint32(env, which_type, &type_index)); VALIDATE_TYPE_INDEX(env, type_index); NODE_API_CALL(env, napi_create_object(env, &instance)); - NODE_API_CALL(env, napi_type_tag_object(env, instance, &type_tags[type_index])); + tag = type_tags[type_index]; + NODE_API_CALL(env, napi_type_tag_object(env, instance, &tag)); + + // Since the tag passed to napi_type_tag_object() was copied to the stack, + // a type tagging implementation that uses a pointer instead of the + // tag value would end up pointing to stack memory. + // When CheckTypeTag() is called later on, it might be the case that this + // stack address has been left untouched by accident (if no subsequent + // function call has clobbered it), which means the pointer would still + // point to valid data. + // To make sure that tags are stored by value and not by reference, + // clear this copy; any implementation using a pointer would end up with + // random stack data or { 0, 0 }, but not the original tag value, and fail. + memset(&tag, 0, sizeof(tag)); return instance; } @@ -655,6 +672,10 @@ static napi_value TypeTaggedExternal(napi_env env, napi_callback_info info) { size_t argc = 1; uint32_t type_index; napi_value instance, which_type; + napi_type_tag tag; + + // See TypeTaggedInstance() for an explanation about why we copy the tag + // to the stack and why we call memset on it after the external is tagged. NODE_API_CALL(env, napi_get_cb_info(env, info, &argc, &which_type, NULL, NULL)); @@ -662,8 +683,10 @@ static napi_value TypeTaggedExternal(napi_env env, napi_callback_info info) { VALIDATE_TYPE_INDEX(env, type_index); NODE_API_CALL( env, napi_create_external(env, IN_LIEU_OF_NULL, NULL, NULL, &instance)); - NODE_API_CALL(env, - napi_type_tag_object(env, instance, &type_tags[type_index])); + tag = type_tags[type_index]; + NODE_API_CALL(env, napi_type_tag_object(env, instance, &tag)); + + memset(&tag, 0, sizeof(tag)); return instance; }