[MERGE chakra-core#5529 @VSadov] Avoid deep recursion in GetTypeOfString when dealing with proxies

VSadov · VSadov · commit 9d3835c9dd99 · 2018-07-31T13:10:53.000-07:00
Merge pull request chakra-core#5529 from VSadov:fix18260402 It is unusual, but possible that proxies contain proxies and could be deeply nested. Recursion over such objects causes stack overflow and should be avoided. With this fix we will unwrap proxies to the inermost proxy without recursing. Fixes: OS:18260402 Found by OSS-Fuzz
diff --git a/lib/Runtime/Library/JavascriptProxy.cpp b/lib/Runtime/Library/JavascriptProxy.cpp
@@ -1621,16 +1621,18 @@ namespace Js
 
     BOOL JavascriptProxy::GetDiagTypeString(StringBuilder<ArenaAllocator>* stringBuilder, ScriptContext* requestContext)
     {
+        const JavascriptProxy* proxy = UnwrapNestedProxies(this);
+
         //RecyclableObject* targetObj;
-        if (this->handler == nullptr)
+        if (proxy->handler == nullptr)
         {
             ThreadContext* threadContext = GetScriptContext()->GetThreadContext();
             // the proxy has been revoked; TypeError.
             if (!threadContext->RecordImplicitException())
                 return FALSE;
             JavascriptError::ThrowTypeError(GetScriptContext(), JSERR_ErrorOnRevokedProxy, _u("getTypeString"));
         }
-        return target->GetDiagTypeString(stringBuilder, requestContext);
+        return proxy->target->GetDiagTypeString(stringBuilder, requestContext);
     }
 
     RecyclableObject* JavascriptProxy::ToObject(ScriptContext * requestContext)
@@ -1649,7 +1651,9 @@ namespace Js
 
     Var JavascriptProxy::GetTypeOfString(ScriptContext* requestContext)
     {
-        if (this->handler == nullptr)
+        const JavascriptProxy* proxy = UnwrapNestedProxies(this);
+
+        if (proxy->handler == nullptr)
         {
             // even if handler is nullptr, return typeof as "object"
             return requestContext->GetLibrary()->GetObjectTypeDisplayString();
@@ -1662,7 +1666,7 @@ namespace Js
         else
         {
             // handle nested cases recursively
-            return this->target->GetTypeOfString(requestContext);
+            return proxy->target->GetTypeOfString(requestContext);
         }
     }
 
diff --git a/lib/Runtime/Library/JavascriptProxy.h b/lib/Runtime/Library/JavascriptProxy.h
@@ -54,6 +54,26 @@ namespace Js
         static BOOL Is(_In_ RecyclableObject* obj);
         static JavascriptProxy* FromVar(Var obj) { AssertOrFailFast(Is(obj)); return static_cast<JavascriptProxy*>(obj); }
         static JavascriptProxy* UnsafeFromVar(Var obj) { Assert(Is(obj)); return static_cast<JavascriptProxy*>(obj); }
+
+        // before recursively calling something on 'target' use this helper in case there is nesting of proxies.
+        // the proxies could be deep nested and cause SO when processed recursively.
+        static const JavascriptProxy* UnwrapNestedProxies(const JavascriptProxy* proxy)
+        {
+            // continue while we have a proxy that is not revoked
+            while (proxy->handler != nullptr)
+            {
+                JavascriptProxy* nestedProxy = JavascriptOperators::TryFromVar<JavascriptProxy>(proxy->target);
+                if (nestedProxy == nullptr)
+                {
+                    break;
+                }
+
+                proxy = nestedProxy;
+            }
+
+            return proxy;
+        }
+
 #ifndef IsJsDiag
         RecyclableObject* GetTarget();
         RecyclableObject* GetHandler();
diff --git a/test/es6/ProxyInProxy.baseline b/test/es6/ProxyInProxy.baseline
@@ -112,3 +112,5 @@ function
 *** proxied function and Function.prototype.toString.call
 function a() { }
 function a() { }
+*** deeply nested proxy and typeof
+pass
diff --git a/test/es6/ProxyInProxy.js b/test/es6/ProxyInProxy.js
@@ -222,10 +222,27 @@ function test7() {
     // "function a() { }"
 }
 
+function test8() {
+    print("*** deeply nested proxy and typeof");
+
+    var nestedProxy = Proxy.revocable([], {}).proxy;
+    for (let i = 0; i < 1e5; i++) {
+        nestedProxy = new Proxy(nestedProxy, {});
+    }
+
+    (function () {
+        if (nestedProxy != null && typeof nestedProxy == "object")
+        {
+            console.log("pass");
+        }
+    })();
+}
+
 test1();
 test2();
 test3();
 test4();
 test5();
 test6();
 test7();
+test8();

Original file line number	Diff line number	Diff line change
`@@ -1621,16 +1621,18 @@ namespace Js`
`1621`	`1621`
`1622`	`1622`	`BOOL JavascriptProxy::GetDiagTypeString(StringBuilder<ArenaAllocator>* stringBuilder, ScriptContext* requestContext)`
`1623`	`1623`	`{`
	`1624`	`+ const JavascriptProxy* proxy = UnwrapNestedProxies(this);`
	`1625`	`+`
`1624`	`1626`	`//RecyclableObject* targetObj;`
`1625`		`- if (this->handler == nullptr)`
	`1627`	`+ if (proxy->handler == nullptr)`
`1626`	`1628`	`{`
`1627`	`1629`	`ThreadContext* threadContext = GetScriptContext()->GetThreadContext();`
`1628`	`1630`	`// the proxy has been revoked; TypeError.`
`1629`	`1631`	`if (!threadContext->RecordImplicitException())`
`1630`	`1632`	`return FALSE;`
`1631`	`1633`	`JavascriptError::ThrowTypeError(GetScriptContext(), JSERR_ErrorOnRevokedProxy, _u("getTypeString"));`
`1632`	`1634`	`}`
`1633`		`- return target->GetDiagTypeString(stringBuilder, requestContext);`
	`1635`	`+ return proxy->target->GetDiagTypeString(stringBuilder, requestContext);`
`1634`	`1636`	`}`
`1635`	`1637`
`1636`	`1638`	`RecyclableObject* JavascriptProxy::ToObject(ScriptContext * requestContext)`
`@@ -1649,7 +1651,9 @@ namespace Js`
`1649`	`1651`
`1650`	`1652`	`Var JavascriptProxy::GetTypeOfString(ScriptContext* requestContext)`
`1651`	`1653`	`{`
`1652`		`- if (this->handler == nullptr)`
	`1654`	`+ const JavascriptProxy* proxy = UnwrapNestedProxies(this);`
	`1655`	`+`
	`1656`	`+ if (proxy->handler == nullptr)`
`1653`	`1657`	`{`
`1654`	`1658`	`// even if handler is nullptr, return typeof as "object"`
`1655`	`1659`	`return requestContext->GetLibrary()->GetObjectTypeDisplayString();`
`@@ -1662,7 +1666,7 @@ namespace Js`
`1662`	`1666`	`else`
`1663`	`1667`	`{`
`1664`	`1668`	`// handle nested cases recursively`
`1665`		`- return this->target->GetTypeOfString(requestContext);`
	`1669`	`+ return proxy->target->GetTypeOfString(requestContext);`
`1666`	`1670`	`}`
`1667`	`1671`	`}`
`1668`	`1672`