From 7707494bc56ba0282e149a1f8dd0eb872ea3b56c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 7 Jan 2018 22:07:13 +0100 Subject: [PATCH 1/2] src: harden JSStream callbacks Since these are executing JS code, and in particular parts of that code may be provided by userland, handle such exceptions in C++. Ref: https://github.com/nodejs/node/pull/17938#issuecomment-354683850 --- src/js_stream.cc | 56 ++++++++++++++----- .../test-wrap-js-stream-exceptions.js | 19 +++++++ 2 files changed, 62 insertions(+), 13 deletions(-) create mode 100644 test/parallel/test-wrap-js-stream-exceptions.js diff --git a/src/js_stream.cc b/src/js_stream.cc index dba6d1a52b8013..45d6bd33d5b755 100644 --- a/src/js_stream.cc +++ b/src/js_stream.cc @@ -14,9 +14,9 @@ using v8::FunctionCallbackInfo; using v8::FunctionTemplate; using v8::HandleScope; using v8::Local; -using v8::MaybeLocal; using v8::Object; using v8::String; +using v8::TryCatch; using v8::Value; @@ -87,24 +87,40 @@ bool JSStream::IsAlive() { bool JSStream::IsClosing() { HandleScope scope(env()->isolate()); Context::Scope context_scope(env()->context()); - return MakeCallback(env()->isclosing_string(), 0, nullptr) - .ToLocalChecked()->IsTrue(); + TryCatch try_catch(env()->isolate()); + Local value; + if (!MakeCallback(env()->isclosing_string(), 0, nullptr).ToLocal(&value)) { + FatalException(env()->isolate(), try_catch); + } + return value->IsTrue(); } int JSStream::ReadStart() { HandleScope scope(env()->isolate()); Context::Scope context_scope(env()->context()); - return MakeCallback(env()->onreadstart_string(), 0, nullptr) - .ToLocalChecked()->Int32Value(); + TryCatch try_catch(env()->isolate()); + Local value; + int value_int = 0; + if (!MakeCallback(env()->onreadstart_string(), 0, nullptr).ToLocal(&value) || + !value->Int32Value(env()->context()).To(&value_int)) { + FatalException(env()->isolate(), try_catch); + } + return value_int; } int JSStream::ReadStop() { HandleScope scope(env()->isolate()); Context::Scope context_scope(env()->context()); - return MakeCallback(env()->onreadstop_string(), 0, nullptr) - .ToLocalChecked()->Int32Value(); + TryCatch try_catch(env()->isolate()); + Local value; + int value_int = 0; + if (!MakeCallback(env()->onreadstop_string(), 0, nullptr).ToLocal(&value) || + !value->Int32Value(env()->context()).To(&value_int)) { + FatalException(env()->isolate(), try_catch); + } + return value_int; } @@ -117,10 +133,17 @@ int JSStream::DoShutdown(ShutdownWrap* req_wrap) { }; req_wrap->Dispatched(); - MaybeLocal res = - MakeCallback(env()->onshutdown_string(), arraysize(argv), argv); - return res.ToLocalChecked()->Int32Value(); + TryCatch try_catch(env()->isolate()); + Local value; + int value_int = 0; + if (!MakeCallback(env()->onshutdown_string(), + arraysize(argv), + argv).ToLocal(&value) || + !value->Int32Value(env()->context()).To(&value_int)) { + FatalException(env()->isolate(), try_catch); + } + return value_int; } @@ -146,10 +169,17 @@ int JSStream::DoWrite(WriteWrap* w, }; w->Dispatched(); - MaybeLocal res = - MakeCallback(env()->onwrite_string(), arraysize(argv), argv); - return res.ToLocalChecked()->Int32Value(); + TryCatch try_catch(env()->isolate()); + Local value; + int value_int = 0; + if (!MakeCallback(env()->onwrite_string(), + arraysize(argv), + argv).ToLocal(&value) || + !value->Int32Value(env()->context()).To(&value_int)) { + FatalException(env()->isolate(), try_catch); + } + return value_int; } diff --git a/test/parallel/test-wrap-js-stream-exceptions.js b/test/parallel/test-wrap-js-stream-exceptions.js new file mode 100644 index 00000000000000..bf2b82de49515e --- /dev/null +++ b/test/parallel/test-wrap-js-stream-exceptions.js @@ -0,0 +1,19 @@ +// Flags: --expose-internals +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const JSStreamWrap = require('internal/wrap_js_stream'); +const { Duplex } = require('stream'); + +process.once('uncaughtException', common.mustCall((err) => { + assert.strictEqual(err.message, 'exception!'); +})); + +const socket = new JSStreamWrap(new Duplex({ + read: common.mustCall(), + write: common.mustCall((buffer, data, cb) => { + throw new Error('exception!'); + }) +})); + +socket.end('foo'); From 9b1e0e963741b8d8103221ae7bf817a3187ab4ec Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 7 Jan 2018 23:01:58 +0100 Subject: [PATCH 2/2] [squash] handle return from FatalException() --- src/js_stream.cc | 9 +++++---- test/parallel/test-wrap-js-stream-exceptions.js | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/js_stream.cc b/src/js_stream.cc index 45d6bd33d5b755..7d1115f12ac3e2 100644 --- a/src/js_stream.cc +++ b/src/js_stream.cc @@ -91,6 +91,7 @@ bool JSStream::IsClosing() { Local value; if (!MakeCallback(env()->isclosing_string(), 0, nullptr).ToLocal(&value)) { FatalException(env()->isolate(), try_catch); + return true; } return value->IsTrue(); } @@ -101,7 +102,7 @@ int JSStream::ReadStart() { Context::Scope context_scope(env()->context()); TryCatch try_catch(env()->isolate()); Local value; - int value_int = 0; + int value_int = UV_EPROTO; if (!MakeCallback(env()->onreadstart_string(), 0, nullptr).ToLocal(&value) || !value->Int32Value(env()->context()).To(&value_int)) { FatalException(env()->isolate(), try_catch); @@ -115,7 +116,7 @@ int JSStream::ReadStop() { Context::Scope context_scope(env()->context()); TryCatch try_catch(env()->isolate()); Local value; - int value_int = 0; + int value_int = UV_EPROTO; if (!MakeCallback(env()->onreadstop_string(), 0, nullptr).ToLocal(&value) || !value->Int32Value(env()->context()).To(&value_int)) { FatalException(env()->isolate(), try_catch); @@ -136,7 +137,7 @@ int JSStream::DoShutdown(ShutdownWrap* req_wrap) { TryCatch try_catch(env()->isolate()); Local value; - int value_int = 0; + int value_int = UV_EPROTO; if (!MakeCallback(env()->onshutdown_string(), arraysize(argv), argv).ToLocal(&value) || @@ -172,7 +173,7 @@ int JSStream::DoWrite(WriteWrap* w, TryCatch try_catch(env()->isolate()); Local value; - int value_int = 0; + int value_int = UV_EPROTO; if (!MakeCallback(env()->onwrite_string(), arraysize(argv), argv).ToLocal(&value) || diff --git a/test/parallel/test-wrap-js-stream-exceptions.js b/test/parallel/test-wrap-js-stream-exceptions.js index bf2b82de49515e..57ecd70189d106 100644 --- a/test/parallel/test-wrap-js-stream-exceptions.js +++ b/test/parallel/test-wrap-js-stream-exceptions.js @@ -16,4 +16,4 @@ const socket = new JSStreamWrap(new Duplex({ }) })); -socket.end('foo'); +assert.throws(() => socket.end('foo'), /Error: write EPROTO/);