0

In <napi.h>, there are some overloads declaration for the static method Accessor in the class PropertyDescriptor.

One of such overloads declaration is here:

template <typename Getter>
static PropertyDescriptor Accessor(Napi::Env env,
                                   Napi::Object object,
                                   const std::string& utf8name,
                                   Getter getter,
                                   napi_property_attributes attributes = napi_default,
                                   void* data = nullptr);

That declaration wants a reference to a const std::string

The definition of that declaration is in <napi-inl.h>:

template <typename Getter>
inline PropertyDescriptor PropertyDescriptor::Accessor(Napi::Env env,
                                                       Napi::Object object,
                                                       const std::string& utf8name,
                                                       Getter getter,
                                                       napi_property_attributes attributes,
                                                       void* data) {
  return Accessor(env, object, utf8name.c_str(), getter, attributes, data);
}

As you can see, the implementation takes the .c_str() of the passed std::string, as the implementation in fact uses another overload which wants a const char*.

The overload used is also in <napi-inl.h>:

template <typename Getter>
inline PropertyDescriptor
PropertyDescriptor::Accessor(Napi::Env env,
                             Napi::Object object,
                             const char* utf8name,
                             Getter getter,
                             napi_property_attributes attributes,
                             void* data) {
  using CbData = details::CallbackData<Getter, Napi::Value>;
  auto callbackData = new CbData({ getter, data });

  napi_status status = AttachData(env, object, callbackData);
  if (status != napi_ok) {
    delete callbackData;
    NAPI_THROW_IF_FAILED(env, status, napi_property_descriptor());
  }

  return PropertyDescriptor({
    utf8name,
    nullptr,
    nullptr,
    CbData::Wrapper,
    nullptr,
    nullptr,
    attributes,
    callbackData
  });
}

That implementation uses Node-API, as node-addon-api is just a C++ wrapper arround Node-API. The fact is that the const char* utf8name obtained from .c_str() is passed as is to the constructor and is stored in the private member napi_property_descriptor _desc;

Conclusion: each time you use Napi::PropertyDescriptor::Accessor passing a std::string and then storing the PD in a vector, or just making the string goes out of scope with the PD still here, you trigger an undefined behavior!

Example:

Napi::Object TheNewObject = Napi::Object::New(env);
std::vector<Napi::PropertyDescriptor> vProps;
for (size_t index = 0; index < 2; ++index ) {
    std::string strName("MyName_");
    strName += std::to_string(index);
    auto PD = Napi::PropertyDescriptor::Accessor(env, TheNewObject, strName, Caller);
    vProps.push_back(PD);
} 

I opened an issue here, 12 days ago, with no avail.

EDIT: After reading comments, I should add that:

  1. The napi_property_descriptor is a simple C structure.
  2. Address of that struct will later be passed to the Node-API napi_define_properties
manuell
  • 7,528
  • 5
  • 31
  • 58
  • the code you posted is merely passing around the `const char*`, if there is an issue then elsewhere. Even storing the `const char*` beyond the lifetime of the string is fine, as long as the pointer isnt used – 463035818_is_not_an_ai Feb 01 '22 at 14:53
  • Looks like the argument passed to the `PropertyDescriptor` constructor (in the `return` statement) actually constructs a `napi_property_descriptor` object from the list of arguments, which is then copied to the private member. Do you have the source code for the `napi_property_descriptor` c'tor? Maybe it makes a copy of the `const char*` argument? – Adrian Mole Feb 01 '22 at 14:54
  • What I meant is that the argument is actually a `napi_property_descriptor` object - note the `{...}`. And there's only one c'tor for `PropertyDescriptor`. – Adrian Mole Feb 01 '22 at 14:58
  • Is it expected that `Accessor` makes a copy of the string? If it would, then I'd expect it to take a non-reference parameter. Have you checked the manual whether this use case is supported? In any case, it is not clear to me what your question is. Do you want to know whether your current code has UB? Probably yes, you answered that yourself. Or do you want to know whether this is a bug rather than itended? To me it looks rather intentional. – user17732522 Feb 01 '22 at 15:00
  • 1
    @AdrianMole OP doesn't mention it, but that is just a C struct. – user17732522 Feb 01 '22 at 15:01
  • @463035818_is_not_a_number I don't understand. Property Descriptor are used to manage in the addon the properties of the JavaScript Object you create. of course the pointer is used! – manuell Feb 01 '22 at 15:22
  • i am just saying that it isnt apparent from the code you posted that the dangling pointer is used (not even wether it is stored). If the constructorcopies the string then there is no issue. It isnt possible from the code you posted to judge whether there will be UB. For a bug report I think they also require more – 463035818_is_not_an_ai Feb 01 '22 at 15:24
  • @463035818_is_not_a_number See edit. – manuell Feb 01 '22 at 15:45

1 Answers1

0

There is indeed a problem with current implementations of PropertyDescriptor. See https://github.com/nodejs/node-addon-api/issues/1127#issuecomment-1036394322

Glad I wasn't completely delirious.

manuell
  • 7,528
  • 5
  • 31
  • 58