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 nodejs#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: nodejs#52426
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>

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<v8::Value> persistent_;
   void* data_;
-  const napi_type_tag* type_tag_ = nullptr;
+  napi_type_tag type_tag_;
+  bool has_tag_ = false;
 };
 
 }  // end of namespace v8impl

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,15 +672,21 @@ 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));
   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_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;
 }