From 5b8e46dd29773e2115fac18372ef073138444c05 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Sun, 7 Jan 2018 22:07:13 +0100
Subject: [PATCH] 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++.

Refs: https://github.com/nodejs/node/pull/17938#issuecomment-354683850
PR-URL: https://github.com/nodejs/node/pull/18028
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
---
 src/js_stream.cc                              | 57 ++++++++++++++-----
 .../test-wrap-js-stream-exceptions.js         | 19 +++++++
 2 files changed, 63 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 e8e31f41cb64ad..ac51d38a728f81 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;
 
 
@@ -98,24 +98,41 @@ 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 true;
+  }
+  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 = UV_EPROTO;
+  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 = UV_EPROTO;
+  if (!MakeCallback(env()->onreadstop_string(), 0, nullptr).ToLocal(&value) ||
+      !value->Int32Value(env()->context()).To(&value_int)) {
+    FatalException(env()->isolate(), try_catch);
+  }
+  return value_int;
 }
 
 
@@ -128,10 +145,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 = UV_EPROTO;
+  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;
 }
 
 
@@ -157,10 +181,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 = UV_EPROTO;
+  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..57ecd70189d106
--- /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!');
+  })
+}));
+
+assert.throws(() => socket.end('foo'), /Error: write EPROTO/);