Skip to content

Commit c31a86a

Browse files
author
Suwei Chen
committed
Merge pull request #1982 from suwc:build/suwc/bugfix Type confusion in Array.prototype.filter Type confusion due to reentrancy can cause a Var to be written into a native int array. Fix by making sure type-specialized code path is used only when ArraySpeciesCreate() invokes built-in Array constructor. Heap overflow in Array.prototype.splice In Array.prototype.splice, array length is cached before ArraySpeciesCreate() is invoked. Side-effect from ArraySpeciesCreate() can change array length and result in inconsistent states and possibly heap overflow. Fix by adding length check to keep cases with side effects out of fast path with pre-calculated length. Also tweak logic in ArraySpeciesCreate() to flag a non-built-in constructor with missing [@species] property. Type confusion in FillFromPrototypes In ForEachOwnMissingArrayIndexOfObject(), existing array enumeration logic assumes Var array. A native array from caller can cause type confusion and leak. Fix by converting incoming native arrays to Var arrays. Parameter type confusion in eval Extra argument signified by CallFlags_ExtraArg shall be cast to FrameDisplay unless the extra argument is used for new.target, in which case CallFlags_NewTarget is be set. Type confusion and AV occur because existing logic in eval() does not check if CallFlags_NewTarget is cleared before using extra argument as FrameDisplay. Fix by adding CallFlags_NewTarget check to eval() before cast to FrameDisplay. Type confusion in JSON.parse Non-native array is expected in JSONParser::Walk(). A native array from caller can cause type confusion and heap overflow Fix by converting native arrays to Var arrays. Type confusion in Array.prototype.concat and .splice Array newly created by ArraySpeciesCreate is not being checked if it is a JavascriptCopyOnAccessNativeIntArray, causing near-nullptr AVs. Fix by adding check-and-convert against JavascriptCopyOnAccessNativeIntArray in affected built-ins.
2 parents 103f02f + c2787ef commit c31a86a

File tree

7 files changed

+274
-27
lines changed

7 files changed

+274
-27
lines changed

lib/Runtime/Base/CallInfo.h

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,16 @@ namespace Js
6565
static const ushort ksizeofCount;
6666
static const ushort ksizeofCallFlags;
6767
static const uint kMaxCountArgs;
68+
69+
static bool isDirectEvalCall(CallFlags flags)
70+
{
71+
// This was recognized as an eval call at compile time. The last one or two args are internal to us.
72+
// Argcount will be one of the following when called from global code
73+
// - eval("...") : argcount 3 : this, evalString, frameDisplay
74+
// - eval.call("..."): argcount 2 : this(which is string) , frameDisplay
75+
76+
return (flags & (CallFlags_ExtraArg | CallFlags_NewTarget)) == CallFlags_ExtraArg; // ExtraArg == 1 && NewTarget == 0
77+
}
6878
};
6979

7080
struct InlineeCallInfo

lib/Runtime/Library/GlobalObject.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -491,7 +491,7 @@ namespace Js
491491
// TODO: Handle call from global scope, strict mode
492492
BOOL isIndirect = FALSE;
493493

494-
if (args.Info.Flags & CallFlags_ExtraArg)
494+
if (Js::CallInfo::isDirectEvalCall(args.Info.Flags))
495495
{
496496
// This was recognized as an eval call at compile time. The last one or two args are internal to us.
497497
// Argcount will be one of the following when called from global code

lib/Runtime/Library/JSONParser.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,8 +83,7 @@ namespace JSON
8383
// this is a post order walk. Visit the children before calling walk on this object
8484
if (Js::DynamicObject::IsAnyArray(value))
8585
{
86-
Js::JavascriptArray* arrayVal = Js::JavascriptArray::FromAnyArray(value);
87-
// REVIEW: How do we guarantee that JSON objects are not native arrays?
86+
Js::JavascriptArray* arrayVal = JavascriptArray::EnsureNonNativeArray(Js::JavascriptArray::FromAnyArray(value));
8887
Assert(!Js::JavascriptNativeIntArray::Is(arrayVal) && !Js::JavascriptNativeFloatArray::Is(arrayVal));
8988
uint length = arrayVal->GetLength();
9089
if (!arrayVal->IsCrossSiteObject())

lib/Runtime/Library/JavascriptArray.cpp

Lines changed: 72 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -714,6 +714,40 @@ namespace Js
714714
return IsMissingHeadSegmentItemImpl<double>(index);
715715
}
716716

717+
template<typename T>
718+
void JavascriptArray::InternalFillFromPrototype(JavascriptArray *dstArray, const T& dstIndex, JavascriptArray *srcArray, uint32 start, uint32 end, uint32 count)
719+
{
720+
RecyclableObject* prototype = srcArray->GetPrototype();
721+
while (start + count != end && JavascriptOperators::GetTypeId(prototype) != TypeIds_Null)
722+
{
723+
ForEachOwnMissingArrayIndexOfObject(srcArray, dstArray, prototype, start, end, dstIndex, [&](uint32 index, Var value) {
724+
T n = dstIndex + (index - start);
725+
dstArray->DirectSetItemAt(n, value);
726+
727+
count++;
728+
});
729+
730+
prototype = prototype->GetPrototype();
731+
}
732+
}
733+
734+
template<>
735+
void JavascriptArray::InternalFillFromPrototype<uint32>(JavascriptArray *dstArray, const uint32& dstIndex, JavascriptArray *srcArray, uint32 start, uint32 end, uint32 count)
736+
{
737+
RecyclableObject* prototype = srcArray->GetPrototype();
738+
while (start + count != end && JavascriptOperators::GetTypeId(prototype) != TypeIds_Null)
739+
{
740+
ForEachOwnMissingArrayIndexOfObject(srcArray, dstArray, prototype, start, end, dstIndex, [&](uint32 index, Var value) {
741+
uint32 n = dstIndex + (index - start);
742+
dstArray->SetItem(n, value, PropertyOperation_None);
743+
744+
count++;
745+
});
746+
747+
prototype = prototype->GetPrototype();
748+
}
749+
}
750+
717751
/* static */
718752
bool JavascriptArray::HasInlineHeadSegment(uint32 length)
719753
{
@@ -3308,6 +3342,9 @@ namespace Js
33083342
pDestObj = ArraySpeciesCreate(args[0], 0, scriptContext);
33093343
if (pDestObj)
33103344
{
3345+
#if ENABLE_COPYONACCESS_ARRAY
3346+
JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray<Var>(pDestObj);
3347+
#endif
33113348
// Check the thing that species create made. If it's a native array that can't handle the source
33123349
// data, convert it. If it's a more conservative kind of array than the source data, indicate that
33133350
// so that the data will be converted on copy.
@@ -5863,13 +5900,19 @@ namespace Js
58635900
}
58645901

58655902
newArr = CreateNewArrayHelper(static_cast<uint32>(newLenT), isIntArray, isFloatArray, pArr, scriptContext);
5903+
#if ENABLE_COPYONACCESS_ARRAY
5904+
JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray<Var>(newArr);
5905+
#endif
58665906
newObj = newArr;
58675907
}
58685908
else
58695909
{
58705910
// If the new object we created is an array, remember that as it will save us time setting properties in the object below
58715911
if (JavascriptArray::Is(newObj))
58725912
{
5913+
#if ENABLE_COPYONACCESS_ARRAY
5914+
JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray<Var>(newObj);
5915+
#endif
58735916
newArr = JavascriptArray::FromVar(newObj);
58745917
}
58755918
}
@@ -6649,6 +6692,9 @@ namespace Js
66496692
// If the new object we created is an array, remember that as it will save us time setting properties in the object below
66506693
if (JavascriptArray::Is(newObj))
66516694
{
6695+
#if ENABLE_COPYONACCESS_ARRAY
6696+
JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray<Var>(newObj);
6697+
#endif
66526698
newArr = JavascriptArray::FromVar(newObj);
66536699
}
66546700
}
@@ -6657,10 +6703,13 @@ namespace Js
66576703
{
66586704
pArr->GetArrayTypeAndConvert(&isIntArray, &isFloatArray);
66596705
newArr = CreateNewArrayHelper(deleteLen, isIntArray, isFloatArray, pArr, scriptContext);
6706+
#if ENABLE_COPYONACCESS_ARRAY
6707+
JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray<Var>(newArr);
6708+
#endif
66606709
}
66616710

66626711
// If return object is a JavascriptArray, we can use all the array splice helpers
6663-
if (newArr && isBuiltinArrayCtor)
6712+
if (newArr && isBuiltinArrayCtor && len == pArr->length)
66646713
{
66656714

66666715
// Array has a single segment (need not start at 0) and splice start lies in the range
@@ -7151,6 +7200,9 @@ namespace Js
71517200

71527201
if (JavascriptArray::Is(pNewObj))
71537202
{
7203+
#if ENABLE_COPYONACCESS_ARRAY
7204+
JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray<Var>(pNewObj);
7205+
#endif
71547206
pnewArr = JavascriptArray::FromVar(pNewObj);
71557207
}
71567208

@@ -8924,6 +8976,9 @@ namespace Js
89248976
// If the new object we created is an array, remember that as it will save us time setting properties in the object below
89258977
if (JavascriptArray::Is(newObj))
89268978
{
8979+
#if ENABLE_COPYONACCESS_ARRAY
8980+
JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray<Var>(newObj);
8981+
#endif
89278982
newArr = JavascriptArray::FromVar(newObj);
89288983
}
89298984
}
@@ -9123,7 +9178,8 @@ namespace Js
91239178
}
91249179

91259180
// If the source object is an Array exotic object we should try to load the constructor property and use it to construct the return object.
9126-
RecyclableObject* newObj = ArraySpeciesCreate(obj, 0, scriptContext);
9181+
bool isBuiltinArrayCtor = true;
9182+
RecyclableObject* newObj = ArraySpeciesCreate(obj, 0, scriptContext, nullptr, nullptr, &isBuiltinArrayCtor);
91279183
JavascriptArray* newArr = nullptr;
91289184

91299185
if (newObj == nullptr)
@@ -9137,6 +9193,9 @@ namespace Js
91379193
// If the new object we created is an array, remember that as it will save us time setting properties in the object below
91389194
if (JavascriptArray::Is(newObj))
91399195
{
9196+
#if ENABLE_COPYONACCESS_ARRAY
9197+
JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray<Var>(newObj);
9198+
#endif
91409199
newArr = JavascriptArray::FromVar(newObj);
91419200
}
91429201
}
@@ -9164,7 +9223,7 @@ namespace Js
91649223
if (JavascriptConversion::ToBoolean(selected, scriptContext))
91659224
{
91669225
// Try to fast path if the return object is an array
9167-
if (newArr)
9226+
if (newArr && isBuiltinArrayCtor)
91689227
{
91699228
newArr->DirectSetItemAt(i, element);
91709229
}
@@ -10041,6 +10100,7 @@ namespace Js
1004110100
{
1004210101
if (JavascriptArray::Is(arr))
1004310102
{
10103+
arr = EnsureNonNativeArray(arr);
1004410104
ArrayElementEnumerator e(arr, startIndex, limitIndex);
1004510105

1004610106
while(e.MoveNext<Var>())
@@ -10096,6 +10156,7 @@ namespace Js
1009610156
{
1009710157
if (JavascriptArray::Is(arr))
1009810158
{
10159+
arr = EnsureNonNativeArray(arr);
1009910160
ArrayElementEnumerator e(arr, startIndex, limitIndex);
1010010161

1010110162
while(e.MoveNext<Var>())
@@ -10849,6 +10910,9 @@ namespace Js
1084910910

1085010911
JavascriptArray *JavascriptArray::EnsureNonNativeArray(JavascriptArray *arr)
1085110912
{
10913+
#if ENABLE_COPYONACCESS_ARRAY
10914+
JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray<Var>(arr);
10915+
#endif
1085210916
if (JavascriptNativeIntArray::Is(arr))
1085310917
{
1085410918
arr = JavascriptNativeIntArray::ToVarArray((JavascriptNativeIntArray*)arr);
@@ -10931,23 +10995,6 @@ namespace Js
1093110995
}
1093210996
}
1093310997

10934-
template<typename T>
10935-
void JavascriptArray::InternalFillFromPrototype(JavascriptArray *dstArray, const T& dstIndex, JavascriptArray *srcArray, uint32 start, uint32 end, uint32 count)
10936-
{
10937-
RecyclableObject* prototype = srcArray->GetPrototype();
10938-
while (start + count != end && JavascriptOperators::GetTypeId(prototype) != TypeIds_Null)
10939-
{
10940-
ForEachOwnMissingArrayIndexOfObject(srcArray, dstArray, prototype, start, end, dstIndex, [&](uint32 index, Var value) {
10941-
T n = dstIndex + (index - start);
10942-
dstArray->DirectSetItemAt(n, value);
10943-
10944-
count++;
10945-
});
10946-
10947-
prototype = prototype->GetPrototype();
10948-
}
10949-
}
10950-
1095110998
Var JavascriptArray::SpreadArrayArgs(Var arrayToSpread, const Js::AuxArray<uint32> *spreadIndices, ScriptContext *scriptContext)
1095210999
{
1095311000
// At this stage we have an array literal with some arguments to be spread.
@@ -11458,6 +11505,11 @@ namespace Js
1145811505
{
1145911506
if (!JavascriptOperators::GetProperty((RecyclableObject*)constructor, PropertyIds::_symbolSpecies, &constructor, scriptContext))
1146011507
{
11508+
if (pIsBuiltinArrayCtor != nullptr)
11509+
{
11510+
*pIsBuiltinArrayCtor = false;
11511+
}
11512+
1146111513
return nullptr;
1146211514
}
1146311515
if (constructor == scriptContext->GetLibrary()->GetNull())

lib/Runtime/Library/JavascriptPromise.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -910,7 +910,7 @@ namespace Js
910910

911911
try
912912
{
913-
values->DirectSetItemAt(index, x);
913+
values->SetItem(index, x, PropertyOperation_None);
914914
}
915915
catch (JavascriptExceptionObject* e)
916916
{

0 commit comments

Comments
 (0)