[WIP] node-api: fix data race and use-after-free in napi_threadsafe_function #55877
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
|
I still need to see how to integrate the test from here https://github.com/mika-fischer/node-bug-napi-tsfn |
|
@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. |
|
LGTM. @mika-fischer, please add the test when you get a chance! |
| using node::AsyncResource::CallbackScope; | ||
| }; | ||
|
|
||
| enum State : unsigned char { OPEN, CLOSING, CLOSED }; |
There was a problem hiding this comment.
Please follow these guidelines for the enum names: https://google.github.io/styleguide/cppguide.html#Enumerator_Names
The Node.js guidelines https://github.com/nodejs/node/blob/main/doc/contributing/cpp-style-guide.md are based on the Google C++ Style Guide.
gabrielschulhof
force-pushed
the
fix-55706
branch
from
0384dff to
791bc6a
Compare
gabrielschulhof
changed the title
|
Marking as ready for review so the tests will run. |
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
gabrielschulhof
force-pushed
the
fix-55706
branch
from
791bc6a to
fac8a80
Compare
@gabrielschulhof Thanks for pushing this further, and sorry for not following up! FYI The test case only triggers reliably under valgrind. Otherwise it crashes only occasionally. I don't know if anthing special is needed or if all tests are run with valgrind anyway. |
Other threads can still hold a valid handle to the tsfn after
finalization if finalization was triggered by
Handle this by:
drops to 0
Fixes: #55706