Skip to content

Commit

Permalink
node-api: copy external type tags when they are set
Browse files Browse the repository at this point in the history
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: #52426
Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
  • Loading branch information
Koromix authored and marco-ippolito committed May 3, 2024
1 parent b4d7032 commit 0a225a4
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 9 deletions.
13 changes: 7 additions & 6 deletions src/js_native_api_v8.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
29 changes: 26 additions & 3 deletions test/js-native-api/test_object/test_object.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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;
}
Expand Down

0 comments on commit 0a225a4

Please sign in to comment.