Skip to content

Valgrind detected memory leak #891

Closed
Closed
Valgrind detected memory leak#891
Assignees
KevinEady

Description

@lygstate
lygstate
opened on Jan 29, 2021
==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;
}

Activity

  1. gabrielschulhof commented on Jan 29, 2021

    @gabrielschulhof
    gabrielschulhof
    on Jan 29, 2021
    Contributor

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

  2. KevinEady
    self-assigned this
    on Feb 2, 2021
  3. KevinEady
    added a commit that references this issue on Feb 3, 2021

    src: fix leak in ObjectWrap instance set/getters

    23e2313

  4. KevinEady commented on Feb 3, 2021

    @KevinEady
    KevinEady
    on Feb 3, 2021
    Contributor

    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!

  5. lygstate commented on Feb 4, 2021

    @lygstate
    lygstate
    on Feb 4, 2021
    Author

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

  6. mhdawson
    closed this as completedin a575a6eon Feb 12, 2021
  7. kevindavies8
    added a commit that references this issue on Aug 24, 2022

    src: fix leak in ObjectWrap instance set/getters

    e6e8fde
  8. Marlyfleitas
    added a commit that references this issue on Aug 26, 2022

    src: fix leak in ObjectWrap instance set/getters

    a3f8aaa
  9. wroy7860
    added a commit that references this issue on Sep 19, 2022

    src: fix leak in ObjectWrap instance set/getters

    8e617b3
  10. johnfrench3
    added a commit that references this issue on Aug 11, 2023

    src: fix leak in ObjectWrap instance set/getters

    8bf3a53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions