src: harden JSStream callbacks

addaleax · addaleax · commit 7707494bc56b · 2018-01-07T22:07:13.000+01:00
Since these are executing JS code, and in particular parts of that code may be provided by userland, handle such exceptions in C++. Ref: nodejs#17938 (comment)
diff --git 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> 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> 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> 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<Value> res =
-      MakeCallback(env()->onshutdown_string(), arraysize(argv), argv);
 
-  return res.ToLocalChecked()->Int32Value();
+  TryCatch try_catch(env()->isolate());
+  Local<Value> 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<Value> res =
-      MakeCallback(env()->onwrite_string(), arraysize(argv), argv);
 
-  return res.ToLocalChecked()->Int32Value();
+  TryCatch try_catch(env()->isolate());
+  Local<Value> 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
@@ -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');
