Skip to content

Commit

Permalink
src: fix missing void*data usage in PropertyDescriptors
Browse files Browse the repository at this point in the history 
PR-URL: nodejs/node-addon-api#374
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
  • Loading branch information
John French committed Dec 28, 2018
1 parent ce0b327 commit 48c5004
Show file tree
Hide file tree
Showing 4 changed files with 50 additions and 10 deletions.
4 changes: 2 additions & 2 deletions napi-inl.deprecated.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ inline PropertyDescriptor PropertyDescriptor::Accessor(const char* utf8name,
void* /*data*/) {
typedef details::AccessorCallbackData<Getter, Setter> CbData;
// TODO: Delete when the function is destroyed
auto callbackData = new CbData({ getter, setter });
auto callbackData = new CbData({ getter, setter, nullptr });

return PropertyDescriptor({
utf8name,
Expand Down Expand Up @@ -104,7 +104,7 @@ inline PropertyDescriptor PropertyDescriptor::Accessor(napi_value name,
void* /*data*/) {
typedef details::AccessorCallbackData<Getter, Setter> CbData;
// TODO: Delete when the function is destroyed
auto callbackData = new CbData({ getter, setter });
auto callbackData = new CbData({ getter, setter, nullptr });

return PropertyDescriptor({
nullptr,
Expand Down
19 changes: 11 additions & 8 deletions napi-inl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ struct AccessorCallbackData {
CallbackInfo callbackInfo(env, info);
AccessorCallbackData* callbackData =
static_cast<AccessorCallbackData*>(callbackInfo.Data());
callbackInfo.SetData(callbackData->data);
return callbackData->getterCallback(callbackInfo);
});
}
Expand All @@ -186,13 +187,15 @@ struct AccessorCallbackData {
CallbackInfo callbackInfo(env, info);
AccessorCallbackData* callbackData =
static_cast<AccessorCallbackData*>(callbackInfo.Data());
callbackInfo.SetData(callbackData->data);
callbackData->setterCallback(callbackInfo);
return nullptr;
});
}

Getter getterCallback;
Setter setterCallback;
void* data;
};

} // namespace details
Expand Down Expand Up @@ -2603,9 +2606,9 @@ PropertyDescriptor::Accessor(Napi::Env env,
const char* utf8name,
Getter getter,
napi_property_attributes attributes,
void* /*data*/) {
void* data) {
typedef details::CallbackData<Getter, Napi::Value> CbData;
auto callbackData = new CbData({ getter, nullptr });
auto callbackData = new CbData({ getter, data });

napi_status status = AttachData(env, object, callbackData);
NAPI_THROW_IF_FAILED(env, status, napi_property_descriptor());
Expand Down Expand Up @@ -2638,9 +2641,9 @@ inline PropertyDescriptor PropertyDescriptor::Accessor(Napi::Env env,
Name name,
Getter getter,
napi_property_attributes attributes,
void* /*data*/) {
void* data) {
typedef details::CallbackData<Getter, Napi::Value> CbData;
auto callbackData = new CbData({ getter, nullptr });
auto callbackData = new CbData({ getter, data });

napi_status status = AttachData(env, object, callbackData);
NAPI_THROW_IF_FAILED(env, status, napi_property_descriptor());
Expand All @@ -2664,9 +2667,9 @@ inline PropertyDescriptor PropertyDescriptor::Accessor(Napi::Env env,
Getter getter,
Setter setter,
napi_property_attributes attributes,
void* /*data*/) {
void* data) {
typedef details::AccessorCallbackData<Getter, Setter> CbData;
auto callbackData = new CbData({ getter, setter });
auto callbackData = new CbData({ getter, setter, data });

napi_status status = AttachData(env, object, callbackData);
NAPI_THROW_IF_FAILED(env, status, napi_property_descriptor());
Expand Down Expand Up @@ -2701,9 +2704,9 @@ inline PropertyDescriptor PropertyDescriptor::Accessor(Napi::Env env,
Getter getter,
Setter setter,
napi_property_attributes attributes,
void* /*data*/) {
void* data) {
typedef details::AccessorCallbackData<Getter, Setter> CbData;
auto callbackData = new CbData({ getter, setter });
auto callbackData = new CbData({ getter, setter, data });

napi_status status = AttachData(env, object, callbackData);
NAPI_THROW_IF_FAILED(env, status, napi_property_descriptor());
Expand Down
26 changes: 26 additions & 0 deletions test/object/object.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ Value HasPropertyWithCStyleString(const CallbackInfo& info);
Value HasPropertyWithCppStyleString(const CallbackInfo& info);

static bool testValue = true;
// Used to test void* Data() integrity
struct UserDataHolder {
int32_t value;
};

Value TestGetter(const CallbackInfo& info) {
return Boolean::New(info.Env(), testValue);
Expand All @@ -42,6 +46,16 @@ void TestSetter(const CallbackInfo& info) {
testValue = info[0].As<Boolean>();
}

Value TestGetterWithUserData(const CallbackInfo& info) {
const UserDataHolder* holder = reinterpret_cast<UserDataHolder*>(info.Data());
return Number::New(info.Env(), holder->value);
}

void TestSetterWithUserData(const CallbackInfo& info) {
UserDataHolder* holder = reinterpret_cast<UserDataHolder*>(info.Data());
holder->value = info[0].As<Number>().Int32Value();
}

Value TestFunction(const CallbackInfo& info) {
return Boolean::New(info.Env(), true);
}
Expand All @@ -58,11 +72,15 @@ void DefineProperties(const CallbackInfo& info) {
Env env = info.Env();

Boolean trueValue = Boolean::New(env, true);
UserDataHolder* holder = new UserDataHolder();
holder->value = 1234;

if (nameType.Utf8Value() == "literal") {
obj.DefineProperties({
PropertyDescriptor::Accessor(env, obj, "readonlyAccessor", TestGetter),
PropertyDescriptor::Accessor(env, obj, "readwriteAccessor", TestGetter, TestSetter),
PropertyDescriptor::Accessor(env, obj, "readonlyAccessorWithUserData", TestGetterWithUserData, napi_property_attributes::napi_default, reinterpret_cast<void*>(holder)),
PropertyDescriptor::Accessor(env, obj, "readwriteAccessorWithUserData", TestGetterWithUserData, TestSetterWithUserData, napi_property_attributes::napi_default, reinterpret_cast<void*>(holder)),
PropertyDescriptor::Value("readonlyValue", trueValue),
PropertyDescriptor::Value("readwriteValue", trueValue, napi_writable),
PropertyDescriptor::Value("enumerableValue", trueValue, napi_enumerable),
Expand All @@ -76,6 +94,8 @@ void DefineProperties(const CallbackInfo& info) {
// work around the issue.
std::string str1("readonlyAccessor");
std::string str2("readwriteAccessor");
std::string str1a("readonlyAccessorWithUserData");
std::string str2a("readwriteAccessorWithUserData");
std::string str3("readonlyValue");
std::string str4("readwriteValue");
std::string str5("enumerableValue");
Expand All @@ -85,6 +105,8 @@ void DefineProperties(const CallbackInfo& info) {
obj.DefineProperties({
PropertyDescriptor::Accessor(env, obj, str1, TestGetter),
PropertyDescriptor::Accessor(env, obj, str2, TestGetter, TestSetter),
PropertyDescriptor::Accessor(env, obj, str1a, TestGetterWithUserData, napi_property_attributes::napi_default, reinterpret_cast<void*>(holder)),
PropertyDescriptor::Accessor(env, obj, str2a, TestGetterWithUserData, TestSetterWithUserData, napi_property_attributes::napi_default, reinterpret_cast<void*>(holder)),
PropertyDescriptor::Value(str3, trueValue),
PropertyDescriptor::Value(str4, trueValue, napi_writable),
PropertyDescriptor::Value(str5, trueValue, napi_enumerable),
Expand All @@ -97,6 +119,10 @@ void DefineProperties(const CallbackInfo& info) {
Napi::String::New(env, "readonlyAccessor"), TestGetter),
PropertyDescriptor::Accessor(env, obj,
Napi::String::New(env, "readwriteAccessor"), TestGetter, TestSetter),
PropertyDescriptor::Accessor(env, obj,
Napi::String::New(env, "readonlyAccessorWithUserData"), TestGetterWithUserData, napi_property_attributes::napi_default, reinterpret_cast<void*>(holder)),
PropertyDescriptor::Accessor(env, obj,
Napi::String::New(env, "readwriteAccessorWithUserData"), TestGetterWithUserData, TestSetterWithUserData, napi_property_attributes::napi_default, reinterpret_cast<void*>(holder)),
PropertyDescriptor::Value(
Napi::String::New(env, "readonlyValue"), trueValue),
PropertyDescriptor::Value(
Expand Down
11 changes: 11 additions & 0 deletions test/object/object.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,24 @@ function test(binding) {
assertPropertyIsNot(obj, 'readonlyAccessor', 'configurable');
assert.strictEqual(obj.readonlyAccessor, true);

assertPropertyIsNot(obj, 'readonlyAccessorWithUserData', 'enumerable');
assertPropertyIsNot(obj, 'readonlyAccessorWithUserData', 'configurable');
assert.strictEqual(obj.readonlyAccessorWithUserData, 1234, nameType);

assertPropertyIsNot(obj, 'readwriteAccessor', 'enumerable');
assertPropertyIsNot(obj, 'readwriteAccessor', 'configurable');
obj.readwriteAccessor = false;
assert.strictEqual(obj.readwriteAccessor, false);
obj.readwriteAccessor = true;
assert.strictEqual(obj.readwriteAccessor, true);

assertPropertyIsNot(obj, 'readwriteAccessorWithUserData', 'enumerable');
assertPropertyIsNot(obj, 'readwriteAccessorWithUserData', 'configurable');
obj.readwriteAccessorWithUserData = 2;
assert.strictEqual(obj.readwriteAccessorWithUserData, 2);
obj.readwriteAccessorWithUserData = -14;
assert.strictEqual(obj.readwriteAccessorWithUserData, -14);

assertPropertyIsNot(obj, 'readonlyValue', 'writable');
assertPropertyIsNot(obj, 'readonlyValue', 'enumerable');
assertPropertyIsNot(obj, 'readonlyValue', 'configurable');
Expand Down

0 comments on commit 48c5004

Please sign in to comment.