Avoid deep recursion in GetTypeOfString when dealing with proxies

VSadov · VSadov · commit 475e1eca69a3 · 2018-07-31T11:19:44.000-07:00
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,19 @@ 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)
+        {
+            while (proxy->handler != nullptr && JavascriptProxy::Is(proxy->target))
+            {
+                proxy = JavascriptProxy::FromVar(proxy->target);
+            }
+
+            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 __v_8697 = Proxy.revocable([], {}).proxy;
+    for (var __v_8698 = 0; __v_8698 < 1e5; __v_8698++) {
+        __v_8697 = new Proxy(__v_8697, {});
+    }
+      var __v_8702 = new Proxy({}, {
+      });
+      (function () {
+        if (__v_8697 != null && typeof __v_8697 == "object") try {
+            console.log("pass");
+        } catch (e) {}
+      })();
+}
+
 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`