[MERGE #6326 @zenparsing] Correctly use VarIs in native array methods

Merge pull request #6326 from zenparsing:fix-nativeintarray-push Fixes #6320 In some situations, a float value can appear as the `array` parameter of `JavascriptNativeIntArray::Push`. We were not correctly dealing with this possibility and attempting to get a virtual address pointer from a float.
chakra-core · Nov 25, 2019 · eaaf7ac · eaaf7ac
2 parents 2d67da9 + 1572d05
commit eaaf7ac
Show file tree

Hide file tree

Showing 4 changed files with 33 additions and 33 deletions.
diff --git a/lib/Runtime/Library/JavascriptArray.cpp b/lib/Runtime/Library/JavascriptArray.cpp
@@ -5176,26 +5176,26 @@ using namespace Js;
     {
         JIT_HELPER_REENTRANT_HEADER(Array_NativeIntPush);
         JIT_HELPER_SAME_ATTRIBUTES(Array_NativeIntPush, Array_VarPush);
-        // Handle non crossSite native int arrays here length within MaxArrayLength.
-        // JavascriptArray::Push will handle other cases.
-        if (JavascriptNativeIntArray::IsNonCrossSite(array))
+
+        // Fast path for case where `array` is a same-site JavascriptNativeIntArray
+        // instance with a length less than MaxArrayLength
+        if (VarIs<JavascriptNativeIntArray>(array) &&
+            VirtualTableInfo<JavascriptNativeIntArray>::HasVirtualTable(array))
         {
-            JavascriptNativeIntArray * nativeIntArray = UnsafeVarTo<JavascriptNativeIntArray>(array);
+            auto* nativeIntArray = UnsafeVarTo<JavascriptNativeIntArray>(array);
             Assert(!nativeIntArray->IsCrossSiteObject());
             uint32 n = nativeIntArray->length;
-
-            if(n < JavascriptArray::MaxArrayLength)
+            if (n < JavascriptArray::MaxArrayLength)
             {
                 nativeIntArray->SetItem(n, value);
-
                 n++;
-
                 AssertMsg(n == nativeIntArray->length, "Wrong update to the length of the native Int array");
-
                 return JavascriptNumber::ToVar(n, scriptContext);
             }
         }
+
         return JavascriptArray::Push(scriptContext, array, JavascriptNumber::ToVar(value, scriptContext));
+
         JIT_HELPER_END(Array_NativeIntPush);
     }
 
@@ -5208,26 +5208,26 @@ using namespace Js;
     {
         JIT_HELPER_REENTRANT_HEADER(Array_NativeFloatPush);
         JIT_HELPER_SAME_ATTRIBUTES(Array_NativeFloatPush, Array_VarPush);
-        // Handle non crossSite native int arrays here length within MaxArrayLength.
-        // JavascriptArray::Push will handle other cases.
-        if(JavascriptNativeFloatArray::IsNonCrossSite(array))
+
+        // Fast path for case where `array` is a same-site JavascriptNativeFloatArray
+        // instance with a length less than MaxArrayLength
+        if (VarIs<JavascriptNativeFloatArray>(array) &&
+            VirtualTableInfo<JavascriptNativeFloatArray>::HasVirtualTable(array))
         {
-            JavascriptNativeFloatArray * nativeFloatArray = UnsafeVarTo<JavascriptNativeFloatArray>(array);
+            auto* nativeFloatArray = UnsafeVarTo<JavascriptNativeFloatArray>(array);
             Assert(!nativeFloatArray->IsCrossSiteObject());
             uint32 n = nativeFloatArray->length;
-
-            if(n < JavascriptArray::MaxArrayLength)
+            if( n < JavascriptArray::MaxArrayLength)
             {
                 nativeFloatArray->SetItem(n, value);
-
                 n++;
-
                 AssertMsg(n == nativeFloatArray->length, "Wrong update to the length of the native Float array");
                 return JavascriptNumber::ToVar(n, scriptContext);
             }
         }
 
         return JavascriptArray::Push(scriptContext, array, JavascriptNumber::ToVarNoCheck(value, scriptContext));
+
         JIT_HELPER_END(Array_NativeFloatPush);
     }
 
@@ -13075,25 +13075,11 @@ using namespace Js;
         return typeId == TypeIds_NativeIntArray;
     }
 
-    bool JavascriptNativeIntArray::IsNonCrossSite(Var aValue)
-    {
-        bool ret = !TaggedInt::Is(aValue) && VirtualTableInfo<JavascriptNativeIntArray>::HasVirtualTable(aValue);
-        Assert(ret == (VarIs<JavascriptNativeIntArray>(aValue) && !VarTo<JavascriptNativeIntArray>(aValue)->IsCrossSiteObject()));
-        return ret;
-    }
-
     bool JavascriptNativeFloatArray::Is(TypeId typeId)
     {
         return typeId == TypeIds_NativeFloatArray;
     }
 
-    bool JavascriptNativeFloatArray::IsNonCrossSite(Var aValue)
-    {
-        bool ret = !TaggedInt::Is(aValue) && VirtualTableInfo<JavascriptNativeFloatArray>::HasVirtualTable(aValue);
-        Assert(ret == (VarIs<JavascriptNativeFloatArray>(aValue) && !VarTo<JavascriptNativeFloatArray>(aValue)->IsCrossSiteObject()));
-        return ret;
-    }
-
     template int   Js::JavascriptArray::GetParamForIndexOf<unsigned int>(unsigned int, Js::Arguments const&, void*&, unsigned int&, Js::ScriptContext*);
     template bool  Js::JavascriptArray::ArrayElementEnumerator::MoveNext<void*>();
     template void  Js::JavascriptArray::SetArrayLiteralItem<void*>(unsigned int, void*);

diff --git a/lib/Runtime/Library/JavascriptArray.h b/lib/Runtime/Library/JavascriptArray.h
@@ -1059,7 +1059,6 @@ namespace Js
         static Var NewInstance(RecyclableObject* function, Arguments args);
 
         static bool Is(TypeId typeId);
-        static bool IsNonCrossSite(Var aValue);
 
         typedef int32 TElement;
 
@@ -1227,7 +1226,6 @@ namespace Js
         static Var NewInstance(RecyclableObject* function, Arguments args);
 
         static bool Is(TypeId typeId);
-        static bool IsNonCrossSite(Var aValue);
 
         typedef double TElement;
 

diff --git a/test/Array/bug_gh6320.js b/test/Array/bug_gh6320.js
@@ -0,0 +1,10 @@
+function f(array) {
+  Array.prototype.push.call(array, 1);
+  ' ' + array;
+}
+
+f([0]);
+f([0]);
+f(2.3023e-320);
+
+print('PASS');
diff --git a/test/Array/rlexe.xml b/test/Array/rlexe.xml
@@ -774,6 +774,12 @@
       <compile-flags>-JsBuiltIn-</compile-flags>
     </default>
   </test>
+  <test>
+    <default>
+      <files>bug_gh6320.js</files>
+      <tags>exclude_nonative</tags>
+    </default>
+  </test>
   <!-- TODO improve performance of sort for no-jit builds then re-enable this case for them-->
   <test>
     <default>