diff --git a/napi-inl.h b/napi-inl.h index 59f1ca14e..990e74af3 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -6746,6 +6746,25 @@ inline AsyncProgressWorkerBase::AsyncProgressWorkerBase( /** context */ this, OnThreadSafeFunctionFinalize, /** finalizeData */ this); + + // Register an environment cleanup hook to abort the thread-safe function and + // prevent event-loop hangs or running JavaScript if the environment + // terminates while the worker is active. + _cleanup_hook = + Env().AddCleanupHook(OnEnvironmentTeardown, static_cast(this)); +} + +template +inline void AsyncProgressWorkerBase::OnEnvironmentTeardown( + void* arg) { + auto* self = static_cast*>(arg); + if (self != nullptr) { + self->_teardown_called = true; + self->_cleanup_hook = BasicEnv::CleanupHook(); + if (self->_tsfn) { + self->_tsfn.Abort(); + } + } } #if NAPI_VERSION > 4 @@ -6771,11 +6790,20 @@ inline AsyncProgressWorkerBase::AsyncProgressWorkerBase( /** context */ this, OnThreadSafeFunctionFinalize, /** finalizeData */ this); + + // Register an environment cleanup hook to abort the thread-safe function and + // prevent event-loop hangs or running JavaScript if the environment + // terminates while the worker is active. + _cleanup_hook = + env.AddCleanupHook(OnEnvironmentTeardown, static_cast(this)); } #endif template inline AsyncProgressWorkerBase::~AsyncProgressWorkerBase() { + if (!_cleanup_hook.IsEmpty()) { + _cleanup_hook.Remove(Env()); + } // Abort pending tsfn call. // Don't send progress events after we've already completed. // It's ok to call ThreadSafeFunction::Abort and ThreadSafeFunction::Release @@ -6804,17 +6832,44 @@ inline napi_status AsyncProgressWorkerBase::NonBlockingCall( template inline void AsyncProgressWorkerBase::OnWorkComplete( - Napi::Env /* env */, napi_status status) { + Napi::Env env, napi_status status) { _work_completed = true; _complete_status = status; - _tsfn.Release(); + if (_tsfn_finalized || _teardown_called) { + AsyncWorker::OnWorkComplete(env, napi_cancelled); + } else { + _tsfn.Release(); + } } template inline void AsyncProgressWorkerBase::OnThreadSafeFunctionFinalize( Napi::Env env, void* /* data */, AsyncProgressWorkerBase* context) { + context->_tsfn_finalized = true; if (context->_work_completed) { - context->AsyncWorker::OnWorkComplete(env, context->_complete_status); + napi_status status = + context->_teardown_called ? napi_cancelled : context->_complete_status; + if (status != napi_cancelled) { + napi_value test_script, test_result; + napi_status s = napi_create_string_utf8(env, "0", 1, &test_script); + if (s == napi_ok) { + s = napi_run_script(env, test_script, &test_result); + } + if (s != napi_ok) { + status = napi_cancelled; + } + } +#ifdef NODE_ADDON_API_CPP_EXCEPTIONS + try { + context->AsyncWorker::OnWorkComplete(env, status); + } catch (const Error&) { + if (!context->_suppress_destruct) { + context->Destroy(); + } + } +#else + context->AsyncWorker::OnWorkComplete(env, status); +#endif } } @@ -7051,7 +7106,10 @@ inline void AsyncProgressQueueWorker::SendProgress_(const T* data, std::copy(data, data + count, new_data); auto pair = new std::pair(new_data, count); - this->NonBlockingCall(pair); + if (this->NonBlockingCall(pair) != napi_ok) { + delete[] new_data; + delete pair; + } } template diff --git a/napi.h b/napi.h index 6b4692c7d..3b9a37278 100644 --- a/napi.h +++ b/napi.h @@ -2863,7 +2863,9 @@ class AsyncWorker { ObjectReference _receiver; FunctionReference _callback; std::string _error; - bool _suppress_destruct; + + protected: + bool _suppress_destruct = false; }; #endif // NAPI_HAS_THREADS @@ -3338,10 +3340,14 @@ class AsyncProgressWorkerBase : public AsyncWorker { private: ThreadSafeFunction _tsfn; + BasicEnv::CleanupHook _cleanup_hook; bool _work_completed = false; + bool _tsfn_finalized = false; + bool _teardown_called = false; napi_status _complete_status; static inline void OnThreadSafeFunctionFinalize( Napi::Env env, void* data, AsyncProgressWorkerBase* context); + static inline void OnEnvironmentTeardown(void* arg); }; template diff --git a/test/async_progress_worker.cc b/test/async_progress_worker.cc index 17aaef3e5..1388f1d96 100644 --- a/test/async_progress_worker.cc +++ b/test/async_progress_worker.cc @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -340,6 +341,65 @@ class SignalAfterProgressTestWorker : public AsyncProgressWorker { } FunctionReference _progress; }; + +std::promise promise_for_worker_thread_; +std::promise promise_for_parent_thread_; + +class TerminatingWorker : public AsyncProgressWorker { + public: + static void DoWork(const CallbackInfo& info) { + Function cb = info[0].As(); + Function progress = info[1].As(); + + TerminatingWorker* worker = new TerminatingWorker(cb, progress); + worker->Queue(); + } + + protected: + void Execute(const ExecutionProgress& progress) override { + // Notify the parent thread that we are executing + promise_for_worker_thread_.set_value(); + + // Wait until parent thread initiates termination + std::future future = promise_for_parent_thread_.get_future(); + std::future_status status = future.wait_for(std::chrono::seconds(30)); + if (status != std::future_status::ready) { + Error::Fatal("TerminatingWorker::Execute", + "status != std::future_status::ready"); + } + + // Try to send progress during/after termination + ProgressData data{1}; + progress.Send(&data, 1); + } + + void OnProgress(const ProgressData* /* data */, size_t /* count */) override { + } + + private: + TerminatingWorker(Function cb, Function progress) : AsyncProgressWorker(cb) { + _progress.Reset(progress, 1); + } + FunctionReference _progress; +}; + +void ResetPromises(const CallbackInfo&) { + promise_for_worker_thread_ = std::promise(); + promise_for_parent_thread_ = std::promise(); +} + +void WaitForWorkerThread(const CallbackInfo&) { + std::future future = promise_for_worker_thread_.get_future(); + std::future_status status = future.wait_for(std::chrono::seconds(30)); + if (status != std::future_status::ready) { + Error::Fatal("WaitForWorkerThread", "status != std::future_status::ready"); + } +} + +void ReleaseWorkerThread(const CallbackInfo&) { + promise_for_parent_thread_.set_value(); +} + } // namespace Object InitAsyncProgressWorker(Env env) { @@ -351,6 +411,10 @@ Object InitAsyncProgressWorker(Env env) { exports["runWorkerNoCb"] = Function::New(env, TestWorkerWithNoCb::DoWork); exports["runWorkerWithRecv"] = Function::New(env, TestWorkerWithRecv::DoWork); exports["runWorkerWithCb"] = Function::New(env, TestWorkerWithCb::DoWork); + exports["resetPromises"] = Function::New(env, ResetPromises); + exports["waitForWorkerThread"] = Function::New(env, WaitForWorkerThread); + exports["releaseWorkerThread"] = Function::New(env, ReleaseWorkerThread); + exports["doTerminatingWork"] = Function::New(env, TerminatingWorker::DoWork); return exports; } diff --git a/test/async_progress_worker_terminating_environment.js b/test/async_progress_worker_terminating_environment.js new file mode 100644 index 000000000..df50db42d --- /dev/null +++ b/test/async_progress_worker_terminating_environment.js @@ -0,0 +1,91 @@ +'use strict'; + +const assert = require('assert'); +const { whichBuildType } = require('./common'); + +// These tests ensure that AsyncProgressWorker can be used in a terminating +// environment without triggering any crashes, use-after-free, or fatal errors. + +if (process.argv[2] === 'runInChildProcess') { + const bindingPath = process.argv[3]; + const testCase = Number(process.argv[4]); + + const binding = require(bindingPath); + + binding.asyncprogressworker.resetPromises(); + + const { Worker } = require('worker_threads'); + + const worker = new Worker( + __filename, + { + argv: [ + 'runInWorkerThread', + bindingPath, + testCase + ] + } + ); + + if (testCase === 0) { + // Test case 0: Environment teardown occurs while worker is actively running + binding.asyncprogressworker.waitForWorkerThread(); + worker.terminate(); + binding.asyncprogressworker.releaseWorkerThread(); + } else if (testCase === 1) { + // Test case 1: Worker finishes normally, then environment is torn down + // (Verifies cleanup hook was safely removed and does not trigger use-after-free on teardown) + worker.on('exit', () => { + process.exit(0); + }); + } +} else if (process.argv[2] === 'runInWorkerThread') { + const bindingPath = process.argv[3]; + const testCase = Number(process.argv[4]); + + const binding = require(bindingPath); + + if (testCase === 0) { + binding.asyncprogressworker.doTerminatingWork( + () => {}, + () => {} + ); + } else if (testCase === 1) { + binding.asyncprogressworker.doWork( + 1, + () => {}, + () => {} + ); + } +} else { + module.exports = wrapTest(); + + async function wrapTest () { + const path = require('path'); + const buildType = await whichBuildType(); + test(path.join(__dirname, `build/${buildType}/binding.node`)); + test(path.join(__dirname, `build/${buildType}/binding_noexcept.node`)); + } + + function test (bindingPath) { + const numberOfTestCases = 2; + + for (let i = 0; i < numberOfTestCases; ++i) { + const childProcess = require('./napi_child').spawnSync( + process.execPath, + [ + __filename, + 'runInChildProcess', + bindingPath, + i + ] + ); + + assert.strictEqual( + childProcess.status, + 0, + `Test case ${bindingPath} ${i} failed with status ${childProcess.status}: ${childProcess.stderr}` + ); + } + } +} diff --git a/test/index.js b/test/index.js index f0c034584..a5b00f728 100644 --- a/test/index.js +++ b/test/index.js @@ -109,6 +109,7 @@ if (napiVersion < 3) { if (napiVersion < 4 && !filterConditionsProvided) { testModules.splice(testModules.indexOf('asyncprogressqueueworker'), 1); testModules.splice(testModules.indexOf('asyncprogressworker'), 1); + testModules.splice(testModules.indexOf('async_progress_worker_terminating_environment'), 1); testModules.splice(testModules.indexOf('threadsafe_function/threadsafe_function_ctx'), 1); testModules.splice(testModules.indexOf('threadsafe_function/threadsafe_function_existing_tsfn'), 1); testModules.splice(testModules.indexOf('threadsafe_function/threadsafe_function_ptr'), 1); @@ -131,6 +132,7 @@ if (napiVersion < 6 && !filterConditionsProvided) { if (majorNodeVersion < 12 && !filterConditionsProvided) { testModules.splice(testModules.indexOf('objectwrap_worker_thread'), 1); testModules.splice(testModules.indexOf('error_terminating_environment'), 1); + testModules.splice(testModules.indexOf('async_progress_worker_terminating_environment'), 1); } if (napiVersion < 8 && !filterConditionsProvided) {