Last Comment Bug 675520 - Crash [@ js::NativeIterator::isKeyIter] with evil proxy
: Crash [@ js::NativeIterator::isKeyIter] with evil proxy
Status: RESOLVED FIXED
: crash, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- critical (vote)
: mozilla10
Assigned To: Josh Matthews [:jdm]
:
:
Mentors:
Depends on: 676936
Blocks: 326633
  Show dependency treegraph
 
Reported: 2011-07-31 14:15 PDT by Jesse Ruderman
Modified: 2013-01-19 14:30 PST (History)
9 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Ensure all uses of obj->getNativeIterator return valid iterators. (5.99 KB, patch)
2011-08-01 10:51 PDT, Josh Matthews [:jdm]
no flags Details | Diff | Splinter Review
Ensure iterator prototype has a native iterator. (2.98 KB, patch)
2011-10-04 15:21 PDT, Josh Matthews [:jdm]
jwalden+bmo: review-
Details | Diff | Splinter Review
Ensure iterator prototype has a native iterator. (3.66 KB, patch)
2011-10-04 18:09 PDT, Josh Matthews [:jdm]
jwalden+bmo: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2011-07-31 14:15:33 PDT
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] 2011-08-01 10:51:47 PDT
Created attachment 549853 [details] [diff] [review]
Ensure all uses of obj->getNativeIterator return valid iterators.
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-08-03 17:07:50 PDT
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] (remove +bmo to email) 2011-08-05 13:54:17 PDT
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] 2011-08-05 13:59:11 PDT
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] (remove +bmo to email) 2011-08-05 14:20:34 PDT
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] (remove +bmo to email) 2011-08-05 15:19:07 PDT
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] (remove +bmo to email) 2011-10-03 23:06:51 PDT
Refactoring's done -- you still up for doing this?
Comment 8 Josh Matthews [:jdm] 2011-10-04 06:37:04 PDT
I've got a working patch. I just need to do the null-check removal now.
Comment 9 Josh Matthews [:jdm] 2011-10-04 15:21:32 PDT
Created attachment 564685 [details] [diff] [review]
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] (remove +bmo to email) 2011-10-04 18:01:17 PDT
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] 2011-10-04 18:09:32 PDT
Created attachment 564727 [details] [diff] [review]
Ensure iterator prototype has a native iterator.
Comment 12 Jeff Walden [:Waldo] (remove +bmo to email) 2011-10-04 23:35:20 PDT
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 14 Marco Bonardo [::mak] 2011-10-13 07:31:47 PDT
https://hg.mozilla.org/mozilla-central/rev/366efc04fd3c
Comment 15 Christian Holler (:decoder) 2013-01-19 14:30:34 PST
Automatically extracted testcase for this bug was committed:

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

Note You need to log in before you can comment on or make changes to this bug.