Bugzilla

Comment 7

•

13 years ago

Bob, please grab someone on IRC if you get this in gdb -- need to debug a bit.

/be

Andreas Gal :gal

Comment 8

•

13 years ago

This could be sg:crit. We need an analysis here. My plate is full sorry.

Comment 9

•

13 years ago

Jason said "Karate night" and had to run, but he said he'd take it. We believe this is #ifdef DEBUG code that calls lookupProperty which seems like it could call nsHTMLFormElementSH::NewResolve and mess with the tracer's carefully guarded invariants. If so, this bug is not sg:crit and not a blocker except to bc's life testing. It ought to be an easy NPOT(non-DEBUG)B fix.

/be

Assignee: general → jorendorff

Assignee

Comment 10

•

13 years ago

If a resolve hook is causing this, I think that means it's a dup of bug 458271, and that might lead to a small test case. Certainly we shouldn't be asserting rules that we know we're not actually following.

Brendan suggested switching the assertion from obj->lookupProperty to something like SafeLookup. Sounds good to me.

Assignee

Comment 11

•

13 years ago

Doesn't reproduce here (32-bit build on Windows).

Assignee

Comment 12

•

13 years ago

bclary found a URL that reproduces this for me on Windows. It is:

http://www.msnbc.msn.com/id/41584626/ns/business-small_business/?GT1=43001

Assignee

Comment 13

•

13 years ago

This triggers the same assertion with the same stack.

<!doctype html>

<form></form>
<form><input name="id"></form>

<script>
//var HOTLOOP = 8;
var x = document.forms[0], y = document.forms[1];
var forms = [];
for (var j = 0; j < 10000; j++)
    forms[j] = x;
forms[j] = y;
for (var i = 0; i < forms.length; i++)
    forms[i].id;
</script>

Assignee

Comment 14

•

13 years ago

It is definitely a case of bug 458271. In this bug, I'll just weaken the assertion. I'll leave bug 458271 open for the real fix. (Which will follow Ff4, I imagine. This bug report tells us there is now a real-world web site tickling bug 458271, which is a little surprising and should make that a higher priority; but not high enough to block, I think.)

More in a second.

Assignee

Comment 15

•

13 years ago

The bug does not lead to an exploitable crash or undefined behavior in the C++ sense. The situation is:

- Two DOM objects x and y have the same shape.
- They have the same prototype, P, which has a property P.id.
- They have the same resolve hook, but it behaves differently for x vs. y.

At trace record time x.id is inherited from P; and at trace run time, we treat y just like x, since they're the same shape, even though y's resolve hook would have resolved an own property y.id, shadowing P.id. We skip calling the resolve hook on x. Wrong computation, no crash.

Given that it doesn't crash, is it exploitable some other way? I can't think of a plausible scenario. The only remote possibilities I can think of are:

- Inasmuch as chrome code depends on getting right answers, when we
give a wrong answer, that might be exploitable. But in this case we're
talking about stuff like document.forms. Wrappers for such content nodes
have x-ray behavior by default, which causes these resolve hooks to be
skipped anyway. So the bug won't even occur. (We don't trace wrapper
accesses either -- but that is little comfort since I suspect methodjit
has the same bug.)

There might be problems here I'm not seeing, but my hunch is we're OK.

- It is just barely possible that some Web page somewhere contains both
site-generated content and user-generated content, and scripts in the page
distinguish the two by the presence or absence of a form field like
this one. If such a site triggers this bug, it might trust user-generated
content that it should not trust. It could amount to cross-site scripting
in the worst case.

This seems laughably unlikely, but proving that would be hard, maybe
harder than just fixing the bug.

Assignee

Comment 16

•

13 years ago

With resolver (see patch in bug 458271) this can be triggered in the shell:

var p = /./, x = resolver({}, p), y = resolver({lastIndex: 2}, p), v;
var a = [];
for (var i = 0; i < HOTLOOP; i++)
    a[i] = x;
a[HOTLOOP] = y;
for (i = 0; i < a.length; i++)
    v = a[i].lastIndex;
assertEq(v, 2);

Assignee

Comment 17

•

13 years ago

Attached patch v1 — Details — Splinter Review

This patch depends on resolver.

Attachment #513216 - Flags: review?(brendan)

Johnny Stenback (:jst)

Comment 18

•

13 years ago

Based on comment #9 etc this sounds like it should be opened up. Or am I missing something here?

Updated

•

13 years ago

Attachment #513216 - Flags: review?(brendan) → review+

Updated

•

13 years ago

Group: core-security

Assignee

Comment 19

•

13 years ago

Comment on attachment 513216 [details] [diff] [review]
v1

Looking for approval, since the patch is extremely low-risk (#ifdef DEBUG only) and assertions are bad.

Attachment #513216 - Flags: approval2.0?

Johnny Stenback (:jst)

Updated

•

13 years ago

Attachment #513216 - Flags: approval2.0? → approval2.0+

Assignee

Comment 20

•

13 years ago

http://hg.mozilla.org/tracemonkey/rev/711804514c8e

Whiteboard: [fixed-in-tracemonkey]

Robert Sayre

Comment 21

•

13 years ago

http://hg.mozilla.org/mozilla-central/rev/711804514c8e

Status: NEW → RESOLVED

Closed: 13 years ago

Resolution: --- → FIXED

Comment 22

•

13 years ago

I'd really like to make the following change:

--- a/js/src/jsinterp.cpp
+++ b/js/src/jsinterp.cpp
@@ -2054,7 +2054,7 @@ AssertValidPropertyCacheHit(JSContext *c
     }
     if (!ok)
         return false;
-    if (cx->runtime->gcNumber != sample || entry->vshape() != pobj->shape())
+    if (cx->runtime->gcNumber != sample)
         return true;
     JS_ASSERT(prop);
     JS_ASSERT(pobj == found);

because without it, AssertValidPropertyCacheHit really doesn't do its job. However, if we do that, the test here trips that assertion.

I think it's more valuable to be properly vetting property cache hits than to allow this resolver test to run. Can we back out the addition of js/src/jit-test/tests/basic/bug633890.js?

Comment 23

•

13 years ago

Or perhaps the fix is to extend jit_tests.py to cope with tests that are expected to trip an assertion.

Jeff Walden [:Waldo]

Comment 24

•

13 years ago

Slippery slope!  :-)

(Note browser would have to skip regardless since it uses a single browser instance, not one per test.)

Comment 25

•

13 years ago

(In reply to comment #24)
> Slippery slope!  :-)

Well, okay: The test should be deleted, as it's checking something that we don't support yet. :)