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

RESOLVED FIXED in mozilla10

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: Jesse Ruderman, Assigned: jdm)

Tracking

(Blocks: 1 bug, {crash, testcase})

Trunk
mozilla10
x86_64
Mac OS X
crash, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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).
(Assignee)

Comment 1

6 years ago
Created attachment 549853 [details] [diff] [review]
Ensure all uses of obj->getNativeIterator return valid iterators.
Attachment #549853 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

6 years ago
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.
(Assignee)

Comment 4

6 years ago
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 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.
Refactoring's done -- you still up for doing this?
(Assignee)

Comment 8

6 years ago
I've got a working patch. I just need to do the null-check removal now.
(Assignee)

Comment 9

6 years ago
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.
Attachment #564685 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

6 years ago
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-
(Assignee)

Comment 11

6 years ago
Created attachment 564727 [details] [diff] [review]
Ensure iterator prototype has a native iterator.
Attachment #564727 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

6 years ago
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+
(Assignee)

Comment 13

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/366efc04fd3c
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/366efc04fd3c
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.