Web Inspector: Crash generating object preview for ArrayIterator

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Summary:
Crash generating object preview for ArrayIterator

Test:
<script>
(function() {
    try {
        var iter = [7][Symbol.iterator]();
        debugger;
        iter['return'] = function() {};
        Array.from(iter, function(){ throw 2; });
    } catch(e) { /* empty */ }
})();
</script>

Steps to reproduce:
1. Inspect test page
2. Reload (to trigger debugger)
3. Step through code
  => Crash after the `iter["return"]` line.

Notes:
- JSInjectedScriptHost::iteratorEntries clones the ArrayIterator and reuses the structure but doesn't fill in all properties that are important for the iterable operations (namely the `return` function it claims to implement).

[Joseph Pecoraro]

<rdar://problem/32859012>

[Joseph Pecoraro]

Created attachment 313688 [details]
[PATCH] Proposed Fix

[Saam Barati]

Comment on attachment 313688 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:530
> +    clone->putDirect(vm, vm.propertyNames->builtinNames().arrayIteratorKindPrivateName(), iteratorObject->getDirect(vm, vm.propertyNames->builtinNames().arrayIteratorKindPrivateName()));

When is this called? What if the iterator object gets mutated by user code? What I’m worried about is a user adds random properties to this object, and then we still end up with properties that are zero filled that we didn’t expect to exist. If this is even possible, I think the best way to proceed would be to just start off the result with the empty structure, and add the properties allowing the object’s structure to transition.

[Joseph Pecoraro]

Comment on attachment 313688 [details]
[PATCH] Proposed Fix

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

>> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:530
>> +    clone->putDirect(vm, vm.propertyNames->builtinNames().arrayIteratorKindPrivateName(), iteratorObject->getDirect(vm, vm.propertyNames->builtinNames().arrayIteratorKindPrivateName()));
> 
> When is this called? What if the iterator object gets mutated by user code? What I’m worried about is a user adds random properties to this object, and then we still end up with properties that are zero filled that we didn’t expect to exist. If this is even possible, I think the best way to proceed would be to just start off the result with the empty structure, and add the properties allowing the object’s structure to transition.

This is called only in this file. This object itself should not be exposed to user code. It is created here, iterated up to 5 times, and closed.

The idea here is that Web Inspector wants to peek and show the next 5 elements in an iterator. Since "Array Iterator" and other native iterators are guaranteed to have side-effect free iteration we can peek upcoming values by cloning the iterator instance, walk the clone forward a few values, and then getting rid of the clone. Array Iterator, unlike Map Iterator, Set Iterator, etc, is different in that it is implemented in JavaScript built-ins. But we can identify one with PrivateNames. This is our implementation of cloning an Array Iterator.

If you see a way that user code can access this object then we might have to abandon this approach or seek an alternative. Maybe that is possible if someone mutates the ArrayIterator prototype? Namely `[].values().__proto__.next`? Starting off with an empty structure won't get us the necessary `next` function.

[Joseph Pecoraro]

Comment on attachment 313688 [details]
[PATCH] Proposed Fix

After talking with Saam going to go a safer route:

  - Only peek iterators when not observable (no user code)
  - Do a better job cloning ArrayIterators

[Joseph Pecoraro]

Created attachment 313767 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 313768 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Comment on attachment 313768 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:557
> +        // FIXME: Reflect.enumerate iteration can be observable, but it is also removed from ECMAScript.

I am removing this in: <https://webkit.org/b/173806> Remove Reflect.enumerate

[Saam Barati]

Comment on attachment 313768 [details]
[PATCH] Proposed Fix

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

r=me

> Source/JavaScriptCore/ChangeLog:9
> +        When Inspector generates an object preview for an ArrayIterator instance it made

I like how this patch turned out

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:527
> +    JSObject* clone = constructEmptyObject(exec, ArrayIteratorPrototype::create(vm, globalObject, ArrayIteratorPrototype::createStructure(vm, globalObject, globalObject->iteratorPrototype())));

Does the global object not cache this structure?

[Joseph Pecoraro]

> > Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:527
> > +    JSObject* clone = constructEmptyObject(exec, ArrayIteratorPrototype::create(vm, globalObject, ArrayIteratorPrototype::createStructure(vm, globalObject, globalObject->iteratorPrototype())));
> 
> Does the global object not cache this structure?

It doesn't. Want me to add it?

[Joseph Pecoraro]

<https://trac.webkit.org/r218836>

[Saam Barati]

(In reply to Joseph Pecoraro from comment #10)
> > > Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:527
> > > +    JSObject* clone = constructEmptyObject(exec, ArrayIteratorPrototype::create(vm, globalObject, ArrayIteratorPrototype::createStructure(vm, globalObject, globalObject->iteratorPrototype())));
> > 
> > Does the global object not cache this structure?
> 
> It doesn't. Want me to add it?

I'm not sure what you went with. It's probably better in the long run, but not essential for this code.