Closed Bug 675520 Opened 9 years ago Closed 8 years ago

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


(Core :: JavaScript Engine, defect, critical)

Not set





(Reporter: jruderman, Assigned: jdm)


(Blocks 1 open bug)


(Keywords: crash, testcase)

Crash Data


(1 file, 2 obsolete files)

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).
Attachment #549853 - Flags: review?(jwalden+bmo)
Assignee: general → josh
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?
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 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.
Attachment #549853 - Flags: review?(jwalden+bmo)
Depends on: 676936
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.
Testing my proto-patch (you know about the and tests, right?) I discovered we have a test which depends on weirdnesses of how Iterator.prototype iteration works:

Nobody cares about these details (except some potential future spec which doesn't exist yet), so just change the test.  Changing to throw StopIteration rather than a non-standard error is really a change for the better, anyway.
Refactoring's done -- you still up for doing this?
I've got a working patch. I just need to do the null-check removal now.
I left the null checks in iterator_finalize and iterator_trace, but all the rest are gone.
Attachment #564685 - Flags: review?(jwalden+bmo)
Attachment #549853 - Attachment is obsolete: true
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.
Attachment #564685 - Flags: review?(jwalden+bmo) → review-
Attachment #564727 - Flags: review?(jwalden+bmo)
Attachment #564685 - Attachment is obsolete: true
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...
Attachment #564727 - Flags: review?(jwalden+bmo) → review+
Closed: 8 years ago
Resolution: --- → FIXED
Automatically extracted testcase for this bug was committed:
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.