From 9baf6f2140ffc26d043f9717a1ea49fd0e08eed8 Mon Sep 17 00:00:00 2001 From: Marc Horowitz Date: Wed, 3 Jun 2020 18:11:35 -0700 Subject: [PATCH] Handle stack overflow in JSError construction gracefully Summary: JSError creation can lead to further errors. Make sure these cases are handled and don't cause weird crashes or other issues. This solution has a few parts: * include a ScopedNativeDepthTracker in checkStatus * If an exception object message or stack property is already a String, don't call JS String ctor on it * Verify that a jsi::Value is a String before calling getString on it. * Add more tests for JSError construction Changelog: [Internal] Reviewed By: dulinriley Differential Revision: D21851645 fbshipit-source-id: 2d10da0e741ad4ede93cd806320f68ad512e5138 --- ReactCommon/jsi/jsi/jsi.cpp | 60 +++++++++++++++------- ReactCommon/jsi/jsi/test/testlib.cpp | 77 +++++++++++++++++++++++----- 2 files changed, 106 insertions(+), 31 deletions(-) diff --git a/ReactCommon/jsi/jsi/jsi.cpp b/ReactCommon/jsi/jsi/jsi.cpp index 388f122c178..e4a7e431fca 100644 --- a/ReactCommon/jsi/jsi/jsi.cpp +++ b/ReactCommon/jsi/jsi/jsi.cpp @@ -401,32 +401,63 @@ JSError::JSError(std::string what, Runtime& rt, Value&& value) } void JSError::setValue(Runtime& rt, Value&& value) { - value_ = std::make_shared(std::move(value)); + value_ = std::make_shared(std::move(value)); try { if ((message_.empty() || stack_.empty()) && value_->isObject()) { auto obj = value_->getObject(rt); if (message_.empty()) { - jsi::Value message = obj.getProperty(rt, "message"); - if (!message.isUndefined()) { - message_ = - callGlobalFunction(rt, "String", message).getString(rt).utf8(rt); + try { + Value message = obj.getProperty(rt, "message"); + if (!message.isUndefined() && !message.isString()) { + message = callGlobalFunction(rt, "String", message); + } + if (message.isString()) { + message_ = message.getString(rt).utf8(rt); + } else if (!message.isUndefined()) { + message_ = "String(e.message) is a " + kindToString(message, &rt); + } + } catch (const std::exception& ex) { + message_ = std::string("[Exception while creating message string: ") + + ex.what() + "]"; } } if (stack_.empty()) { - jsi::Value stack = obj.getProperty(rt, "stack"); - if (!stack.isUndefined()) { - stack_ = - callGlobalFunction(rt, "String", stack).getString(rt).utf8(rt); + try { + Value stack = obj.getProperty(rt, "stack"); + if (!stack.isUndefined() && !stack.isString()) { + stack = callGlobalFunction(rt, "String", stack); + } + if (stack.isString()) { + stack_ = stack.getString(rt).utf8(rt); + } else if (!stack.isUndefined()) { + stack_ = "String(e.stack) is a " + kindToString(stack, &rt); + } + } catch (const std::exception& ex) { + message_ = std::string("[Exception while creating stack string: ") + + ex.what() + "]"; } } } if (message_.empty()) { - message_ = - callGlobalFunction(rt, "String", *value_).getString(rt).utf8(rt); + try { + if (value_->isString()) { + message_ = value_->getString(rt).utf8(rt); + } else { + Value message = callGlobalFunction(rt, "String", *value_); + if (message.isString()) { + message_ = message.getString(rt).utf8(rt); + } else { + message_ = "String(e) is a " + kindToString(message, &rt); + } + } + } catch (const std::exception& ex) { + message_ = std::string("[Exception while creating message string: ") + + ex.what() + "]"; + } } if (stack_.empty()) { @@ -436,13 +467,6 @@ void JSError::setValue(Runtime& rt, Value&& value) { if (what_.empty()) { what_ = message_ + "\n\n" + stack_; } - } catch (const std::exception& ex) { - message_ = std::string("[Exception while creating message string: ") + - ex.what() + "]"; - stack_ = std::string("Exception while creating stack string: ") + - ex.what() + "]"; - what_ = - std::string("Exception while getting value fields: ") + ex.what() + "]"; } catch (...) { message_ = "[Exception caught creating message string]"; stack_ = "[Exception caught creating stack string]"; diff --git a/ReactCommon/jsi/jsi/test/testlib.cpp b/ReactCommon/jsi/jsi/test/testlib.cpp index a8573d6ebb6..996c2cb614a 100644 --- a/ReactCommon/jsi/jsi/test/testlib.cpp +++ b/ReactCommon/jsi/jsi/test/testlib.cpp @@ -981,19 +981,6 @@ TEST_P(JSITest, JSErrorsCanBeConstructedWithStack) { } TEST_P(JSITest, JSErrorDoesNotInfinitelyRecurse) { - Value globalString = rt.global().getProperty(rt, "String"); - rt.global().setProperty(rt, "String", Value::undefined()); - try { - eval("throw Error('whoops')"); - FAIL() << "expected exception"; - } catch (const JSError& ex) { - EXPECT_EQ( - ex.getMessage(), - "[Exception while creating message string: callGlobalFunction: " - "JS global property 'String' is undefined, expected a Function]"); - } - rt.global().setProperty(rt, "String", globalString); - Value globalError = rt.global().getProperty(rt, "Error"); rt.global().setProperty(rt, "Error", Value::undefined()); try { @@ -1272,6 +1259,70 @@ TEST_P(JSITest, SymbolTest) { EXPECT_FALSE(Value::strictEquals(rt, eval("Symbol('a')"), eval("'a'"))); } +TEST_P(JSITest, JSErrorTest) { + // JSError creation can lead to further errors. Make sure these + // cases are handled and don't cause weird crashes or other issues. + // + // Getting message property can throw + + EXPECT_THROW( + eval("var GetMessageThrows = {get message() { throw Error('ex'); }};" + "throw GetMessageThrows;"), + JSIException); + + EXPECT_THROW( + eval("var GetMessageThrows = {get message() { throw GetMessageThrows; }};" + "throw GetMessageThrows;"), + JSIException); + + // Converting exception message to String can throw + + EXPECT_THROW( + eval( + "Object.defineProperty(" + " globalThis, 'String', {configurable:true, get() { var e = Error(); e.message = 23; throw e; }});" + "var e = Error();" + "e.message = 17;" + "throw e;"), + JSIException); + + EXPECT_THROW( + eval( + "var e = Error();" + "Object.defineProperty(" + " e, 'message', {configurable:true, get() { throw Error('getter'); }});" + "throw e;"), + JSIException); + + EXPECT_THROW( + eval("var e = Error();" + "String = function() { throw Error('ctor'); };" + "throw e;"), + JSIException); + + // Converting an exception message to String can return a non-String + + EXPECT_THROW( + eval("String = function() { return 42; };" + "var e = Error();" + "e.message = 17;" + "throw e;"), + JSIException); + + // Exception can be non-Object + + EXPECT_THROW(eval("throw 17;"), JSIException); + + EXPECT_THROW(eval("throw undefined;"), JSIException); + + // Converting exception with no message or stack property to String can throw + + EXPECT_THROW( + eval("var e = {toString() { throw new Error('errstr'); }};" + "throw e;"), + JSIException); +} + //---------------------------------------------------------------------- // Test that multiple levels of delegation in DecoratedHostObjects works.