From 671346ee8f1e37ec73c9d48521d1701025995e05 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 7 Jun 2018 14:43:45 +0200 Subject: [PATCH] src: do proper error checking in `AsyncWrap::MakeCallback` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit At least one method on a native object is added as a getter, namely `MessagePort.prototype.onmessage`. When a MessagePort attempts to call this method from C++ in response to receiving data, it will first invoke that getter and then call the function. Since `worker.terminate()` interrupts execution, this means that the getter may fail (without being faulty code on its own). This means that at least one test exercising these methods in combination has been flaky and could have crashed, because we did not actually check that the getter returns a value so far, resulting in dereferencing an empty `Local`. The proper fix for this is to use the non-deprecated overload of `Get()` and check the result like we should be doing. Also, as a (related) fix, don’t crash if the method is not a function but rather something else, like a getter could provide. Example test failure: https://ci.nodejs.org/job/node-test-commit-linux-containered/4976/nodes=ubuntu1604_sharedlibs_zlib_x64/console 17:56:56 not ok 1955 parallel/test-worker-dns-terminate 17:56:56 --- 17:56:56 duration_ms: 1.237 17:56:56 severity: crashed 17:56:56 exitcode: -11 17:56:56 stack: |- PR-URL: https://github.com/nodejs/node/pull/21189 Reviewed-By: Anatoli Papirovski Reviewed-By: Benjamin Gruenbaum Reviewed-By: Joyee Cheung Reviewed-By: James M Snell --- src/async_wrap-inl.h | 20 +++----- src/async_wrap.h | 3 -- src/handle_wrap.h | 4 ++ src/node_messaging.cc | 13 ++++- src/node_messaging.h | 2 + .../test-async-wrap-missing-method.js | 49 +++++++++++++++++++ 6 files changed, 75 insertions(+), 16 deletions(-) create mode 100644 test/parallel/test-async-wrap-missing-method.js diff --git a/src/async_wrap-inl.h b/src/async_wrap-inl.h index 5763b17aa08bc4..4405bb3a9baa52 100644 --- a/src/async_wrap-inl.h +++ b/src/async_wrap-inl.h @@ -81,18 +81,14 @@ inline v8::MaybeLocal AsyncWrap::MakeCallback( const v8::Local symbol, int argc, v8::Local* argv) { - v8::Local cb_v = object()->Get(symbol); - CHECK(cb_v->IsFunction()); - return MakeCallback(cb_v.As(), argc, argv); -} - - -inline v8::MaybeLocal AsyncWrap::MakeCallback( - uint32_t index, - int argc, - v8::Local* argv) { - v8::Local cb_v = object()->Get(index); - CHECK(cb_v->IsFunction()); + v8::Local cb_v; + if (!object()->Get(env()->context(), symbol).ToLocal(&cb_v)) + return v8::MaybeLocal(); + if (!cb_v->IsFunction()) { + // TODO(addaleax): We should throw an error here to fulfill the + // `MaybeLocal<>` API contract. + return v8::MaybeLocal(); + } return MakeCallback(cb_v.As(), argc, argv); } diff --git a/src/async_wrap.h b/src/async_wrap.h index f696facd485188..82c57910925be9 100644 --- a/src/async_wrap.h +++ b/src/async_wrap.h @@ -173,9 +173,6 @@ class AsyncWrap : public BaseObject { const v8::Local symbol, int argc, v8::Local* argv); - inline v8::MaybeLocal MakeCallback(uint32_t index, - int argc, - v8::Local* argv); virtual size_t self_size() const = 0; virtual std::string diagnostic_name() const; diff --git a/src/handle_wrap.h b/src/handle_wrap.h index 4e177d249f28b5..bd7ef4000bad6f 100644 --- a/src/handle_wrap.h +++ b/src/handle_wrap.h @@ -83,6 +83,10 @@ class HandleWrap : public AsyncWrap { void MarkAsInitialized(); void MarkAsUninitialized(); + inline bool IsHandleClosing() const { + return state_ == kClosing || state_ == kClosed; + } + private: friend class Environment; friend void GetActiveHandles(const v8::FunctionCallbackInfo&); diff --git a/src/node_messaging.cc b/src/node_messaging.cc index 352749ea48f483..407557d4f4ae13 100644 --- a/src/node_messaging.cc +++ b/src/node_messaging.cc @@ -391,9 +391,21 @@ uv_async_t* MessagePort::async() { } void MessagePort::TriggerAsync() { + if (IsHandleClosing()) return; CHECK_EQ(uv_async_send(async()), 0); } +void MessagePort::Close(v8::Local close_callback) { + if (data_) { + // Wrap this call with accessing the mutex, so that TriggerAsync() + // can check IsHandleClosing() without race conditions. + Mutex::ScopedLock sibling_lock(data_->mutex_); + HandleWrap::Close(close_callback); + } else { + HandleWrap::Close(close_callback); + } +} + void MessagePort::New(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); if (!args.IsConstructCall()) { @@ -476,7 +488,6 @@ void MessagePort::OnMessage() { }; if (args[0].IsEmpty() || - !object()->Has(context, env()->onmessage_string()).FromMaybe(false) || MakeCallback(env()->onmessage_string(), 1, args).IsEmpty()) { // Re-schedule OnMessage() execution in case of failure. if (data_) diff --git a/src/node_messaging.h b/src/node_messaging.h index 9a13437d19a331..28122c526cccea 100644 --- a/src/node_messaging.h +++ b/src/node_messaging.h @@ -154,6 +154,8 @@ class MessagePort : public HandleWrap { std::unique_ptr Detach(); bool IsSiblingClosed() const; + void Close( + v8::Local close_callback = v8::Local()) override; size_t self_size() const override; diff --git a/test/parallel/test-async-wrap-missing-method.js b/test/parallel/test-async-wrap-missing-method.js new file mode 100644 index 00000000000000..72d6d9fdf3fac4 --- /dev/null +++ b/test/parallel/test-async-wrap-missing-method.js @@ -0,0 +1,49 @@ +// Flags: --experimental-worker +'use strict'; +const common = require('../common'); +const assert = require('assert'); + +const { MessageChannel } = require('worker_threads'); + +{ + const { port1, port2 } = new MessageChannel(); + + // Returning a non-function in the getter should not crash. + Object.defineProperty(port1, 'onmessage', { + get() { + port1.unref(); + return 42; + } + }); + + port2.postMessage({ foo: 'bar' }); + + // We need to start the port manually because .onmessage assignment tracking + // has been overridden. + port1.start(); + port1.ref(); +} + +{ + const err = new Error('eyecatcher'); + process.on('uncaughtException', common.mustCall((exception) => { + port1.unref(); + assert.strictEqual(exception, err); + })); + + const { port1, port2 } = new MessageChannel(); + + // Throwing in the getter should not crash. + Object.defineProperty(port1, 'onmessage', { + get() { + throw err; + } + }); + + port2.postMessage({ foo: 'bar' }); + + // We need to start the port manually because .onmessage assignment tracking + // has been overridden. + port1.start(); + port1.ref(); +}