Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

N-API reference management leaks memory #34731

Closed
addaleax opened this issue Aug 11, 2020 · 4 comments
Closed

N-API reference management leaks memory #34731

addaleax opened this issue Aug 11, 2020 · 4 comments
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI. memory Issues and PRs related to the memory management or memory footprint. node-api Issues and PRs related to the Node-API.

Comments

@addaleax
Copy link
Member

After making the node-api/test_worker_terminate_finalization complete run as expected in #34726, it starts failing under ASAN/valgrind because it leaks memory (the reference from napi_create_reference() in the test is never freed).

I’ve tried to debug this for an hour, but got nowhere, so I’ll open a PR to just skip the test (shouldn’t exactly make things worse since the test hasn’t been working until today anyway).

/cc The only people who understand N-API reference lifetime management, aka @gabrielschulhof and maybe @mhdawson
@addaleax addaleax added flaky-test Issues and PRs related to the tests with unstable failures on the CI. memory Issues and PRs related to the memory management or memory footprint. node-api Issues and PRs related to the Node-API. labels Aug 11, 2020
addaleax added a commit to addaleax/node that referenced this issue Aug 11, 2020
@addaleax
test: skip node-api/test_worker_terminate_finalization
1078495 
The test fails under ASAN/valgrind. Since it has not been working
properly until today anyway, skip it.

Refs: nodejs#34731
@addaleax addaleax mentioned this issue Aug 11, 2020
Closed
3 tasks
mmarchini pushed a commit that referenced this issue Aug 12, 2020
@addaleax @mmarchini
test: skip node-api/test_worker_terminate_finalization
cf5d7c0 
The test fails under ASAN/valgrind. Since it has not been working
properly until today anyway, skip it.

Refs: #34731

PR-URL: #34732
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
@gengjiawen

This comment has been minimized.

Sign in to view
@gengjiawen
Copy link
Member

Is there a way I can directly run this single test ? python3 tools/test.py --mode=debug test/node-api/test_worker_terminate_finalization/test.js looks like not working.

@richardlau
Copy link
Member

@gengjiawen Did you build the addon (make build-node-api-tests) first? It's possible you'll need to build the addon manually for debug mode as I think the makefile assumes release in a couple of places.

@gengjiawen
Copy link
Member

@gengjiawen Did you build the addon (make build-node-api-tests) first? It's possible you'll need to build the addon manually for debug mode as I think the makefile assumes release in a couple of places.

Thanks, this do work, although the script has force me to build a release build 😭

Debug stacktrace

=== debug test ===                   
Path: node-api/test_worker_terminate_finalization/test
=================================================================
==19561==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 80 byte(s) in 1 object(s) allocated from:
    #0 0xf0932d in operator new(unsigned long) (/root/node/out/Debug/node+0xf0932d)
    #1 0x1290ff8 in v8impl::(anonymous namespace)::Reference::New(napi_env__*, v8::Local<v8::Value>, unsigned int, bool, void (*)(napi_env__*, void*, void*), void*, void*) /root/node/out/Debug/../../src/js_native_api_v8.cc:313:12
    #2 0x12936fa in napi_create_reference /root/node/out/Debug/../../src/js_native_api_v8.cc:2462:7
    #3 0x7f86ceefdf72  (<unknown module>)
    #4 0x12a1612 in v8impl::(anonymous namespace)::CallbackWrapperBase<v8::FunctionCallbackInfo<v8::Value>, &(v8impl::(anonymous namespace)::CallbackBundle::function_or_getter)>::InvokeCallback()::'lambda'(napi_env__*)::operator()(napi_env__*) const /root/node/out/Debug/../../src/js_native_api_v8.cc:477:16
    #5 0x12a12bf in void napi_env__::CallIntoModule<v8impl::(anonymous namespace)::CallbackWrapperBase<v8::FunctionCallbackInfo<v8::Value>, &(v8impl::(anonymous namespace)::CallbackBundle::function_or_getter)>::InvokeCallback()::'lambda'(napi_env__*), void (napi_env__*, v8::Local<v8::Value>)>(v8::FunctionCallbackInfo<v8::Value>&&, &(v8impl::(anonymous namespace)::CallbackBundle::function_or_getter)&&) /root/node/out/Debug/../../src/js_native_api_v8.h:95:5
    #6 0x129ff67 in v8impl::(anonymous namespace)::CallbackWrapperBase<v8::FunctionCallbackInfo<v8::Value>, &(v8impl::(anonymous namespace)::CallbackBundle::function_or_getter)>::InvokeCallback() /root/node/out/Debug/../../src/js_native_api_v8.cc:476:10
    #7 0x12706c2 in v8impl::(anonymous namespace)::FunctionCallbackWrapper::Invoke(v8::FunctionCallbackInfo<v8::Value> const&) /root/node/out/Debug/../../src/js_native_api_v8.cc:495:15
    #8 0x1fb3db2 in v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo) /root/node/out/Debug/../../deps/v8/src/api/api-arguments-inl.h:158:3
    #9 0x1fb071f in v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) /root/node/out/Debug/../../deps/v8/src/builtins/builtins-api.cc:111:36
    #10 0x1fad6fa in v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) /root/node/out/Debug/../../deps/v8/src/builtins/builtins-api.cc:141:5
    #11 0x1fac950 in v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) /root/node/out/Debug/../../deps/v8/src/builtins/builtins-api.cc:129:1
    #12 0x401a13f in Builtins_CEntry_Return1_DontSaveFPRegs_ArgvOnStack_BuiltinExit (/root/node/out/Debug/node+0x401a13f)
    #13 0x3e16be1 in Builtins_InterpreterEntryTrampoline (/root/node/out/Debug/node+0x3e16be1)
    #14 0x3e16be1 in Builtins_InterpreterEntryTrampoline (/root/node/out/Debug/node+0x3e16be1)
    #15 0x3e16be1 in Builtins_InterpreterEntryTrampoline (/root/node/out/Debug/node+0x3e16be1)
    #16 0x3e16be1 in Builtins_InterpreterEntryTrampoline (/root/node/out/Debug/node+0x3e16be1)
    #17 0x3e16be1 in Builtins_InterpreterEntryTrampoline (/root/node/out/Debug/node+0x3e16be1)
    #18 0x3e16be1 in Builtins_InterpreterEntryTrampoline (/root/node/out/Debug/node+0x3e16be1)
    #19 0x3e16be1 in Builtins_InterpreterEntryTrampoline (/root/node/out/Debug/node+0x3e16be1)
    #20 0x3e16be1 in Builtins_InterpreterEntryTrampoline (/root/node/out/Debug/node+0x3e16be1)
    #21 0x3e16be1 in Builtins_InterpreterEntryTrampoline (/root/node/out/Debug/node+0x3e16be1)
    #22 0x3e0d839 in Builtins_JSEntryTrampoline (/root/node/out/Debug/node+0x3e0d839)
    #23 0x3e0d617 in Builtins_JSEntry (/root/node/out/Debug/node+0x3e0d617)
    #24 0x23a0a4b in v8::internal::GeneratedCode<unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, long, unsigned long**>::Call(unsigned long, unsigned long, unsigned long, unsigned long, long, unsigned long**) /root/node/out/Debug/../../deps/v8/src/execution/simulator.h:142:12
    #25 0x23a0a4b in v8::internal::(anonymous namespace)::Invoke(v8::internal::Isolate*, v8::internal::(anonymous namespace)::InvokeParams const&) /root/node/out/Debug/../../deps/v8/src/execution/execution.cc:367:33
    #26 0x239f6f2 in v8::internal::Execution::Call(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*) /root/node/out/Debug/../../deps/v8/src/execution/execution.cc:461:10
    #27 0x1da8c33 in v8::Function::Call(v8::Local<v8::Context>, v8::Local<v8::Value>, int, v8::Local<v8::Value>*) /root/node/out/Debug/../../deps/v8/src/api/api.cc:4876:7
    #28 0xff6503 in node::InternalMakeCallback(node::Environment*, v8::Local<v8::Object>, v8::Local<v8::Object>, v8::Local<v8::Function>, int, v8::Local<v8::Value>*, node::async_context) /root/node/out/Debug/../../src/api/callback.cc:191:21
    #29 0x109d902 in node::AsyncWrap::MakeCallback(v8::Local<v8::Function>, int, v8::Local<v8::Value>*) /root/node/out/Debug/../../src/async_wrap.cc:974:27
    #30 0x15dc8b7 in node::worker::MessagePort::OnMessage() /root/node/out/Debug/../../src/node_messaging.cc:772:9

SUMMARY: AddressSanitizer: 80 byte(s) leaked in 1 allocation(s).

MylesBorins pushed a commit that referenced this issue Aug 17, 2020
@addaleax @MylesBorins
test: skip node-api/test_worker_terminate_finalization
e5a3991 
The test fails under ASAN/valgrind. Since it has not been working
properly until today anyway, skip it.

Refs: #34731

PR-URL: #34732
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
@gabrielschulhof gabrielschulhof mentioned this issue Aug 18, 2020
Closed
3 tasks
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this issue Aug 18, 2020
test,n-api: delete strong ref w/o unrefing first
2a8342e 
Re: nodejs#34731
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
@gabrielschulhof gabrielschulhof mentioned this issue Aug 19, 2020
Closed
3 tasks
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this issue Aug 19, 2020
n-api: handle weak no-finalizer refs correctly
2a25930 
When deleting a weak reference that has no finalizer we must not defer
deletion until the non-existent finalizer gets called.

Fixes: nodejs#34731
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
BethGriggs pushed a commit that referenced this issue Aug 20, 2020
@addaleax @BethGriggs
test: skip node-api/test_worker_terminate_finalization
0472d16 
The test fails under ASAN/valgrind. Since it has not been working
properly until today anyway, skip it.

Refs: #34731

PR-URL: #34732
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
@Trott Trott closed this as completed in acd423b Aug 21, 2020
BethGriggs pushed a commit that referenced this issue Aug 24, 2020
@BethGriggs
n-api: handle weak no-finalizer refs correctly
98f7d8e 
When deleting a weak reference that has no finalizer we must not defer
deletion until the non-existent finalizer gets called.

Fixes: #34731
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>

PR-URL: #34839
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
addaleax added a commit that referenced this issue Sep 22, 2020
@addaleax
test: skip node-api/test_worker_terminate_finalization
3b70c10 
The test fails under ASAN/valgrind. Since it has not been working
properly until today anyway, skip it.

Refs: #34731

PR-URL: #34732
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
addaleax pushed a commit that referenced this issue Sep 22, 2020
@addaleax
n-api: handle weak no-finalizer refs correctly
6ccff8e 
When deleting a weak reference that has no finalizer we must not defer
deletion until the non-existent finalizer gets called.

Fixes: #34731
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>

PR-URL: #34839
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
addaleax added a commit that referenced this issue Sep 22, 2020
@addaleax
test: skip node-api/test_worker_terminate_finalization
335b61a 
The test fails under ASAN/valgrind. Since it has not been working
properly until today anyway, skip it.

Refs: #34731

PR-URL: #34732
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
addaleax pushed a commit that referenced this issue Sep 22, 2020
@addaleax
n-api: handle weak no-finalizer refs correctly
a6b6556 
When deleting a weak reference that has no finalizer we must not defer
deletion until the non-existent finalizer gets called.

Fixes: #34731
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>

PR-URL: #34839
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@mhdawson mhdawson mentioned this issue Oct 23, 2020
Closed
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this issue Nov 2, 2020
n-api: unlink reference during its destructor
dd7d042 
Currently, a reference is being unlinked from the list of references
tracked by the environment when `v8impl::Reference::Delete` is called.
This causes a leak when deletion must be deferred because the finalizer
hasn't yet run, but the finalizer does not run because environment
teardown is in progress, and so no more gc runs will happen, and the
`FinalizeAll` run that happens during environment teardown does not
catch the reference because it's no longer in the list. The test below
will fail when running with ASAN:
```
./node ./test/node-api/test_worker_terminate_finalization/test.js
```

OTOH if, to address the above leak, we make a special case to not
unlink a reference during environment teardown, we run into a
situation where the reference gets deleted by
`v8impl::Reference::Delete` but does not get unlinked because it's
environment teardown time. This leaves a stale pointer in the linked
list which will result in a use-after-free in `FinalizeAll` during
environment teardown. The test below will fail if we make the above
change:
```
./node -e "require('./test/node-api/test_instance_data/build/Release/test_ref_then_set.node');"
```

Thus, we unlink a reference precisely when we destroy it – in its
destructor.

Refs: nodejs#34731
Refs: nodejs#34839
Refs: nodejs#35620
Refs: nodejs#35777
Fixes: nodejs#35778
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
@gabrielschulhof gabrielschulhof mentioned this issue Nov 2, 2020
Closed
4 tasks
gabrielschulhof pushed a commit that referenced this issue Nov 5, 2020
n-api: unlink reference during its destructor
c822ba7 
Currently, a reference is being unlinked from the list of references
tracked by the environment when `v8impl::Reference::Delete` is called.
This causes a leak when deletion must be deferred because the finalizer
hasn't yet run, but the finalizer does not run because environment
teardown is in progress, and so no more gc runs will happen, and the
`FinalizeAll` run that happens during environment teardown does not
catch the reference because it's no longer in the list. The test below
will fail when running with ASAN:

```
./node ./test/node-api/test_worker_terminate_finalization/test.js
```

OTOH if, to address the above leak, we make a special case to not
unlink a reference during environment teardown, we run into a
situation where the reference gets deleted by
`v8impl::Reference::Delete` but does not get unlinked because it's
environment teardown time. This leaves a stale pointer in the linked
list which will result in a use-after-free in `FinalizeAll` during
environment teardown. The test below will fail if we make the above
change:

```
./node -e "require('./test/node-api/test_instance_data/build/Release/test_ref_then_set.node');"
```

Thus, we unlink a reference precisely when we destroy it – in its
destructor.

Refs: #34731
Refs: #34839
Refs: #35620
Refs: #35777
Fixes: #35778
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
PR-URL: #35933
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
danielleadams pushed a commit that referenced this issue Nov 9, 2020
@danielleadams
n-api: unlink reference during its destructor
9b6512f 
Currently, a reference is being unlinked from the list of references
tracked by the environment when `v8impl::Reference::Delete` is called.
This causes a leak when deletion must be deferred because the finalizer
hasn't yet run, but the finalizer does not run because environment
teardown is in progress, and so no more gc runs will happen, and the
`FinalizeAll` run that happens during environment teardown does not
catch the reference because it's no longer in the list. The test below
will fail when running with ASAN:

```
./node ./test/node-api/test_worker_terminate_finalization/test.js
```

OTOH if, to address the above leak, we make a special case to not
unlink a reference during environment teardown, we run into a
situation where the reference gets deleted by
`v8impl::Reference::Delete` but does not get unlinked because it's
environment teardown time. This leaves a stale pointer in the linked
list which will result in a use-after-free in `FinalizeAll` during
environment teardown. The test below will fail if we make the above
change:

```
./node -e "require('./test/node-api/test_instance_data/build/Release/test_ref_then_set.node');"
```

Thus, we unlink a reference precisely when we destroy it – in its
destructor.

Refs: #34731
Refs: #34839
Refs: #35620
Refs: #35777
Fixes: #35778
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
PR-URL: #35933
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
BethGriggs pushed a commit that referenced this issue Dec 9, 2020
@BethGriggs
n-api: unlink reference during its destructor
c5926f9 
Currently, a reference is being unlinked from the list of references
tracked by the environment when `v8impl::Reference::Delete` is called.
This causes a leak when deletion must be deferred because the finalizer
hasn't yet run, but the finalizer does not run because environment
teardown is in progress, and so no more gc runs will happen, and the
`FinalizeAll` run that happens during environment teardown does not
catch the reference because it's no longer in the list. The test below
will fail when running with ASAN:

```
./node ./test/node-api/test_worker_terminate_finalization/test.js
```

OTOH if, to address the above leak, we make a special case to not
unlink a reference during environment teardown, we run into a
situation where the reference gets deleted by
`v8impl::Reference::Delete` but does not get unlinked because it's
environment teardown time. This leaves a stale pointer in the linked
list which will result in a use-after-free in `FinalizeAll` during
environment teardown. The test below will fail if we make the above
change:

```
./node -e "require('./test/node-api/test_instance_data/build/Release/test_ref_then_set.node');"
```

Thus, we unlink a reference precisely when we destroy it – in its
destructor.

Refs: #34731
Refs: #34839
Refs: #35620
Refs: #35777
Fixes: #35778
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
PR-URL: #35933
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
BethGriggs pushed a commit that referenced this issue Dec 10, 2020
@BethGriggs
n-api: unlink reference during its destructor
512fb7b 
Currently, a reference is being unlinked from the list of references
tracked by the environment when `v8impl::Reference::Delete` is called.
This causes a leak when deletion must be deferred because the finalizer
hasn't yet run, but the finalizer does not run because environment
teardown is in progress, and so no more gc runs will happen, and the
`FinalizeAll` run that happens during environment teardown does not
catch the reference because it's no longer in the list. The test below
will fail when running with ASAN:

```
./node ./test/node-api/test_worker_terminate_finalization/test.js
```

OTOH if, to address the above leak, we make a special case to not
unlink a reference during environment teardown, we run into a
situation where the reference gets deleted by
`v8impl::Reference::Delete` but does not get unlinked because it's
environment teardown time. This leaves a stale pointer in the linked
list which will result in a use-after-free in `FinalizeAll` during
environment teardown. The test below will fail if we make the above
change:

```
./node -e "require('./test/node-api/test_instance_data/build/Release/test_ref_then_set.node');"
```

Thus, we unlink a reference precisely when we destroy it – in its
destructor.

Refs: #34731
Refs: #34839
Refs: #35620
Refs: #35777
Fixes: #35778
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
PR-URL: #35933
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
BethGriggs pushed a commit that referenced this issue Dec 15, 2020
@BethGriggs
n-api: unlink reference during its destructor
88aa4e0 
Currently, a reference is being unlinked from the list of references
tracked by the environment when `v8impl::Reference::Delete` is called.
This causes a leak when deletion must be deferred because the finalizer
hasn't yet run, but the finalizer does not run because environment
teardown is in progress, and so no more gc runs will happen, and the
`FinalizeAll` run that happens during environment teardown does not
catch the reference because it's no longer in the list. The test below
will fail when running with ASAN:

```
./node ./test/node-api/test_worker_terminate_finalization/test.js
```

OTOH if, to address the above leak, we make a special case to not
unlink a reference during environment teardown, we run into a
situation where the reference gets deleted by
`v8impl::Reference::Delete` but does not get unlinked because it's
environment teardown time. This leaves a stale pointer in the linked
list which will result in a use-after-free in `FinalizeAll` during
environment teardown. The test below will fail if we make the above
change:

```
./node -e "require('./test/node-api/test_instance_data/build/Release/test_ref_then_set.node');"
```

Thus, we unlink a reference precisely when we destroy it – in its
destructor.

Refs: #34731
Refs: #34839
Refs: #35620
Refs: #35777
Fixes: #35778
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
PR-URL: #35933
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
@RaisinTen RaisinTen mentioned this issue Jan 10, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI. memory Issues and PRs related to the memory management or memory footprint. node-api Issues and PRs related to the Node-API.
Projects
None yet
Milestone
No milestone
3 participants
@addaleax @gengjiawen @richardlau