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.

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

[GIT PULL] Fix a memory leak bug in AsyncProgressWorkerBase #1264

Closed

Conversation

ammarfaizi2
Copy link
Contributor

Hi,

We found a serious memory leak issue in the async worker. ASAN reported
2706523824 bytes (~2.5 GB) direct leak in our application that uses
node-addon-api.

Short technical explanation:

In AsyncProgressWorkerBase<DataType>::NonBlockingCall if the call to
_tsfn.NonBlockingCall() doesn't return a napi_ok, the ThreadSafeData
object is not deleted by OnAsyncWorkProgress(), resulting a memory
leak bug.

Report from ASAN (Address Sanitizer):

  Direct leak of 2706523824 byte(s) in 169157739 object(s) allocated:
  # 0 0x7fc83c2dd76d in operator new(unsigned long)
  # 1 0x7fc83b639fc2 in Napi::AsyncProgressWorkerBase<void>::NonBlockingCall(void*)
  # 2 0x7fc83b639fc2 in Napi::AsyncProgressWorker<unsigned char>::SendProgress_()
  # 3 0x7fc83b635cd0 in Napi::AsyncProgressWorker<unsigned char>::ExecutionProgress::Send()
  # 4 0x7fc83b635cd0 in WaitCQEWorker::Execute()
  # 5 0x7fc83b636545 in Napi::AsyncProgressWorker<unsigned char>::Execute()
  # 6 0xb8df59 in node::ThreadPoolWork::ScheduleWork()::'lambda'(uv_work_s*)::_FUN(uv_work_s*)
  # 7 0x1768fb3 in worker /home/iojs/build/ws/out/../deps/uv/src/threadpool.c:122:5
  # 8 0x7fc83ba94b42 in start_thread nptl/./nptl/pthread_create.c:442:8

Fix this by deleting the tsd variable if _tsfn.NonBlockingCall()
doesn't return a napi_ok.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>


By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or

(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.

(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.

Please pull!

The following changes since commit 55bd08ee26fded907a9e291c3a901e4b000c70f0:

  src: api to get callback_info from CallBackInfo (2023-01-03 15:56:24 -0500)

are available in the Git repository at:

  https://github.com/ammarfaizi2/node-addon-api fix_memory_leak_async_worker

for you to fetch changes up to 1774815b26471a865a0cbfb33e2f7a1a370029b4:

  src: napi-inl: Fix a memory leak bug in `AsyncProgressWorkerBase` (2023-01-05 15:02:30 +0700)

----------------------------------------------------------------
Ammar Faizi (1):
      src: napi-inl: Fix a memory leak bug in `AsyncProgressWorkerBase`

 napi-inl.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

In `AsyncProgressWorkerBase<DataType>::NonBlockingCall` if the call to
`_tsfn.NonBlockingCall()` doesn't return a `napi_ok`, the ThreadSafeData
object is not deleted by `OnAsyncWorkProgress()`, resulting a memory
leak bug.

Report from ASAN (Address Sanitizer):
```
  Direct leak of 2706523824 byte(s) in 169157739 object(s) allocated:
  # 0 0x7fc83c2dd76d in operator new(unsigned long)
  # 1 0x7fc83b639fc2 in Napi::AsyncProgressWorkerBase<void>::NonBlockingCall(void*)
  # 2 0x7fc83b639fc2 in Napi::AsyncProgressWorker<unsigned char>::SendProgress_()
  # 3 0x7fc83b635cd0 in Napi::AsyncProgressWorker<unsigned char>::ExecutionProgress::Send()
  # 4 0x7fc83b635cd0 in WaitCQEWorker::Execute()
  # 5 0x7fc83b636545 in Napi::AsyncProgressWorker<unsigned char>::Execute()
  # 6 0xb8df59 in node::ThreadPoolWork::ScheduleWork()::'lambda'(uv_work_s*)::_FUN(uv_work_s*)
  # 7 0x1768fb3 in worker /home/iojs/build/ws/out/../deps/uv/src/threadpool.c:122:5
  # 8 0x7fc83ba94b42 in start_thread nptl/./nptl/pthread_create.c:442:8
```

Fix this by deleting the tsd variable if `_tsfn.NonBlockingCall()`
doesn't return a `napi_ok`.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
@mhdawson mhdawson requested a review from KevinEady January 5, 2023 16:49
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@KevinEady KevinEady left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

The ThreadSafeData was only deleted inside the OnAsyncWorkProgress callback, and of course if this call fails to schedule, it would never be deleted.

mhdawson pushed a commit that referenced this pull request Jan 9, 2023
In `AsyncProgressWorkerBase<DataType>::NonBlockingCall` if the call to
`_tsfn.NonBlockingCall()` doesn't return a `napi_ok`, the ThreadSafeData
object is not deleted by `OnAsyncWorkProgress()`, resulting a memory
leak bug.

Report from ASAN (Address Sanitizer):
```
  Direct leak of 2706523824 byte(s) in 169157739 object(s) allocated:
  # 0 0x7fc83c2dd76d in operator new(unsigned long)
  # 1 0x7fc83b639fc2 in Napi::AsyncProgressWorkerBase<void>::NonBlockingCall(void*)
  # 2 0x7fc83b639fc2 in Napi::AsyncProgressWorker<unsigned char>::SendProgress_()
  # 3 0x7fc83b635cd0 in Napi::AsyncProgressWorker<unsigned char>::ExecutionProgress::Send()
  # 4 0x7fc83b635cd0 in WaitCQEWorker::Execute()
  # 5 0x7fc83b636545 in Napi::AsyncProgressWorker<unsigned char>::Execute()
  # 6 0xb8df59 in node::ThreadPoolWork::ScheduleWork()::'lambda'(uv_work_s*)::_FUN(uv_work_s*)
  # 7 0x1768fb3 in worker /home/iojs/build/ws/out/../deps/uv/src/threadpool.c:122:5
  # 8 0x7fc83ba94b42 in start_thread nptl/./nptl/pthread_create.c:442:8
```

Fix this by deleting the tsd variable if `_tsfn.NonBlockingCall()`
doesn't return a `napi_ok`.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>

PR-URL: #1264
Reviewed-By: Michael Dawson <midawson@redhat.com
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com>
@mhdawson
Copy link
Member

mhdawson commented Jan 9, 2023

Landed in 01c6169

@mhdawson mhdawson closed this Jan 9, 2023
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
In `AsyncProgressWorkerBase<DataType>::NonBlockingCall` if the call to
`_tsfn.NonBlockingCall()` doesn't return a `napi_ok`, the ThreadSafeData
object is not deleted by `OnAsyncWorkProgress()`, resulting a memory
leak bug.

Report from ASAN (Address Sanitizer):
```
  Direct leak of 2706523824 byte(s) in 169157739 object(s) allocated:
  # 0 0x7fc83c2dd76d in operator new(unsigned long)
  # 1 0x7fc83b639fc2 in Napi::AsyncProgressWorkerBase<void>::NonBlockingCall(void*)
  # 2 0x7fc83b639fc2 in Napi::AsyncProgressWorker<unsigned char>::SendProgress_()
  # 3 0x7fc83b635cd0 in Napi::AsyncProgressWorker<unsigned char>::ExecutionProgress::Send()
  # 4 0x7fc83b635cd0 in WaitCQEWorker::Execute()
  # 5 0x7fc83b636545 in Napi::AsyncProgressWorker<unsigned char>::Execute()
  # 6 0xb8df59 in node::ThreadPoolWork::ScheduleWork()::'lambda'(uv_work_s*)::_FUN(uv_work_s*)
  # 7 0x1768fb3 in worker /home/iojs/build/ws/out/../deps/uv/src/threadpool.c:122:5
  # 8 0x7fc83ba94b42 in start_thread nptl/./nptl/pthread_create.c:442:8
```

Fix this by deleting the tsd variable if `_tsfn.NonBlockingCall()`
doesn't return a `napi_ok`.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>

PR-URL: nodejs/node-addon-api#1264
Reviewed-By: Michael Dawson <midawson@redhat.com
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants