From 6257b4044a181fcaa462aba6c361e3c8fdcad2c9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= <biwanczuk@gmail.com>
Date: Tue, 22 Oct 2019 16:49:58 +0200
Subject: [PATCH] core: gracefully handle bad op id (#3131)

---
 core/isolate.rs     | 36 +++++++++++++++++++++++++++++++++++-
 core/libdeno.rs     |  1 +
 core/libdeno/api.cc |  6 ++++++
 core/libdeno/deno.h |  2 ++
 core/ops.rs         | 18 +++++++++++++-----
 5 files changed, 57 insertions(+), 6 deletions(-)

diff --git a/core/isolate.rs b/core/isolate.rs
index ede07b1c5f..8b15befa30 100644
--- a/core/isolate.rs
+++ b/core/isolate.rs
@@ -311,12 +311,19 @@ impl Isolate {
   ) {
     let isolate = unsafe { Isolate::from_raw_ptr(user_data) };
 
-    let op = isolate.op_registry.call(
+    let maybe_op = isolate.op_registry.call(
       op_id,
       control_buf.as_ref(),
       PinnedBuf::new(zero_copy_buf),
     );
 
+    let op = match maybe_op {
+      Some(op) => op,
+      None => {
+        return isolate.throw_exception(&format!("Unknown op id: {}", op_id))
+      }
+    };
+
     // To avoid latency problems we eagerly poll 50 futures and then
     // allow to poll ops from `pending_ops`
     let op = if isolate.eager_poll_count != 50 {
@@ -414,6 +421,13 @@ impl Isolate {
     }
   }
 
+  fn throw_exception(&mut self, exception_text: &str) {
+    let text = CString::new(exception_text).unwrap();
+    unsafe {
+      libdeno::deno_throw_exception(self.libdeno_isolate, text.as_ptr())
+    }
+  }
+
   fn respond(
     &mut self,
     maybe_buf: Option<(OpId, &[u8])>,
@@ -1418,6 +1432,26 @@ pub mod tests {
     });
   }
 
+  #[test]
+  fn test_pre_dispatch() {
+    run_in_task(|| {
+      let (mut isolate, _dispatch_count) = setup(Mode::OverflowResAsync);
+      js_check(isolate.execute(
+        "bad_op_id.js",
+        r#"
+          let thrown;
+          try {
+            Deno.core.dispatch(100, []);
+          } catch (e) {
+            thrown = e;
+          }
+          assert(thrown == "Unknown op id: 100");
+         "#,
+      ));
+      assert_eq!(Async::Ready(()), isolate.poll().unwrap());
+    });
+  }
+
   #[test]
   fn test_js() {
     run_in_task(|| {
diff --git a/core/libdeno.rs b/core/libdeno.rs
index c46880f1f3..a3bf0d66b1 100644
--- a/core/libdeno.rs
+++ b/core/libdeno.rs
@@ -266,6 +266,7 @@ extern "C" {
   pub fn deno_check_promise_errors(i: *const isolate);
   pub fn deno_lock(i: *const isolate);
   pub fn deno_unlock(i: *const isolate);
+  pub fn deno_throw_exception(i: *const isolate, text: *const c_char);
   pub fn deno_respond(
     i: *const isolate,
     user_data: *const c_void,
diff --git a/core/libdeno/api.cc b/core/libdeno/api.cc
index 061638cb53..332a1c5553 100644
--- a/core/libdeno/api.cc
+++ b/core/libdeno/api.cc
@@ -160,6 +160,12 @@ void deno_pinned_buf_delete(deno_pinned_buf* buf) {
   auto _ = deno::PinnedBuf(buf);
 }
 
+void deno_throw_exception(Deno* d_, const char* text) {
+  auto* d = unwrap(d_);
+  auto* isolate = d->isolate_;
+  isolate->ThrowException(deno::v8_str(text));
+}
+
 void deno_respond(Deno* d_, void* user_data, deno_op_id op_id, deno_buf buf) {
   auto* d = unwrap(d_);
   if (d->current_args_ != nullptr) {
diff --git a/core/libdeno/deno.h b/core/libdeno/deno.h
index 0bdd31f506..946978e3b2 100644
--- a/core/libdeno/deno.h
+++ b/core/libdeno/deno.h
@@ -110,6 +110,8 @@ void deno_execute(Deno* d, void* user_data, const char* js_filename,
 // If a JS exception was encountered, deno_last_exception() will be non-NULL.
 void deno_respond(Deno* d, void* user_data, deno_op_id op_id, deno_buf buf);
 
+void deno_throw_exception(Deno* d, const char* text);
+
 // consumes zero_copy
 void deno_pinned_buf_delete(deno_pinned_buf* buf);
 
diff --git a/core/ops.rs b/core/ops.rs
index 84c15e096c..cce454348b 100644
--- a/core/ops.rs
+++ b/core/ops.rs
@@ -63,21 +63,26 @@ impl OpRegistry {
     op_map_json.as_bytes().to_owned().into_boxed_slice()
   }
 
+  /// This function returns None only if op with given id doesn't exist in registry.
   pub fn call(
     &self,
     op_id: OpId,
     control: &[u8],
     zero_copy_buf: Option<PinnedBuf>,
-  ) -> CoreOp {
+  ) -> Option<CoreOp> {
     // Op with id 0 has special meaning - it's a special op that is always
     // provided to retrieve op id map. The map consists of name to `OpId`
     // mappings.
     if op_id == 0 {
-      return Op::Sync(self.json_map());
+      return Some(Op::Sync(self.json_map()));
     }
 
-    let d = &*self.dispatchers.get(op_id as usize).expect("Op not found!");
-    d(control, zero_copy_buf)
+    let d = match self.dispatchers.get(op_id as usize) {
+      Some(handler) => &*handler,
+      None => return None,
+    };
+
+    Some(d(control, zero_copy_buf))
   }
 }
 
@@ -101,11 +106,14 @@ fn test_op_registry() {
   expected.insert("test".to_string(), 1);
   assert_eq!(op_registry.name_to_id, expected);
 
-  let res = op_registry.call(test_id, &[], None);
+  let res = op_registry.call(test_id, &[], None).unwrap();
   if let Op::Sync(buf) = res {
     assert_eq!(buf.len(), 0);
   } else {
     unreachable!();
   }
   assert_eq!(c.load(atomic::Ordering::SeqCst), 1);
+
+  let res = op_registry.call(100, &[], None);
+  assert!(res.is_none());
 }