[MERGE #5269 @VSadov] Fix AV when GetOwnPropertyDescriptor trap revokes the proxy

Merge pull request #5269 from VSadov:revokeCrash

- Do not re-fetch proxy.target after invoking `handler.getOwnPropertyDescriptor` since the call may have revoked the proxy. Use the cached `targetObj` instead.

Found by OSS-Fuzz

- Do not consider the case a `TypeError`.
Arguably the proxy was valid at the time of the entry. (also consistent with other JS runtimes).

Author: VSadov

lib/Runtime/Library/JavascriptObject.cpp

@@ -674,12 +674,12 @@ using namespace Js;
         if (obj->IsExternal())
         {
             isPropertyDescriptorDefined = obj->HasOwnProperty(propertyId) ?
-                JavascriptOperators::GetOwnPropertyDescriptor(obj, propertyId, scriptContext, &propertyDescriptor) : obj->GetDefaultPropertyDescriptor(propertyDescriptor);
+                JavascriptOperators::GetOwnPropertyDescriptor(obj, propertyId, scriptContext, &propertyDescriptor) : 
+                FALSE;
         }
         else
         {
-            isPropertyDescriptorDefined = JavascriptOperators::GetOwnPropertyDescriptor(obj, propertyId, scriptContext, &propertyDescriptor) ||
-                obj->GetDefaultPropertyDescriptor(propertyDescriptor);
+            isPropertyDescriptorDefined = JavascriptOperators::GetOwnPropertyDescriptor(obj, propertyId, scriptContext, &propertyDescriptor);
         }
         return isPropertyDescriptorDefined;
     }

lib/Runtime/Library/JavascriptProxy.cpp

@@ -195,8 +195,7 @@ namespace Js
         target = nullptr;
     }
 
-    template <class Fn, class GetPropertyIdFunc>
-    BOOL JavascriptProxy::GetPropertyDescriptorTrap(Var originalInstance, Fn fn, GetPropertyIdFunc getPropertyId, PropertyDescriptor* resultDescriptor, ScriptContext* requestContext)
+    BOOL JavascriptProxy::GetPropertyDescriptorTrap(PropertyId propertyId, PropertyDescriptor* resultDescriptor, ScriptContext* requestContext)
     {
         PROBE_STACK(GetScriptContext(), Js::Constants::MinStackDefault);
 
@@ -223,17 +222,17 @@ namespace Js
 
         Assert((static_cast<DynamicType*>(GetType()))->GetTypeHandler()->GetPropertyCount() == 0 ||
             (static_cast<DynamicType*>(GetType()))->GetTypeHandler()->GetPropertyId(GetScriptContext(), 0) == InternalPropertyIds::WeakMapKeyMap);
+
         JavascriptFunction* gOPDMethod = GetMethodHelper(PropertyIds::getOwnPropertyDescriptor, requestContext);
 
         //7. If trap is undefined, then
         //    a.Return the result of calling the[[GetOwnProperty]] internal method of target with argument P.
         if (nullptr == gOPDMethod || GetScriptContext()->IsHeapEnumInProgress())
         {
             resultDescriptor->SetFromProxy(false);
-            return fn(targetObj);
+            return JavascriptOperators::GetOwnPropertyDescriptor(targetObj, propertyId, requestContext, resultDescriptor);
         }
 
-        PropertyId propertyId = getPropertyId();
         Var propertyName = GetName(requestContext, propertyId);
 
         Assert(JavascriptString::Is(propertyName) || JavascriptSymbol::Is(propertyName));
@@ -254,9 +253,8 @@ namespace Js
         //11. Let targetDesc be the result of calling the[[GetOwnProperty]] internal method of target with argument P.
         //12. ReturnIfAbrupt(targetDesc).
         PropertyDescriptor targetDescriptor;
-        BOOL hasProperty;
+        BOOL hasProperty = JavascriptOperators::GetOwnPropertyDescriptor(targetObj, propertyId, requestContext, &targetDescriptor);
 
-        hasProperty = JavascriptOperators::GetOwnPropertyDescriptor(targetObj, getPropertyId(), requestContext, &targetDescriptor);
         //13. If trapResultObj is undefined, then
         //a.If targetDesc is undefined, then return undefined.
         //b.If targetDesc.[[Configurable]] is false, then throw a TypeError exception.
@@ -274,10 +272,13 @@ namespace Js
             {
                 JavascriptError::ThrowTypeError(requestContext, JSERR_InconsistentTrapResult, _u("getOwnPropertyDescriptor"));
             }
-            if (!target->IsExtensible())
+
+            // do not use "target" here, the trap may have caused it to change
+            if (!targetObj->IsExtensible())
             {
                 JavascriptError::ThrowTypeError(requestContext, JSERR_InconsistentTrapResult, _u("getOwnPropertyDescriptor"));
             }
+
             return FALSE;
         }
 
@@ -293,7 +294,8 @@ namespace Js
         //i.Throw a TypeError exception.
         //22. Return resultDesc.
 
-        BOOL isTargetExtensible = target->IsExtensible();
+        // do not use "target" here, the trap may have caused it to change
+        BOOL isTargetExtensible = targetObj->IsExtensible();
         BOOL toProperty = JavascriptOperators::ToPropertyDescriptor(getResult, resultDescriptor, requestContext);
         if (!toProperty && isTargetExtensible)
         {
@@ -1246,16 +1248,6 @@ namespace Js
         return trapResult;
     }
 
-    BOOL JavascriptProxy::GetDefaultPropertyDescriptor(PropertyDescriptor& descriptor)
-    {
-        if (target == nullptr)
-        {
-            JavascriptError::ThrowTypeError(GetScriptContext(), JSERR_ErrorOnRevokedProxy, _u(""));
-        }
-
-        return target->GetDefaultPropertyDescriptor(descriptor);
-    }
-
     // 7.3.12 in ES 2015. While this should have been no observable behavior change. Till there is obvious change warrant this
     // to be moved to JavascriptOperators, let's keep it in proxy only first.
     BOOL JavascriptProxy::TestIntegrityLevel(IntegrityLevel integrityLevel, RecyclableObject* obj, ScriptContext* scriptContext)
@@ -1672,12 +1664,7 @@ namespace Js
     BOOL JavascriptProxy::GetOwnPropertyDescriptor(RecyclableObject* obj, PropertyId propertyId, ScriptContext* requestContext, PropertyDescriptor* propertyDescriptor)
     {
         JavascriptProxy* proxy = JavascriptProxy::FromVar(obj);
-        auto fn = [&](RecyclableObject *targetObj)-> BOOL {
-            return JavascriptOperators::GetOwnPropertyDescriptor(targetObj, propertyId, requestContext, propertyDescriptor);
-        };
-        auto getPropertyId = [&]() -> PropertyId {return propertyId; };
-        BOOL foundProperty = proxy->GetPropertyDescriptorTrap(obj, fn, getPropertyId, propertyDescriptor, requestContext);
-        return foundProperty;
+        return proxy->GetPropertyDescriptorTrap(propertyId, propertyDescriptor, requestContext);
     }
 
 

lib/Runtime/Library/JavascriptProxy.h

@@ -116,7 +116,6 @@ namespace Js
         virtual BOOL IsExtensible() override;
         virtual BOOL PreventExtensions() override;
         virtual void ThrowIfCannotDefineProperty(PropertyId propId, const PropertyDescriptor& descriptor) { }
-        virtual BOOL GetDefaultPropertyDescriptor(PropertyDescriptor& descriptor) override;
         virtual BOOL Seal() override;
         virtual BOOL Freeze() override;
         virtual BOOL IsSealed() override;
@@ -231,8 +230,7 @@ namespace Js
         template <class Fn, class GetPropertyIdFunc>
         BOOL GetPropertyTrap(Var instance, PropertyDescriptor* propertyDescriptor, Fn fn, GetPropertyIdFunc getPropertyId, ScriptContext* requestContext);
 
-        template <class Fn, class GetPropertyIdFunc>
-        BOOL GetPropertyDescriptorTrap(Var originalInstance, Fn fn, GetPropertyIdFunc getPropertyId, PropertyDescriptor* resultDescriptor, ScriptContext* requestContext);
+        BOOL GetPropertyDescriptorTrap(PropertyId propertyId, PropertyDescriptor* resultDescriptor, ScriptContext* requestContext);
 
 #if ENABLE_TTD
     public:

lib/Runtime/Types/RecyclableObject.cpp

@@ -313,13 +313,6 @@ namespace Js
         // Do nothing
     }
 
-    BOOL RecyclableObject::GetDefaultPropertyDescriptor(PropertyDescriptor& descriptor)
-    {
-        // By default, when GetOwnPropertyDescriptor is called for a nonexistent property,
-        // return undefined.
-        return false;
-    }
-
     HRESULT RecyclableObject::QueryObjectInterface(REFIID riid, void **ppvObj)
     {
         Assert(!this->GetScriptContext()->GetThreadContext()->IsScriptActive());

lib/Runtime/Types/RecyclableObject.h

@@ -340,7 +340,6 @@ namespace Js {
         virtual BOOL IsProtoImmutable() const { return false; }
         virtual BOOL PreventExtensions() { return false; };     // Sets [[Extensible]] flag of instance to false
         virtual void ThrowIfCannotDefineProperty(PropertyId propId, const PropertyDescriptor& descriptor);
-        virtual BOOL GetDefaultPropertyDescriptor(PropertyDescriptor& descriptor);
         virtual BOOL Seal() { return false; }                   // Seals the instance, no additional property can be added or deleted
         virtual BOOL Freeze() { return false; }                 // Freezes the instance, no additional property can be added or deleted or written
         virtual BOOL IsSealed() { return false; }

test/es6/proxybugs.js

@@ -230,7 +230,47 @@ var tests = [
             };
 
             var obj = Proxy.revocable({}, handler);
-            assert.throws( () => { Object.getOwnPropertyDescriptor(obj.proxy, 'a'); }, TypeError);
+            Object.getOwnPropertyDescriptor(obj.proxy, 'a');
+            assert.isTrue(trapCalled);
+        }
+    },
+    {
+        name: "Assertion validation : revoking the proxy in getOwnPropertyDescriptor trap, undefined argument",
+        body() {
+            var trapCalled = false;
+            var handler = {
+                getOwnPropertyDescriptor: function (a, b, c) {
+                    trapCalled = true;
+                    obj.revoke();
+
+                    // used to cause AV
+                    a[undefined] = new String();
+                }
+            };
+
+            var obj = Proxy.revocable({}, handler);
+            Object.getOwnPropertyDescriptor(obj.proxy);
+            assert.isTrue(trapCalled);
+        }
+    },
+    {
+        name: "Assertion validation : revoking the proxy in getOwnPropertyDescriptor trap, not undefined return",
+        body() {
+            var trapCalled = false;
+            var handler = {
+                getOwnPropertyDescriptor: function (a, b, c) {
+                    trapCalled = true;
+                    let result = Object.getOwnPropertyDescriptor(obj, 'proxy');
+
+                    obj.revoke();
+
+                    // used to cause AV
+                    return result;
+                }
+            };
+
+            var obj = Proxy.revocable({}, handler);
+            Object.getOwnPropertyDescriptor(obj.proxy);
             assert.isTrue(trapCalled);
         }
     },