From d2f5044c379b2152ab68ffe5a3d3cdad6109c4de Mon Sep 17 00:00:00 2001
From: John Porto <jpporto@fb.com>
Date: Fri, 28 Jan 2022 13:37:12 -0800
Subject: [PATCH] Catch exceptions thrown by user callbacks

Summary:
Calls to Inspector::evaluate() and Inspector::executeIfEnabled()
take a user-provided callback which are not try-guarded. This
means that, should the user code throw, the inspector's process
will likely die due to the unhandled exception. This change
makes sure that, if the user provided callback throws, then
the promise attached to those methods will be fulfilled by
an exception.

Change: [internal]

Reviewed By: avp

Differential Revision: D33852073

fbshipit-source-id: 7fbb6662b28d393a5d5b494c004aa9521e23ebb6
---
 ReactCommon/hermes/inspector/Exceptions.h     |  6 +++++
 ReactCommon/hermes/inspector/Inspector.cpp    |  7 ++++--
 ReactCommon/hermes/inspector/Inspector.h      | 14 +++++++++++
 .../hermes/inspector/InspectorState.cpp       | 24 ++++++++++++-------
 4 files changed, 41 insertions(+), 10 deletions(-)

diff --git a/ReactCommon/hermes/inspector/Exceptions.h b/ReactCommon/hermes/inspector/Exceptions.h
index 134fe282919..928b795448e 100644
--- a/ReactCommon/hermes/inspector/Exceptions.h
+++ b/ReactCommon/hermes/inspector/Exceptions.h
@@ -47,6 +47,12 @@ class MultipleCommandsPendingException : public std::runtime_error {
             ": a step or resume is already pending") {}
 };
 
+class UserCallbackException : public std::runtime_error {
+ public:
+  UserCallbackException(const std::exception &e)
+      : std::runtime_error(std::string("callback exception: ") + e.what()) {}
+};
+
 } // namespace inspector
 } // namespace hermes
 } // namespace facebook
diff --git a/ReactCommon/hermes/inspector/Inspector.cpp b/ReactCommon/hermes/inspector/Inspector.cpp
index efdba70c6b2..8ec9f82b6b8 100644
--- a/ReactCommon/hermes/inspector/Inspector.cpp
+++ b/ReactCommon/hermes/inspector/Inspector.cpp
@@ -589,8 +589,11 @@ void Inspector::executeIfEnabledOnExecutor(
 
   state_->pushPendingFunc(
       [wrappedFunc = std::move(wrappedFunc), promise]() mutable {
-        wrappedFunc();
-        promise->setValue();
+        if (auto userCallbackException = runUserCallback(wrappedFunc)) {
+          promise->setException(*userCallbackException);
+        } else {
+          promise->setValue();
+        }
       });
 }
 
diff --git a/ReactCommon/hermes/inspector/Inspector.h b/ReactCommon/hermes/inspector/Inspector.h
index dc85d90d646..648499207f6 100644
--- a/ReactCommon/hermes/inspector/Inspector.h
+++ b/ReactCommon/hermes/inspector/Inspector.h
@@ -15,11 +15,13 @@
 #include <unordered_map>
 
 #include <folly/Executor.h>
+#include <folly/Optional.h>
 #include <folly/Unit.h>
 #include <folly/futures/Future.h>
 #include <hermes/DebuggerAPI.h>
 #include <hermes/hermes.h>
 #include <hermes/inspector/AsyncPauseState.h>
+#include <hermes/inspector/Exceptions.h>
 #include <hermes/inspector/RuntimeAdapter.h>
 
 namespace facebook {
@@ -363,6 +365,18 @@ class Inspector : public facebook::hermes::debugger::EventObserver,
   std::unique_ptr<folly::Executor> executor_;
 };
 
+/// Helper function that guards user code execution in a try-catch block.
+template <typename C, typename... A>
+folly::Optional<UserCallbackException> runUserCallback(C &cb, A &&...arg) {
+  try {
+    cb(std::forward<A>(arg)...);
+  } catch (const std::exception &e) {
+    return UserCallbackException(e);
+  }
+
+  return {};
+}
+
 } // namespace inspector
 } // namespace hermes
 } // namespace facebook
diff --git a/ReactCommon/hermes/inspector/InspectorState.cpp b/ReactCommon/hermes/inspector/InspectorState.cpp
index a318180a435..8ee53e9005c 100644
--- a/ReactCommon/hermes/inspector/InspectorState.cpp
+++ b/ReactCommon/hermes/inspector/InspectorState.cpp
@@ -249,10 +249,14 @@ std::pair<NextStatePtr, CommandPtr> InspectorState::Running::didPause(
   } else if (reason == debugger::PauseReason::EvalComplete) {
     assert(pendingEvalPromise_);
 
-    pendingEvalResultTransformer_(
-        inspector_.debugger_.getProgramState().getEvalResult());
-    pendingEvalPromise_->setValue(
-        inspector_.debugger_.getProgramState().getEvalResult());
+    if (auto userCallbackException = runUserCallback(
+            pendingEvalResultTransformer_,
+            inspector_.debugger_.getProgramState().getEvalResult())) {
+      pendingEvalPromise_->setException(*userCallbackException);
+    } else {
+      pendingEvalPromise_->setValue(
+          inspector_.debugger_.getProgramState().getEvalResult());
+    }
     pendingEvalPromise_.reset();
   } else if (
       reason == debugger::PauseReason::Breakpoint &&
@@ -364,10 +368,14 @@ std::pair<NextStatePtr, CommandPtr> InspectorState::Paused::didPause(
       break;
     case debugger::PauseReason::EvalComplete: {
       assert(pendingEvalPromise_);
-      pendingEvalResultTransformer_(
-          inspector_.debugger_.getProgramState().getEvalResult());
-      pendingEvalPromise_->setValue(
-          inspector_.debugger_.getProgramState().getEvalResult());
+      if (auto userCallbackException = runUserCallback(
+              pendingEvalResultTransformer_,
+              inspector_.debugger_.getProgramState().getEvalResult())) {
+        pendingEvalPromise_->setException(*userCallbackException);
+      } else {
+        pendingEvalPromise_->setValue(
+            inspector_.debugger_.getProgramState().getEvalResult());
+      }
       pendingEvalPromise_.reset();
     } break;
     case debugger::PauseReason::ScriptLoaded: