Add "kind" to raiseStackOverflow; increase native stack depth limit.

Summary: Oleg reported the stack overflow detailed in the task. This diff does two things: 1) Increases the native stack depth limit. This allows Oleg's failing execution succeed. 2) Adds a "kind" argument to raiseStackOverflow. This causes different kinds of overflows to get different JS exception messgages, distinguishing between exhausting the runtime's register stack, using up the native stack in which Hermes is executing, and a limit on the depth of JSON expressions being parsed. I added (2) because it would have been really useful in debugging this crash. I had to instrument all call sites to runtime->raiseStackOverflow() to figure this out. (Oleg was running things for me, because it only repro'd for him, and I couldn't very easily ask him to use agdb...) Reviewed By: avp Differential Revision: D16231409 fbshipit-source-id: 852543f44b54ab7dcd15d944f4fcff30d9330a38
snowladygirl2013 · Jul 13, 2019 · 5ff757c · 5ff757c
1 parent a4b8642
commit 5ff757c
Show file tree

Hide file tree

Showing 10 changed files with 61 additions and 29 deletions.
diff --git a/API/hermes/hermes.cpp b/API/hermes/hermes.cpp
@@ -1715,7 +1715,8 @@ jsi::Value HermesRuntimeImpl::call(
                                        vm::HermesValue::encodeUndefinedValue(),
                                        hvFromValue(jsThis)};
     if (LLVM_UNLIKELY(newFrame.overflowed())) {
-      checkStatus(runtime_.raiseStackOverflow());
+      checkStatus(runtime_.raiseStackOverflow(
+          ::hermes::vm::StackRuntime::StackOverflowKind::NativeStack));
     }
 
     for (uint32_t i = 0; i != count; ++i) {
@@ -1791,7 +1792,8 @@ jsi::Value HermesRuntimeImpl::callAsConstructor(
                                        funcHandle.getHermesValue(),
                                        objHandle.getHermesValue()};
     if (newFrame.overflowed()) {
-      checkStatus(runtime_.raiseStackOverflow());
+      checkStatus(runtime_.raiseStackOverflow(
+          ::hermes::vm::StackRuntime::StackOverflowKind::NativeStack));
     }
     for (uint32_t i = 0; i != count; ++i) {
       newFrame->getArgRef(i) = hvFromValue(args[i]);

diff --git a/include/hermes/VM/Callable.h b/include/hermes/VM/Callable.h
@@ -454,7 +454,8 @@ class NativeFunction : public Callable {
       Runtime *runtime) {
     ScopedNativeDepthTracker depthTracker{runtime};
     if (LLVM_UNLIKELY(depthTracker.overflowed())) {
-      return runtime->raiseStackOverflow();
+      return runtime->raiseStackOverflow(
+          Runtime::StackOverflowKind::NativeStack);
     }
 
     auto newFrame = runtime->setCurrentFrameToTopOfStack();

diff --git a/include/hermes/VM/Runtime.h b/include/hermes/VM/Runtime.h
@@ -514,9 +514,21 @@ class Runtime : public HandleRootOwner,
   ExecutionStatus raiseURIError(const TwineChar16 &msg);
 
   /// Raise a stack overflow exception. This is special because constructing
-  /// the object must not execute any custom or JavaScript code.
+  /// the object must not execute any custom or JavaScript code.  The
+  /// argument influences the exception's message, to aid debugging.
   /// \return ExecutionStatus::EXCEPTION
-  ExecutionStatus raiseStackOverflow();
+  enum class StackOverflowKind {
+    // The JS register stack was exhausted.
+    JSRegisterStack,
+    // A limit on the number of native stack frames used in
+    // evaluation, intended to conservatively prevent native stack
+    // overflow, was exceeded.
+    NativeStack,
+    // RuntimeJSONParser has a maximum number of "nesting levels", and
+    // calls raiseStackOverflow if that is exceeded.
+    JSONParser,
+  };
+  ExecutionStatus raiseStackOverflow(StackOverflowKind kind);
 
   /// Raise an error for the quit function. This error is not catchable.
   ExecutionStatus raiseQuitError();
@@ -965,8 +977,11 @@ class Runtime : public HandleRootOwner,
   unsigned nativeCallFrameDepth_{0};
 
   /// A stack overflow exception is thrown when \c nativeCallFrameDepth_ exceeds
-  /// this threshold.
-  static constexpr unsigned MAX_NATIVE_CALL_FRAME_DEPTH = 256;
+  /// this threshold.  (This depth limit was originally 256, and we
+  /// increased when an app violated it.  The new depth is 128
+  /// larger.  See T46966147 for measurements/calculations indicating
+  /// that this limit should still insulate us from native stack overflow.)
+  static constexpr unsigned MAX_NATIVE_CALL_FRAME_DEPTH = 384;
 
   PinnedHermesValue thrownValue_{HermesValue::encodeEmptyValue()};
 

diff --git a/lib/VM/Callable.cpp b/lib/VM/Callable.cpp
@@ -182,7 +182,7 @@ CallResult<HermesValue> Callable::executeCall0(
                                      : HermesValue::encodeUndefinedValue(),
                                  *thisArgHandle};
   if (LLVM_UNLIKELY(newFrame.overflowed()))
-    return runtime->raiseStackOverflow();
+    return runtime->raiseStackOverflow(Runtime::StackOverflowKind::NativeStack);
   return call(selfHandle, runtime);
 }
 
@@ -202,7 +202,7 @@ CallResult<HermesValue> Callable::executeCall1(
                                      : HermesValue::encodeUndefinedValue(),
                                  *thisArgHandle};
   if (LLVM_UNLIKELY(newFrame.overflowed()))
-    return runtime->raiseStackOverflow();
+    return runtime->raiseStackOverflow(Runtime::StackOverflowKind::NativeStack);
   newFrame->getArgRef(0) = param1;
   return call(selfHandle, runtime);
 }
@@ -224,7 +224,7 @@ CallResult<HermesValue> Callable::executeCall2(
                                      : HermesValue::encodeUndefinedValue(),
                                  *thisArgHandle};
   if (LLVM_UNLIKELY(newFrame.overflowed()))
-    return runtime->raiseStackOverflow();
+    return runtime->raiseStackOverflow(Runtime::StackOverflowKind::NativeStack);
   newFrame->getArgRef(0) = param1;
   newFrame->getArgRef(1) = param2;
   return call(selfHandle, runtime);
@@ -248,7 +248,7 @@ CallResult<HermesValue> Callable::executeCall3(
                                      : HermesValue::encodeUndefinedValue(),
                                  *thisArgHandle};
   if (LLVM_UNLIKELY(newFrame.overflowed()))
-    return runtime->raiseStackOverflow();
+    return runtime->raiseStackOverflow(Runtime::StackOverflowKind::NativeStack);
   newFrame->getArgRef(0) = param1;
   newFrame->getArgRef(1) = param2;
   newFrame->getArgRef(2) = param3;
@@ -274,7 +274,7 @@ CallResult<HermesValue> Callable::executeCall4(
                                      : HermesValue::encodeUndefinedValue(),
                                  *thisArgHandle};
   if (LLVM_UNLIKELY(newFrame.overflowed()))
-    return runtime->raiseStackOverflow();
+    return runtime->raiseStackOverflow(Runtime::StackOverflowKind::NativeStack);
   newFrame->getArgRef(0) = param1;
   newFrame->getArgRef(1) = param2;
   newFrame->getArgRef(2) = param3;
@@ -573,7 +573,7 @@ CallResult<HermesValue> BoundFunction::_boundCall(
     Runtime *runtime) {
   ScopedNativeDepthTracker depthTracker{runtime};
   if (LLVM_UNLIKELY(depthTracker.overflowed())) {
-    return runtime->raiseStackOverflow();
+    return runtime->raiseStackOverflow(Runtime::StackOverflowKind::NativeStack);
   }
 
   CallResult<HermesValue> res{ExecutionStatus::EXCEPTION};
@@ -618,7 +618,8 @@ CallResult<HermesValue> BoundFunction::_boundCall(
           runtime->getStackPointer() == originalCalleeFrame.ptr() &&
           "Stack wasn't restored properly");
 
-      runtime->raiseStackOverflow();
+      runtime->raiseStackOverflow(Runtime::StackOverflowKind::JSRegisterStack);
+
       res = ExecutionStatus::EXCEPTION;
       goto bail;
     }

diff --git a/lib/VM/Interpreter.cpp b/lib/VM/Interpreter.cpp
@@ -890,7 +890,7 @@ CallResult<HermesValue> Interpreter::interpretFunction(
 
   ScopedNativeDepthTracker depthTracker{runtime};
   if (LLVM_UNLIKELY(depthTracker.overflowed())) {
-    return runtime->raiseStackOverflow();
+    return runtime->raiseStackOverflow(Runtime::StackOverflowKind::NativeStack);
   }
 
   if (!SingleStep) {
@@ -3249,7 +3249,7 @@ CallResult<HermesValue> Interpreter::interpretFunction(
 
   // We arrive here if we couldn't allocate the registers for the current frame.
   stackOverflow:
-    runtime->raiseStackOverflow();
+    runtime->raiseStackOverflow(Runtime::StackOverflowKind::JSRegisterStack);
 
   // We arrive here when we raised an exception in a callee, but we don't want
   // the callee to be able to handle it.

diff --git a/lib/VM/JSLib/Function.cpp b/lib/VM/JSLib/Function.cpp
@@ -191,7 +191,8 @@ functionPrototypeApply(void *, Runtime *runtime, NativeArgs args) {
   if (args.getArg(1).isNull() || args.getArg(1).isUndefined()) {
     ScopedNativeCallFrame newFrame{runtime, 0, *func, false, args.getArg(0)};
     if (LLVM_UNLIKELY(newFrame.overflowed()))
-      return runtime->raiseStackOverflow();
+      return runtime->raiseStackOverflow(
+          Runtime::StackOverflowKind::NativeStack);
     return Callable::call(func, runtime);
   }
 
@@ -215,7 +216,7 @@ functionPrototypeApply(void *, Runtime *runtime, NativeArgs args) {
 
   ScopedNativeCallFrame newFrame{runtime, n, *func, false, args.getArg(0)};
   if (LLVM_UNLIKELY(newFrame.overflowed()))
-    return runtime->raiseStackOverflow();
+    return runtime->raiseStackOverflow(Runtime::StackOverflowKind::NativeStack);
 
   // Initialize the arguments to undefined because we might allocate and cause
   // a gc while populating them.
@@ -275,7 +276,7 @@ functionPrototypeCall(void *, Runtime *runtime, NativeArgs args) {
   ScopedNativeCallFrame newFrame{
       runtime, argCount ? argCount - 1 : 0, *func, false, args.getArg(0)};
   if (LLVM_UNLIKELY(newFrame.overflowed()))
-    return runtime->raiseStackOverflow();
+    return runtime->raiseStackOverflow(Runtime::StackOverflowKind::NativeStack);
   for (uint32_t i = 1; i < argCount; ++i) {
     newFrame->getArgRef(i - 1) = args.getArg(i);
   }

diff --git a/lib/VM/JSLib/RegExp.cpp b/lib/VM/JSLib/RegExp.cpp
@@ -1549,14 +1549,16 @@ regExpPrototypeSymbolReplace(void *, Runtime *runtime, NativeArgs args) {
         // Arguments: matched, captures, position, S.
         size_t replacerArgsCount = 1 + nCaptures + 2;
         if (LLVM_UNLIKELY(replacerArgsCount >= UINT32_MAX))
-          return runtime->raiseStackOverflow();
+          return runtime->raiseStackOverflow(
+              Runtime::StackOverflowKind::JSRegisterStack);
         ScopedNativeCallFrame newFrame{runtime,
                                        static_cast<uint32_t>(replacerArgsCount),
                                        *replaceFn,
                                        false,
                                        HermesValue::encodeUndefinedValue()};
         if (LLVM_UNLIKELY(newFrame.overflowed()))
-          return runtime->raiseStackOverflow();
+          return runtime->raiseStackOverflow(
+              Runtime::StackOverflowKind::NativeStack);
 
         uint32_t argIdx = 0;
         newFrame->getArgRef(argIdx++) = matched.getHermesValue();

diff --git a/lib/VM/JSLib/RuntimeJSONUtils.cpp b/lib/VM/JSLib/RuntimeJSONUtils.cpp
@@ -263,7 +263,7 @@ CallResult<HermesValue> RuntimeJSONParser::parseValue() {
   llvm::SaveAndRestore<decltype(remainingDepth_)> oldDepth{remainingDepth_,
                                                            remainingDepth_ - 1};
   if (remainingDepth_ <= 0) {
-    return runtime_->raiseStackOverflow();
+    return runtime_->raiseStackOverflow(Runtime::StackOverflowKind::JSONParser);
   }
 
   MutableHandle<> returnValue{runtime_};
@@ -459,7 +459,7 @@ CallResult<HermesValue> RuntimeJSONParser::operationWalk(
   llvm::SaveAndRestore<decltype(remainingDepth_)> oldDepth{remainingDepth_,
                                                            remainingDepth_ - 1};
   if (remainingDepth_ <= 0) {
-    return runtime_->raiseStackOverflow();
+    return runtime_->raiseStackOverflow(Runtime::StackOverflowKind::JSONParser);
   }
 
   auto propRes = JSObject::getComputed_RJS(holder, runtime_, property);

diff --git a/lib/VM/Runtime.cpp b/lib/VM/Runtime.cpp
@@ -739,7 +739,7 @@ CallResult<HermesValue> Runtime::runBytecode(
     ScopedNativeCallFrame newFrame{
         this, 0, *funcRes, HermesValue::encodeUndefinedValue(), *thisArg};
     if (LLVM_UNLIKELY(newFrame.overflowed()))
-      return raiseStackOverflow();
+      return raiseStackOverflow(StackOverflowKind::NativeStack);
     return shouldRandomizeMemoryLayout_
         ? interpretFunctionWithRandomStack(this, globalCode)
         : interpretFunction(globalCode);
@@ -983,11 +983,21 @@ ExecutionStatus Runtime::raiseURIError(const TwineChar16 &msg) {
       this, Handle<JSObject>::vmcast(&URIErrorPrototype), msg);
 }
 
-ExecutionStatus Runtime::raiseStackOverflow() {
+ExecutionStatus Runtime::raiseStackOverflow(StackOverflowKind kind) {
+  const char *msg;
+  switch (kind) {
+    case StackOverflowKind::JSRegisterStack:
+      msg = "Maximum call stack size exceeded";
+      break;
+    case StackOverflowKind::NativeStack:
+      msg = "Maximum call stack size exceeded (native stack depth)";
+      break;
+    case StackOverflowKind::JSONParser:
+      msg = "Maximum nesting level in JSON parser exceeded";
+      break;
+  }
   return raisePlaceholder(
-      this,
-      Handle<JSObject>::vmcast(&RangeErrorPrototype),
-      "Maximum call stack size exceeded");
+      this, Handle<JSObject>::vmcast(&RangeErrorPrototype), msg);
 }
 
 ExecutionStatus Runtime::raiseQuitError() {

diff --git a/unittests/VMRuntime/NativeFrameTest.cpp b/unittests/VMRuntime/NativeFrameTest.cpp
@@ -13,7 +13,7 @@ namespace {
 
 // This value matches the same field in Runtime, but we don't wish to make it
 // public in Runtime.
-constexpr unsigned MAX_NATIVE_CALL_FRAME_DEPTH = 256;
+constexpr unsigned MAX_NATIVE_CALL_FRAME_DEPTH = 384;
 
 // Count the number of native stack frames we can make before it reports
 // overflow. Also verify that our stack descends down.