Valgrind detected memory leak #891

lygstate · 2021-01-29T08:55:35Z

==6515== 
==6515== 20 bytes in 1 blocks are definitely lost in loss record 1 of 7
==6515==    at 0x4834CAB: operator new(unsigned int) (vg_replace_malloc.c:328)
==6515==    by 0x257D1D: Napi::InstanceWrap<node_sqlite3::Database>::InstanceAccessor(char const*, Napi::Value (node_sqlite3::Database::*)(Napi::CallbackInfo const&), void (node_sqlite3::Database::*)(Napi::CallbackInfo const&, Napi::Value const&), napi_property_attributes, void*) (napi-inl.h:3417)
==6515==    by 0x25034D: node_sqlite3::Database::Init(Napi::Env, Napi::Object) (database.cc:22)
==6515==    by 0x1798B1: (anonymous namespace)::RegisterModule(Napi::Env, Napi::Object) (node_sqlite3.cc:21)
==6515==    by 0x17A9E8: Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object))::{lambda()#1}::operator()() const (napi-inl.h:375)
==6515==    by 0x17C4D0: napi_value__* Napi::details::WrapCallback<Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object))::{lambda()#1}>(Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object))::{lambda()#1}) (napi-inl.h:73)
==6515==    by 0x17AA37: Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object)) (napi-inl.h:374)
==6515==    by 0x17A90A: Init(napi_env__*, napi_value__*) (node_sqlite3.cc:118)
==6515==    by 0x1738C6: napi_module_init (node_api_module.c:48)
==6515==    by 0x173A05: napi_module_init_static (node_api_module.c:89)
==6515==    by 0x17A955: iotjs_sqlite3_napi_init (node_sqlite3.cc:121)
==6515==    by 0x1697AA: iotjs_module_get (iotjs_module.c:43)
==6515== 
==6515== 20 bytes in 1 blocks are definitely lost in loss record 2 of 7
==6515==    at 0x4834CAB: operator new(unsigned int) (vg_replace_malloc.c:328)
==6515==    by 0x24A08F: Napi::InstanceWrap<node_sqlite3::Backup>::InstanceAccessor(char const*, Napi::Value (node_sqlite3::Backup::*)(Napi::CallbackInfo const&), void (node_sqlite3::Backup::*)(Napi::CallbackInfo const&, Napi::Value const&), napi_property_attributes, void*) (napi-inl.h:3417)
==6515==    by 0x24615A: node_sqlite3::Backup::Init(Napi::Env, Napi::Object) (backup.cc:16)
==6515==    by 0x179901: (anonymous namespace)::RegisterModule(Napi::Env, Napi::Object) (node_sqlite3.cc:23)
==6515==    by 0x17A9E8: Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object))::{lambda()#1}::operator()() const (napi-inl.h:375)
==6515==    by 0x17C4D0: napi_value__* Napi::details::WrapCallback<Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object))::{lambda()#1}>(Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object))::{lambda()#1}) (napi-inl.h:73)
==6515==    by 0x17AA37: Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object)) (napi-inl.h:374)
==6515==    by 0x17A90A: Init(napi_env__*, napi_value__*) (node_sqlite3.cc:118)
==6515==    by 0x1738C6: napi_module_init (node_api_module.c:48)
==6515==    by 0x173A05: napi_module_init_static (node_api_module.c:89)
==6515==    by 0x17A955: iotjs_sqlite3_napi_init (node_sqlite3.cc:121)
==6515==    by 0x1697AA: iotjs_module_get (iotjs_module.c:43)
==6515== 
==6515== 20 bytes in 1 blocks are definitely lost in loss record 3 of 7
==6515==    at 0x4834CAB: operator new(unsigned int) (vg_replace_malloc.c:328)
==6515==    by 0x24A08F: Napi::InstanceWrap<node_sqlite3::Backup>::InstanceAccessor(char const*, Napi::Value (node_sqlite3::Backup::*)(Napi::CallbackInfo const&), void (node_sqlite3::Backup::*)(Napi::CallbackInfo const&, Napi::Value const&), napi_property_attributes, void*) (napi-inl.h:3417)
==6515==    by 0x2461D7: node_sqlite3::Backup::Init(Napi::Env, Napi::Object) (backup.cc:17)
==6515==    by 0x179901: (anonymous namespace)::RegisterModule(Napi::Env, Napi::Object) (node_sqlite3.cc:23)
==6515==    by 0x17A9E8: Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object))::{lambda()#1}::operator()() const (napi-inl.h:375)
==6515==    by 0x17C4D0: napi_value__* Napi::details::WrapCallback<Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object))::{lambda()#1}>(Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object))::{lambda()#1}) (napi-inl.h:73)
==6515==    by 0x17AA37: Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object)) (napi-inl.h:374)
==6515==    by 0x17A90A: Init(napi_env__*, napi_value__*) (node_sqlite3.cc:118)
==6515==    by 0x1738C6: napi_module_init (node_api_module.c:48)
==6515==    by 0x173A05: napi_module_init_static (node_api_module.c:89)
==6515==    by 0x17A955: iotjs_sqlite3_napi_init (node_sqlite3.cc:121)
==6515==    by 0x1697AA: iotjs_module_get (iotjs_module.c:43)
==6515== 
==6515== 20 bytes in 1 blocks are definitely lost in loss record 4 of 7
==6515==    at 0x4834CAB: operator new(unsigned int) (vg_replace_malloc.c:328)
==6515==    by 0x24A08F: Napi::InstanceWrap<node_sqlite3::Backup>::InstanceAccessor(char const*, Napi::Value (node_sqlite3::Backup::*)(Napi::CallbackInfo const&), void (node_sqlite3::Backup::*)(Napi::CallbackInfo const&, Napi::Value const&), napi_property_attributes, void*) (napi-inl.h:3417)
==6515==    by 0x246254: node_sqlite3::Backup::Init(Napi::Env, Napi::Object) (backup.cc:18)
==6515==    by 0x179901: (anonymous namespace)::RegisterModule(Napi::Env, Napi::Object) (node_sqlite3.cc:23)
==6515==    by 0x17A9E8: Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object))::{lambda()#1}::operator()() const (napi-inl.h:375)
==6515==    by 0x17C4D0: napi_value__* Napi::details::WrapCallback<Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object))::{lambda()#1}>(Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object))::{lambda()#1}) (napi-inl.h:73)
==6515==    by 0x17AA37: Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object)) (napi-inl.h:374)
==6515==    by 0x17A90A: Init(napi_env__*, napi_value__*) (node_sqlite3.cc:118)
==6515==    by 0x1738C6: napi_module_init (node_api_module.c:48)
==6515==    by 0x173A05: napi_module_init_static (node_api_module.c:89)
==6515==    by 0x17A955: iotjs_sqlite3_napi_init (node_sqlite3.cc:121)
==6515==    by 0x1697AA: iotjs_module_get (iotjs_module.c:43)
==6515== 
==6515== 20 bytes in 1 blocks are definitely lost in loss record 5 of 7
==6515==    at 0x4834CAB: operator new(unsigned int) (vg_replace_malloc.c:328)
==6515==    by 0x24A08F: Napi::InstanceWrap<node_sqlite3::Backup>::InstanceAccessor(char const*, Napi::Value (node_sqlite3::Backup::*)(Napi::CallbackInfo const&), void (node_sqlite3::Backup::*)(Napi::CallbackInfo const&, Napi::Value const&), napi_property_attributes, void*) (napi-inl.h:3417)
==6515==    by 0x2462CE: node_sqlite3::Backup::Init(Napi::Env, Napi::Object) (backup.cc:19)
==6515==    by 0x179901: (anonymous namespace)::RegisterModule(Napi::Env, Napi::Object) (node_sqlite3.cc:23)
==6515==    by 0x17A9E8: Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object))::{lambda()#1}::operator()() const (napi-inl.h:375)
==6515==    by 0x17C4D0: napi_value__* Napi::details::WrapCallback<Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object))::{lambda()#1}>(Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object))::{lambda()#1}) (napi-inl.h:73)
==6515==    by 0x17AA37: Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object)) (napi-inl.h:374)
==6515==    by 0x17A90A: Init(napi_env__*, napi_value__*) (node_sqlite3.cc:118)
==6515==    by 0x1738C6: napi_module_init (node_api_module.c:48)
==6515==    by 0x173A05: napi_module_init_static (node_api_module.c:89)
==6515==    by 0x17A955: iotjs_sqlite3_napi_init (node_sqlite3.cc:121)
==6515==    by 0x1697AA: iotjs_module_get (iotjs_module.c:43)
==6515== 
==6515== 20 bytes in 1 blocks are definitely lost in loss record 6 of 7
==6515==    at 0x4834CAB: operator new(unsigned int) (vg_replace_malloc.c:328)
==6515==    by 0x24A08F: Napi::InstanceWrap<node_sqlite3::Backup>::InstanceAccessor(char const*, Napi::Value (node_sqlite3::Backup::*)(Napi::CallbackInfo const&), void (node_sqlite3::Backup::*)(Napi::CallbackInfo const&, Napi::Value const&), napi_property_attributes, void*) (napi-inl.h:3417)
==6515==    by 0x246348: node_sqlite3::Backup::Init(Napi::Env, Napi::Object) (backup.cc:20)
==6515==    by 0x179901: (anonymous namespace)::RegisterModule(Napi::Env, Napi::Object) (node_sqlite3.cc:23)
==6515==    by 0x17A9E8: Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object))::{lambda()#1}::operator()() const (napi-inl.h:375)
==6515==    by 0x17C4D0: napi_value__* Napi::details::WrapCallback<Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object))::{lambda()#1}>(Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object))::{lambda()#1}) (napi-inl.h:73)
==6515==    by 0x17AA37: Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object)) (napi-inl.h:374)
==6515==    by 0x17A90A: Init(napi_env__*, napi_value__*) (node_sqlite3.cc:118)
==6515==    by 0x1738C6: napi_module_init (node_api_module.c:48)
==6515==    by 0x173A05: napi_module_init_static (node_api_module.c:89)
==6515==    by 0x17A955: iotjs_sqlite3_napi_init (node_sqlite3.cc:121)
==6515==    by 0x1697AA: iotjs_module_get (iotjs_module.c:43)
==6515== 
==6515== 20 bytes in 1 blocks are definitely lost in loss record 7 of 7
==6515==    at 0x4834CAB: operator new(unsigned int) (vg_replace_malloc.c:328)
==6515==    by 0x24A08F: Napi::InstanceWrap<node_sqlite3::Backup>::InstanceAccessor(char const*, Napi::Value (node_sqlite3::Backup::*)(Napi::CallbackInfo const&), void (node_sqlite3::Backup::*)(Napi::CallbackInfo const&, Napi::Value const&), napi_property_attributes, void*) (napi-inl.h:3417)
==6515==    by 0x2463C4: node_sqlite3::Backup::Init(Napi::Env, Napi::Object) (backup.cc:21)
==6515==    by 0x179901: (anonymous namespace)::RegisterModule(Napi::Env, Napi::Object) (node_sqlite3.cc:23)
==6515==    by 0x17A9E8: Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object))::{lambda()#1}::operator()() const (napi-inl.h:375)
==6515==    by 0x17C4D0: napi_value__* Napi::details::WrapCallback<Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object))::{lambda()#1}>(Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object))::{lambda()#1}) (napi-inl.h:73)
==6515==    by 0x17AA37: Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object)) (napi-inl.h:374)
==6515==    by 0x17A90A: Init(napi_env__*, napi_value__*) (node_sqlite3.cc:118)
==6515==    by 0x1738C6: napi_module_init (node_api_module.c:48)
==6515==    by 0x173A05: napi_module_init_static (node_api_module.c:89)
==6515==    by 0x17A955: iotjs_sqlite3_napi_init (node_sqlite3.cc:121)
==6515==    by 0x1697AA: iotjs_module_get (iotjs_module.c:43)
==6515==

How to delete the following new created data?

  InstanceAccessorCallbackData* callbackData =
    new InstanceAccessorCallbackData({ getter, setter, data });

Or node_api.h needs expost destructor function for data?


template <typename T>
inline ClassPropertyDescriptor<T> InstanceWrap<T>::InstanceAccessor(
    const char* utf8name,
    InstanceGetterCallback getter,
    InstanceSetterCallback setter,
    napi_property_attributes attributes,
    void* data) {
  InstanceAccessorCallbackData* callbackData =
    new InstanceAccessorCallbackData({ getter, setter, data });

  napi_property_descriptor desc = napi_property_descriptor();
  desc.utf8name = utf8name;
  desc.getter = getter != nullptr ? T::InstanceGetterCallbackWrapper : nullptr;
  desc.setter = setter != nullptr ? T::InstanceSetterCallbackWrapper : nullptr;
  desc.data = callbackData;
  desc.attributes = attributes;
  return desc;
}

template <typename T>
inline ClassPropertyDescriptor<T> InstanceWrap<T>::InstanceAccessor(
    Symbol name,
    InstanceGetterCallback getter,
    InstanceSetterCallback setter,
    napi_property_attributes attributes,
    void* data) {
  InstanceAccessorCallbackData* callbackData =
    new InstanceAccessorCallbackData({ getter, setter, data });

  napi_property_descriptor desc = napi_property_descriptor();
  desc.name = name;
  desc.getter = getter != nullptr ? T::InstanceGetterCallbackWrapper : nullptr;
  desc.setter = setter != nullptr ? T::InstanceSetterCallbackWrapper : nullptr;
  desc.data = callbackData;
  desc.attributes = attributes;
  return desc;
}

The text was updated successfully, but these errors were encountered:

gabrielschulhof · 2021-01-29T16:43:00Z

@lygstate do you by any chance have a piece of code that can help us reproduce the valgrind result?

Fixes: nodejs#891

KevinEady · 2021-02-03T21:28:30Z

Hi team,

So I was not able to get the valgrind error, however I replicated the memory leak by throwing a console message in a destructor on InstanceAccessorCallbackData:

 ~AccessorCallbackData() {
  std::cout<<"destructed!\n";
}

And using this simple instance accessor example, the message did NOT show:
addon.cpp:

#include <iostream>
#include <functional>
#include <vector>
#include "napi.h"

class Window : public Napi::ObjectWrap<Window> {
    public:
        static Napi::Function Init(Napi::Env env) {
            Napi::Function ctor = DefineClass(env, "Window", {
                ObjectWrap::InstanceAccessor("buffer", &Window::getBuffer, nullptr),
            });
            return ctor;
        }

        Window(const Napi::CallbackInfo& info) : Napi::ObjectWrap<Window>(info) { };

    private:
        Napi::Value getBuffer(const Napi::CallbackInfo& info) {
            return Napi::String::New(info.Env(), "buffer");
        }
};

Napi::Object Init(Napi::Env env, Napi::Object exports) {
    exports["Window"] = Window::Init(env);
    return exports;
}

NODE_API_MODULE(addon, Init)

addon.js:

const x = new Window("hello");
console.log(x.buffer);

When executing:

root@b36ee5d3b4df:/app# node .
buffer

I believe the issue is in AttachPropData here:

node-addon-api/napi-inl.h

Line 3266 in 286ae21

if (prop->method != nullptr && !(prop->attributes & napi_static)) {

with the check for prop->method != nullptr , because instance get/set properties do not have this member set:

node-addon-api/napi-inl.h

Lines 3409 to 3426 in 286ae21

    
           template <typename T> 
        
           inline ClassPropertyDescriptor<T> InstanceWrap<T>::InstanceAccessor( 
        
               const char* utf8name, 
        
               InstanceGetterCallback getter, 
        
               InstanceSetterCallback setter, 
        
               napi_property_attributes attributes, 
        
               void* data) { 
        
             InstanceAccessorCallbackData* callbackData = 
        
               new InstanceAccessorCallbackData({ getter, setter, data }); 
        
             napi_property_descriptor desc = napi_property_descriptor(); 
        
             desc.utf8name = utf8name; 
        
             desc.getter = getter != nullptr ? T::InstanceGetterCallbackWrapper : nullptr; 
        
             desc.setter = setter != nullptr ? T::InstanceSetterCallbackWrapper : nullptr; 
        
             desc.data = callbackData; 
        
             desc.attributes = attributes; 
        
             return desc; 
        
           }

Within AttachPropData, there is a check for instance get/set properties, but it is never reached due to the prop->method != null check:

node-addon-api/napi-inl.h

Lines 3277 to 3282 in 286ae21

    
           } else if (prop->getter == T::InstanceGetterCallbackWrapper || 
        
               prop->setter == T::InstanceSetterCallbackWrapper) { 
        
             status = Napi::details::AttachData(env, 
        
                                 value, 
        
                                 static_cast<InstanceAccessorCallbackData*>(prop->data)); 
        
             NAPI_THROW_IF_FAILED_VOID(env, status);

By simply removing this method != null check, my ~AccessorCallbackData is now called:

root@b36ee5d3b4df:/app# node .
buffer
destructed!

lygstate · 2021-02-04T11:17:39Z

good catcha, I also fixed this one, also have other issues. I've create a PR for this

Fixes: nodejs/node-addon-api#891 PR-URL: nodejs/node-addon-api#899 Fixes: nodejs/node-addon-api#891 Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

KevinEady self-assigned this Feb 2, 2021

KevinEady added a commit to KevinEady/node-addon-api that referenced this issue Feb 3, 2021

src: fix leak in ObjectWrap instance set/getters

23e2313

Fixes: nodejs#891

KevinEady mentioned this issue Feb 3, 2021

src: fix leak in ObjectWrap instance set/getters #899

Closed

mhdawson closed this as completed in a575a6e Feb 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Valgrind detected memory leak #891

Valgrind detected memory leak #891

lygstate commented Jan 29, 2021 •

edited

Loading

gabrielschulhof commented Jan 29, 2021

KevinEady commented Feb 3, 2021

lygstate commented Feb 4, 2021

Valgrind detected memory leak #891

Valgrind detected memory leak #891

Comments

lygstate commented Jan 29, 2021 • edited Loading

gabrielschulhof commented Jan 29, 2021

KevinEady commented Feb 3, 2021

lygstate commented Feb 4, 2021

lygstate commented Jan 29, 2021 •

edited

Loading