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

node-api: fix data race and use-after-free in napi_threadsafe_function #55877

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

node-api: fix data race and use-after-free in napi_threadsafe_function #55877

wants to merge 2 commits into from
+98 −52

Conversation

mika-fischer
Copy link
Contributor

Other threads can still hold a valid handle to the tsfn after
finalization if finalization was triggered by

  • release with napi_tsfn_abort, or
  • environment shutdown

Handle this by:

  • protecting finalization itself with the mutex
  • if necessary, delay deletion after finalization to when thread_count
    drops to 0
  • releasing all resources as soon as possible before deletion

Fixes: #55706

@mika-fischer
node-api: fix data race and use-after-free in napi_threadsafe_function
b45c815 
Other threads can still hold a valid handle to the tsfn after
finalization if finalization was triggered by
- release with napi_tsfn_abort, or
- environment shutdown

Handle this by:
- protecting finalization itself with the mutex
- if necessary, delay deletion after finalization to when thread_count
  drops to 0
- releasing all resources as soon as possible before deletion

Fixes: nodejs#55706
@mika-fischer
node-api: combine threadsafe_function state flags into single variable
0384dff
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/node-api

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API. labels Nov 16, 2024
@mika-fischer
Copy link
Contributor Author

I still need to see how to integrate the test from here https://github.com/mika-fischer/node-bug-napi-tsfn

@mika-fischer mika-fischer mentioned this pull request Nov 16, 2024
Open
@mhdawson
Copy link
Member

@gabrielschulhof you mentioned you'd take a look at this issue in our weekly meeting last week. If you can take a look at this PR it would be great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API.
Projects
Node-API Team Project
Status: Need Triage
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

napi_threadsafe_function is very hard to use safely
3 participants
@mika-fischer @nodejs-github-bot @mhdawson