crash when attempting to perform array iteration on a non-array with numeric keys not initialized

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Claude Pache]

The following JS code will crash WebKit.

    var b = Object.create(Array.iterator)
    for (var x of b) { console.log(x) } // boom

The crash does not occur (and standard Array iteration is triggered) when some numeric key of the iterated object was initialized.

    var b = Object.create(Array.iterator)
    b[3] = 42
    for (var x of b) { console.log(x) } // no crash

[Claude Pache]

(In reply to comment #0)
>     var b = Object.create(Array.iterator)
Replace with:

     var b = Object.create(Array.prototype)

[Alexey Proskuryakov]

rdar://problem/17538114

[Alexey Proskuryakov]

Created attachment 240083 [details]
test case (will crash on load)

Same test as attachment, ready to run.

[Mark Lam]

Here's what I know so far:

1. The crash occurs in Specialized thunk code for array-iterator-next-value generated by arrayIteratorNextThunkGenerator().
2. No DFG JIT is needed.  But JIT'ting in general do need to be enabled because we need to generate the thunks.
3. It crashed because it's an expecting a non-zero butterfly pointer in the array object, but this is not true i.e. we've got NULL.
4. The classInfo of object being operated on (whose butterfly pointer is NULL) says that it is an ArrayIterator:

(lldb) p *((JSObject*)$rdi)->structure()->classInfo()
(const JSC::ClassInfo) $12 = {
  className = 0x0000000100a5ed86 "ArrayIterator"
  ...

5. The crashed code is executed because the object's iterationKind() is ArrayIterateValue.

(lldb) p ((JSArrayIterator*)$rdi)->iterationKind()
(JSC::ArrayIterationKind) $15 = ArrayIterateValue

6. The iterationKind we are generating the thunk of is ArrayIterateValue.

[Geoffrey Garen]

I suspect the problem here is that we just neglected to consider the case where an ArrayIterator is used to iterate a non-Array. One thing that can happen in that case is that the butterfly pointer can be NULL.

[Mark Lam]

More details:

7. If JSC_useJIT=false, we'll create an iterator JSFunction that points to JSArrayIterator's arrayIteratorNextValue() C++ function.
   If JSC_useJIT=true, we'll create an iterator JSFunction that uses the ArrayIteratorNextValueIntrinsic intrinsic thunk.

The arrayIteratorNextValue() C++ function will first check to ensure the this object is a JSArrayIterator.
The next step is to get the length property of the object.  This ultimately results in a call to JSArray::getOwnPropertySlot() which special cases property "length" and sets the property to the return value of JSArray::length().
JSArray::length() calls JSObject::getArrayLength().
JSObject::getArrayLength() discovers that the object does not have any indexed properties yet, and returns a value of 0.

In contrast, the ArrayIteratorNextValueIntrinsic thunk also checks if the object is a JSArrayIterator.
Thereafter, it assumes that the iterated object has indexed properties and proceeds to grab the length from its butterfly.
Since, the butterfly for an Object with no properties is 0, we crash.

After I added a NULL check on the butterfly (and have the thunk take the slow path if the butterfly is NULL), the test case no longer crash.  However, I need to parse the butterfly code some more to get a more comprehensive understanding of how the butterfly is shaped for JSObjects, JSArrays, and typed JSArrays.  The NULL check I added averted the immediate crash, but the thunk is also making assumptions about the shape of the butterfly.  I'll need to verify that those assumptions hold before I can conclude that this fix is appropriate.

[Mark Lam]

(In reply to comment #6)
> 7. If JSC_useJIT=false, we'll create an iterator JSFunction that points to
> JSArrayIterator's arrayIteratorNextValue() C++ function.
> ...
> The arrayIteratorNextValue() C++ function will first check to ensure the
> this object is a JSArrayIterator.
> The next step is to get the length property of the object.
> ...
> JSObject::getArrayLength() discovers that the object does not have any
> indexed properties yet, and returns a value of 0.

Correction: JSObject::getPropertySlot() fails to find the “length” property in the Object, and looks up the prototype chain.  The next object in the chain is the ArrayPrototype.  JSArray::length() is run on the ArrayPrototype, and returns 0.

JSArray::getOwnPropertySlot() also ends up setting the 0 “length” property on the ArrayPrototype, not the Object we’re testing.

[Mark Lam]

Created attachment 240513 [details]
the patch.

[Geoffrey Garen]

Comment on attachment 240513 [details]
the patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=240513&action=review

r=me

> Source/JavaScriptCore/ChangeLog:12
> +        may have been set on the iterated object.  The thunk only check's the butterfly's

check's => checks

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1103
> +    Jump nullButterfly = jit.branchPtr(SpecializedThunkJIT::Equal, SpecializedThunkJIT::regT2, TrustedImmPtr(0));

It's better to use branchTestPtr. Some of our assemblers will automatically optimize this to a test, and some won't.

[Mark Lam]

Comment on attachment 240513 [details]
the patch.

Cancelling commit.  Will apply feedback and commit manually.

[Mark Lam]

Thanks for the review.  Feedback has been applied, and the fix has ben landed in r175243: <http://trac.webkit.org/r175243>.