675520 - Crash [@ js::NativeIterator::isKeyIter] with evil proxy

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

var handler = {iterate: function() { return Iterator.prototype; }};
var proxy = Proxy.create(handler);
for (var p in proxy) { }

> 0   js::NativeIterator::isKeyIter() const + 12
> 1   IteratorMore(JSContext*, JSObject*, bool*, js::Value*) + 75
> 2   js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) + 36763
> 3   js::RunScript(JSContext*, JSScript*, js::StackFrame*) + 307
> 4   js::Execute(JSContext*, JSScript*, JSObject&, js::Value const&, js::ExecuteType, js::StackFrame*, js::Value*) + 570
> 5   js::ExternalExecute(JSContext*, JSScript*, JSObject&, js::Value*) + 284
> 6   JS_ExecuteScript + 82
> 7   Process(JSContext*, JSObject*, char*, int) + 488
> 8   ProcessArgs(JSContext*, JSObject*, char**, int) + 2202
> 9   Shell(JSContext*, int, char**, char**) + 271
> 10  main + 536
> 11  start + 52

The first bad revision is:
changeset:   6ca8580eb84f
user:        Andreas Gal
date:        Thu May 27 12:03:25 2010 -0700
summary:     Implement iterate trap for proxy handlers (bug 568413, r=brendan).

Comment 1

[Josh Matthews [:jdm]]

Attachment: Ensure all uses of obj->getNativeIterator return valid iterators.

Comment 2

[Jeff Walden [:Waldo]]

Is there a reason we can't give Iterator.prototype a NativeIterator* private, presumably iterating over zero names?  If that's doable, it seems better, to avoid the complexity here.  I'm not familiar enough with the iterator code to be sure I'm not missing some wrinkle, tho.  Does anyone who actually knows that code see a problem with that trick?

Comment 3

[Jeff Walden [:Waldo]]

I talked to Andreas about comment 2, he said it was a good idea.  That trick will fit better amongst the Iterator boostrapping code if it waits until I finish up some refactoring of it that I've been doing, so let's hold off on doing that until that's done.  I'll file that bug and mark the dependency in a few minutes.

Comment 4

[Josh Matthews [:jdm]]

Comment on attachment 549853 [details] [diff] [review]
Ensure all uses of obj->getNativeIterator return valid iterators.

Cool. I'm happy to give it a shot once the refactoring's done.

Comment 5

[Jeff Walden [:Waldo]]

Note that if we associate a NativeIterator* with Iterator.prototype, most current null-checks of getNativeIterator() can go away.  (Depending how it's done, that might be all -- but since Iterator.prototype, with the blocking bug's patches, is allocated generically, there's a small window of time between when Iterator.prototype is created and when it's given its NativeIterator*, when a GC or such could happen.  So there's a little trickiness in a case which probably will never exist outside extremely fine-grained GC testing.)  They could be left in place, sure, but that sort of code detritus won't do anything for confused future readers, so it really should go at the same time as this change happens.

Comment 6

[Jeff Walden [:Waldo]]

Testing my proto-patch (you know about the jstests.py and jit_test.py tests, right?) I discovered we have a test which depends on weirdnesses of how Iterator.prototype iteration works:

http://hg.mozilla.org/integration/mozilla-inbound/file/default/js/src/jit-test/tests/basic/bug649939.js

Nobody cares about these details (except some potential future spec which doesn't exist yet), so just change the test.  Changing Iterator.prototype.next() to throw StopIteration rather than a non-standard error is really a change for the better, anyway.

Comment 7

[Jeff Walden [:Waldo]]

Refactoring's done -- you still up for doing this?

Comment 8

[Josh Matthews [:jdm]]

I've got a working patch. I just need to do the null-check removal now.

Comment 9

[Josh Matthews [:jdm]]

Attachment: Ensure iterator prototype has a native iterator.

I left the null checks in iterator_finalize and iterator_trace, but all the rest are gone.

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 564685 [details] [diff] [review]
Ensure iterator prototype has a native iterator.

Review of attachment 564685 [details] [diff] [review]:
-----------------------------------------------------------------

This line in jsiter.cpp:GetIterator should be updated as well:

        if (op && (obj->getClass() != &IteratorClass || obj->getNativeIterator())) {

::: js/src/jsiter.cpp
@@ +1455,5 @@
> +    if (!ni)
> +        return false;
> +    ni->init(NULL, 0 /* flags */, 0, 0);
> +
> +    iteratorProto->setNativeIterator(ni);

We've done this sort of very-prototype-specific initialization earlier in initialization -- immediately after the createBlankPrototype call.  Basically, it's best if special prototype objects look like normal objects as soon as possible, to avoid any potential for funniness.  Move that up to just after that, please.

Comment 11

[Josh Matthews [:jdm]]

Attachment: Ensure iterator prototype has a native iterator.

Comment 12

[Jeff Walden [:Waldo]]

Comment on attachment 564727 [details] [diff] [review]
Ensure iterator prototype has a native iterator.

Review of attachment 564727 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the noted change, and assuming you concur with my logic.  And another special case bites the dust!  \o/

::: js/src/jsiter.cpp
@@ -569,5 @@
>  
>      if (obj) {
>          /* Enumerate Iterator.prototype directly. */
>          JSIteratorOp op = obj->getClass()->ext.iteratorObject;
> -        if (op && (obj->getClass() != &IteratorClass || obj->getNativeIterator())) {

We have: if there's an iterator op, *and* the object's not IteratorClass or it is and has a NativeIterator, call the op -- (not an IteratorClass OR (is IteratorClass AND has a native iterator)).

But with this patch we change so every IteratorClass object that reaches here has a NativeIterator -- that is, it's not Iterator.prototype.  So the latter condition is simplified to (not an IteratorClass OR true).  So it's really just |if (op && true)|, or just |if (op)|.  Right?

Since |op| is only used within the if-block, you should further change this like so, to reduce variable scope and make the code that much easier to read and understand:

  if (JSIteratorOp op = obj->getClass()->ext.iteratorObject) {
    ...
  }

@@ +1444,5 @@
> +    if (!ni)
> +        return false;
> +    ni->init(NULL, 0 /* flags */, 0, 0);
> +
> +    iteratorProto->setNativeIterator(ni);

So good...

Comment 13

[Josh Matthews [:jdm]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/366efc04fd3c

Comment 14

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/366efc04fd3c

Comment 15

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929