Skip to content

Commit d57de8b

Browse files
msaboffmsaboff
committed
IntlObject uses JSArray::tryCreateUninitialized in an unsafe way
https://bugs.webkit.org/show_bug.cgi?id=167288 Reviewed by Filip Pizlo. Refactored the following "create" methods into a "tryCreate" method and a "create" wrapper: JSArray::create(), Butterfly::create() and createArrayButterfly(). Changed IntlObject.cpp to use JSArray::tryCreate() as it is simpler to use by not requiring the caller to be GC savey. The performance benefits of tryCreateUninitialized() are not needed by the IntlObject c++ code. Did not add a new test as the bug caused LayoutTests/js/intl.html to fail reliably with the JSC option values scribbleFreeCells=true, collectContinuously=true and JSC_useGenerationalGC=false. * runtime/Butterfly.h: * runtime/ButterflyInlines.h: (JSC::Butterfly::tryCreate): Added. (JSC::Butterfly::create): * runtime/IntlObject.cpp: (JSC::canonicalizeLocaleList): (JSC::lookupSupportedLocales): (JSC::intlObjectFuncGetCanonicalLocales): * runtime/JSArray.h: (JSC::createContiguousArrayButterfly): Deleted. (JSC::tryCreateArrayButterfly): Added. (JSC::createArrayButterfly): (JSC::JSArray::tryCreate): Added. (JSC::JSArray::create): Canonical link: https://commits.webkit.org/184355@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@211043 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent e4ac784 commit d57de8b

5 files changed

Lines changed: 108 additions & 34 deletions

File tree

‎Source/JavaScriptCore/ChangeLog‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,37 @@
1+
2017-01-23 Michael Saboff <msaboff@apple.com>
2+
3+
IntlObject uses JSArray::tryCreateUninitialized in an unsafe way
4+
https://bugs.webkit.org/show_bug.cgi?id=167288
5+
6+
Reviewed by Filip Pizlo.
7+
8+
Refactored the following "create" methods into a "tryCreate" method and a
9+
"create" wrapper: JSArray::create(), Butterfly::create() and
10+
createArrayButterfly().
11+
12+
Changed IntlObject.cpp to use JSArray::tryCreate() as it is simpler to use
13+
by not requiring the caller to be GC savey. The performance benefits of
14+
tryCreateUninitialized() are not needed by the IntlObject c++ code.
15+
16+
Did not add a new test as the bug caused LayoutTests/js/intl.html to fail
17+
reliably with the JSC option values scribbleFreeCells=true,
18+
collectContinuously=true and JSC_useGenerationalGC=false.
19+
20+
* runtime/Butterfly.h:
21+
* runtime/ButterflyInlines.h:
22+
(JSC::Butterfly::tryCreate): Added.
23+
(JSC::Butterfly::create):
24+
* runtime/IntlObject.cpp:
25+
(JSC::canonicalizeLocaleList):
26+
(JSC::lookupSupportedLocales):
27+
(JSC::intlObjectFuncGetCanonicalLocales):
28+
* runtime/JSArray.h:
29+
(JSC::createContiguousArrayButterfly): Deleted.
30+
(JSC::tryCreateArrayButterfly): Added.
31+
(JSC::createArrayButterfly):
32+
(JSC::JSArray::tryCreate): Added.
33+
(JSC::JSArray::create):
34+
135
2017-01-23 Joseph Pecoraro <pecoraro@apple.com>
236

337
JavaScriptCore has a weak external symbol in it

‎Source/JavaScriptCore/runtime/Butterfly.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,7 @@ class Butterfly {
111111

112112
static Butterfly* createUninitialized(VM&, JSCell* intendedOwner, size_t preCapacity, size_t propertyCapacity, bool hasIndexingHeader, size_t indexingPayloadSizeInBytes);
113113

114+
static Butterfly* tryCreate(VM& vm, JSCell*, size_t preCapacity, size_t propertyCapacity, bool hasIndexingHeader, const IndexingHeader& indexingHeader, size_t indexingPayloadSizeInBytes);
114115
static Butterfly* create(VM&, JSCell* intendedOwner, size_t preCapacity, size_t propertyCapacity, bool hasIndexingHeader, const IndexingHeader&, size_t indexingPayloadSizeInBytes);
115116
static Butterfly* create(VM&, JSCell* intendedOwner, Structure*);
116117

‎Source/JavaScriptCore/runtime/ButterflyInlines.h‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -67,17 +67,27 @@ inline Butterfly* Butterfly::createUninitialized(VM& vm, JSCell*, size_t preCapa
6767
return result;
6868
}
6969

70-
inline Butterfly* Butterfly::create(VM& vm, JSCell* intendedOwner, size_t preCapacity, size_t propertyCapacity, bool hasIndexingHeader, const IndexingHeader& indexingHeader, size_t indexingPayloadSizeInBytes)
70+
inline Butterfly* Butterfly::tryCreate(VM& vm, JSCell*, size_t preCapacity, size_t propertyCapacity, bool hasIndexingHeader, const IndexingHeader& indexingHeader, size_t indexingPayloadSizeInBytes)
7171
{
72-
Butterfly* result = createUninitialized(
73-
vm, intendedOwner, preCapacity, propertyCapacity, hasIndexingHeader,
74-
indexingPayloadSizeInBytes);
72+
size_t size = totalSize(preCapacity, propertyCapacity, hasIndexingHeader, indexingPayloadSizeInBytes);
73+
void* base = vm.auxiliarySpace.tryAllocate(size);
74+
if (!base)
75+
return nullptr;
76+
Butterfly* result = fromBase(base, preCapacity, propertyCapacity);
7577
if (hasIndexingHeader)
7678
*result->indexingHeader() = indexingHeader;
7779
memset(result->propertyStorage() - propertyCapacity, 0, propertyCapacity * sizeof(EncodedJSValue));
7880
return result;
7981
}
8082

83+
inline Butterfly* Butterfly::create(VM& vm, JSCell* intendedOwner, size_t preCapacity, size_t propertyCapacity, bool hasIndexingHeader, const IndexingHeader& indexingHeader, size_t indexingPayloadSizeInBytes)
84+
{
85+
Butterfly* result = tryCreate(vm, intendedOwner, preCapacity, propertyCapacity, hasIndexingHeader, indexingHeader, indexingPayloadSizeInBytes);
86+
87+
RELEASE_ASSERT(result);
88+
return result;
89+
}
90+
8191
inline Butterfly* Butterfly::create(VM& vm, JSCell* intendedOwner, Structure* structure)
8292
{
8393
return create(

‎Source/JavaScriptCore/runtime/IntlObject.cpp‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -548,7 +548,12 @@ Vector<String> canonicalizeLocaleList(ExecState& state, JSValue locales)
548548
JSObject* localesObject;
549549
if (locales.isString()) {
550550
// a. Let aLocales be CreateArrayFromList(«locales»).
551-
JSArray* localesArray = JSArray::tryCreateUninitialized(vm, globalObject->arrayStructureForIndexingTypeDuringAllocation(ArrayWithContiguous), 1);
551+
JSArray* localesArray = JSArray::tryCreate(vm, globalObject->arrayStructureForIndexingTypeDuringAllocation(ArrayWithContiguous), 1);
552+
if (!localesArray) {
553+
throwOutOfMemoryError(&state, scope);
554+
RETURN_IF_EXCEPTION(scope, Vector<String>());
555+
}
556+
552557
localesArray->initializeIndex(vm, 0, locales);
553558
// 4. Let O be ToObject(aLocales).
554559
localesObject = localesArray;
@@ -887,7 +892,7 @@ static JSArray* lookupSupportedLocales(ExecState& state, const HashSet<String>&
887892

888893
// 3. Let subset be an empty List.
889894
JSGlobalObject* globalObject = state.jsCallee()->globalObject();
890-
JSArray* subset = JSArray::tryCreateUninitialized(vm, globalObject->arrayStructureForIndexingTypeDuringAllocation(ArrayWithUndecided), 0);
895+
JSArray* subset = JSArray::tryCreate(vm, globalObject->arrayStructureForIndexingTypeDuringAllocation(ArrayWithUndecided), 0);
891896
if (!subset) {
892897
throwOutOfMemoryError(&state, scope);
893898
return nullptr;
@@ -1031,7 +1036,7 @@ EncodedJSValue JSC_HOST_CALL intlObjectFuncGetCanonicalLocales(ExecState* state)
10311036

10321037
// 2. Return CreateArrayFromList(ll).
10331038
JSGlobalObject* globalObject = state->jsCallee()->globalObject();
1034-
JSArray* localeArray = JSArray::tryCreateUninitialized(vm, globalObject->arrayStructureForIndexingTypeDuringAllocation(ArrayWithContiguous), localeList.size());
1039+
JSArray* localeArray = JSArray::tryCreate(vm, globalObject->arrayStructureForIndexingTypeDuringAllocation(ArrayWithContiguous), localeList.size());
10351040
if (!localeArray) {
10361041
throwOutOfMemoryError(state, scope);
10371042
return encodedJSValue();

‎Source/JavaScriptCore/runtime/JSArray.h‎

Lines changed: 51 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -52,13 +52,18 @@ class JSArray : public JSNonFinalObject {
5252
}
5353

5454
public:
55+
static JSArray* tryCreate(VM&, Structure*, unsigned initialLength = 0);
5556
static JSArray* create(VM&, Structure*, unsigned initialLength = 0);
5657
static JSArray* createWithButterfly(VM&, GCDeferralContext*, Structure*, Butterfly*);
5758

5859
// tryCreateUninitialized is used for fast construction of arrays whose size and
59-
// contents are known at time of creation. Clients of this interface must:
60+
// contents are known at time of creation. This should be considered a private API.
61+
// Clients of this interface must:
6062
// - null-check the result (indicating out of memory, or otherwise unable to allocate vector).
6163
// - call 'initializeIndex' for all properties in sequence, for 0 <= i < initialLength.
64+
// - Provide a valid GCDefferalContext* if they might garbage collect when initializing properties,
65+
// otherwise the caller can provide a null GCDefferalContext*.
66+
//
6267
JS_EXPORT_PRIVATE static JSArray* tryCreateUninitialized(VM&, GCDeferralContext*, Structure*, unsigned initialLength);
6368
static JSArray* tryCreateUninitialized(VM& vm, Structure* structure, unsigned initialLength)
6469
{
@@ -183,60 +188,79 @@ class JSArray : public JSNonFinalObject {
183188
void setLengthWritable(ExecState*, bool writable);
184189
};
185190

186-
inline Butterfly* createContiguousArrayButterfly(VM& vm, JSCell* intendedOwner, unsigned length, unsigned& vectorLength)
191+
inline Butterfly* tryCreateArrayButterfly(VM& vm, JSCell* intendedOwner, unsigned initialLength)
187192
{
188-
IndexingHeader header;
189-
vectorLength = Butterfly::optimalContiguousVectorLength(
190-
intendedOwner ? intendedOwner->structure(vm) : 0, length);
191-
header.setVectorLength(vectorLength);
192-
header.setPublicLength(length);
193-
Butterfly* result = Butterfly::create(
194-
vm, intendedOwner, 0, 0, true, header, vectorLength * sizeof(EncodedJSValue));
195-
return result;
196-
}
197-
198-
inline Butterfly* createArrayButterfly(VM& vm, JSCell* intendedOwner, unsigned initialLength)
199-
{
200-
Butterfly* butterfly = Butterfly::create(
193+
Butterfly* butterfly = Butterfly::tryCreate(
201194
vm, intendedOwner, 0, 0, true, baseIndexingHeaderForArrayStorage(initialLength),
202195
ArrayStorage::sizeFor(BASE_ARRAY_STORAGE_VECTOR_LEN));
196+
if (!butterfly)
197+
return nullptr;
203198
ArrayStorage* storage = butterfly->arrayStorage();
204199
storage->m_sparseMap.clear();
205200
storage->m_indexBias = 0;
206201
storage->m_numValuesInVector = 0;
207202
return butterfly;
208203
}
209204

205+
inline Butterfly* createArrayButterfly(VM& vm, JSCell* intendedOwner, unsigned initialLength)
206+
{
207+
Butterfly* result = tryCreateArrayButterfly(vm, intendedOwner, initialLength);
208+
RELEASE_ASSERT(result);
209+
return result;
210+
}
211+
210212
Butterfly* createArrayButterflyInDictionaryIndexingMode(
211213
VM&, JSCell* intendedOwner, unsigned initialLength);
212214

213-
inline JSArray* JSArray::create(VM& vm, Structure* structure, unsigned initialLength)
215+
inline JSArray* JSArray::tryCreate(VM& vm, Structure* structure, unsigned initialLength)
214216
{
217+
unsigned outOfLineStorage = structure->outOfLineCapacity();
218+
215219
Butterfly* butterfly;
216-
if (LIKELY(!hasAnyArrayStorage(structure->indexingType()))) {
220+
IndexingType indexingType = structure->indexingType();
221+
if (LIKELY(!hasAnyArrayStorage(indexingType))) {
217222
ASSERT(
218-
hasUndecided(structure->indexingType())
219-
|| hasInt32(structure->indexingType())
220-
|| hasDouble(structure->indexingType())
221-
|| hasContiguous(structure->indexingType()));
222-
unsigned vectorLength;
223-
butterfly = createContiguousArrayButterfly(vm, 0, initialLength, vectorLength);
224-
if (hasDouble(structure->indexingType()))
223+
hasUndecided(indexingType)
224+
|| hasInt32(indexingType)
225+
|| hasDouble(indexingType)
226+
|| hasContiguous(indexingType));
227+
228+
if (initialLength > MAX_STORAGE_VECTOR_LENGTH)
229+
return 0;
230+
231+
unsigned vectorLength = Butterfly::optimalContiguousVectorLength(structure, initialLength);
232+
void* temp = vm.auxiliarySpace.tryAllocate(nullptr, Butterfly::totalSize(0, outOfLineStorage, true, vectorLength * sizeof(EncodedJSValue)));
233+
if (!temp)
234+
return nullptr;
235+
butterfly = Butterfly::fromBase(temp, 0, outOfLineStorage);
236+
butterfly->setVectorLength(vectorLength);
237+
butterfly->setPublicLength(initialLength);
238+
if (hasDouble(indexingType))
225239
clearArray(butterfly->contiguousDouble().data(), vectorLength);
226240
else
227241
clearArray(butterfly->contiguous().data(), vectorLength);
228242
} else {
229243
ASSERT(
230-
structure->indexingType() == ArrayWithSlowPutArrayStorage
231-
|| structure->indexingType() == ArrayWithArrayStorage);
232-
butterfly = createArrayButterfly(vm, 0, initialLength);
244+
indexingType == ArrayWithSlowPutArrayStorage
245+
|| indexingType == ArrayWithArrayStorage);
246+
butterfly = tryCreateArrayButterfly(vm, 0, initialLength);
247+
if (!butterfly)
248+
return nullptr;
233249
for (unsigned i = 0; i < BASE_ARRAY_STORAGE_VECTOR_LEN; ++i)
234250
butterfly->arrayStorage()->m_vector[i].clear();
235251
}
236252

237253
return createWithButterfly(vm, nullptr, structure, butterfly);
238254
}
239255

256+
inline JSArray* JSArray::create(VM& vm, Structure* structure, unsigned initialLength)
257+
{
258+
JSArray* result = JSArray::tryCreate(vm, structure, initialLength);
259+
RELEASE_ASSERT(result);
260+
261+
return result;
262+
}
263+
240264
inline JSArray* JSArray::createWithButterfly(VM& vm, GCDeferralContext* deferralContext, Structure* structure, Butterfly* butterfly)
241265
{
242266
JSArray* array = new (NotNull, allocateCell<JSArray>(vm.heap, deferralContext)) JSArray(vm, structure, butterfly);

0 commit comments

Comments
 (0)