The following test asserts on mozilla-central revision c9cdc5df55f4 (no options required): with(startTest){ for(var TITLE=0 in startTest) SECTION('huh'); }

There's another testcase in bug 673954.

From what I can tell so far, this is caused by Compiler::compileScript calling RecycleTree in jsparse.cpp:1056, with the tree being recycled containing a definition node. AddNodeToFreeList does not allow definition nodes to be recycled in this way, because "It's too hard to clear these nodes from the AtomDefnMaps, etc.". Furthermore, it states that is the caller's job to recognize and process these. I am not yet sure how Compiler::compileScript is supposed to recognize definition nodes at this point in the code (and why it doesn't), nor do I know how it the caller is supposed to process these nodes when it does recognize them. Can anyone shed some light on this?

Cc-ing brendan and Waldo. Please see comment 2.

with({ e: (7) }) for (var b = (7) ? ['' + (d)] : [] in <x/> ) this try { t(b, 5) } catch (e) {} function v() { p[[]], * } crashes m-i changeset b4165ae3685f without any CLI flags when the testcase passed in as a CLI argument, and asserts similarly. Seems to be a null-deref, but locking just-in-case.

Tested on Mac OS X 10.6 js opt shell.

(not sure if this is entirely correct) autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: 73021:938c1a177114 user: Jason Orendorff date: Tue Jul 19 11:00:43 2011 -0500 summary: Bug 648175 - Remove JSOP_FOR*. Second second landing, to coin a phrase. r=dvander.

The test case in comment 4 reduces to exactly the same thing as the one in comment 0: with (0) for (var b = 0 in 0) ;

Tracing revealed that a node with token kind TOK_NUMBER is added to the list of nodes to be recycled twice. In debug mode, when RecycleTree tries to free it for the second time, it contains poisoned data, causing the assertion.

Cc'ing jimb and igor who wrote and r +'ed the last re-do of recycling, IIRC. /be

(In reply to comment #9) > Cc'ing jimb and igor who wrote and r +'ed the last re-do of recycling, IIRC. I'm seeing this bug (and its many variants) happening really often in fuzzers. Could someone knowledgeable please weigh in on this?

Stealing this bug at Gary's request. Gary, try this patch. It doesn't fix the bug, but it should turn almost all of the crash signatures into the same assertion failure: Assertion failure: pn != freepn, at js/src/jsparse.cpp:376

The parse tree for this: with (1) for (var b = 2 in 3) ; looks like this: (with 1 (seq (var (name b 2)) (for (in #null (name b 2) 3) (semi #null)))) Those two "name" nodes have distinct addresses (one is a clone of the other, I guess) but both refer to the same TOK_NUMBER 2 node.

The node I write as (name b 2) is the result of parsing `b = 2`. It has these fields: .pn_arity = PN_NAME, .pn_used = false, .pn_defn = false, .pn_op = JSOP_SETNAME, .pn_u.name.expr = <pointer to the shared TOK_NUMBER node> I think pn_used=true would mean that the parser has already bound the name to a JSDefinition. In this case, that hasn't happened, I guess because of the with-block. (I don't know in detail how `var b = 2` is supposed to work inside a with block, but I trust it's crazy.) The intention of CloneLeftHandSide is for the new clone node to represent `b`, not `b = 2`: the result should have null pn_expr. It's not doing that. Patch coming.

dvander, feel free to shift this to someone who knows about PN_NAME nodes. I sure don't--but I don't know who does, except Brendan.

From the patch (making sure "pn->pn_expr = NULL;" is called) it looks like uninitialized memory. What can pn_expr do if an attacker controls it?

(In reply to Daniel Veditz from comment #15) > From the patch (making sure "pn->pn_expr = NULL;" is called) it looks like > uninitialized memory. What can pn_expr do if an attacker controls it? The preceding statement "pn->pn_u.name = opn->pn_u.name;" populates that field. pn->pn_expr expands to pn->pn_u.name.expr. So it's not uninitialized memory. But that's little comfort. This kind of double free puts the node onto the freelist twice. New nodes are allocated from the freelist. So you can probably get pretty much any two nodes to be allocated in the same location. You might be able to write arbitrary memory with that, by causing a double to be interpreted as a pointer. Maybe. It'd be tricky, but I wouldn't bet against it. sg-critical? seems about right.

(In reply to Jason Orendorff [:jorendorff] from comment #14) > Created attachment 552243 [details] [diff] [review] > fix > > dvander, feel free to shift this to someone who knows about PN_NAME nodes. I > sure don't--but I don't know who does, except Brendan. Preliminary testing shows that this patch does fix most of the issues for me.

Landed on mozilla-inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/5ca0d6677b2c and landed on mozilla-central: http://hg.mozilla.org/mozilla-central/rev/5ca0d6677b2c

The test doesn't assert in Aurora, which is unsurprising because the regressing changeset isn't in Aurora. I think we're done here.

(In reply to Gary Kwong [:gkw, :nth10sd] from comment #18) > Preliminary testing shows that this patch does fix most of the issues for me. Are there bugs filed on the remaining issues?

Jason, is the fix good for 7 (beta), if so, please request approval.

The regressing changeset isn't in 7.

Does that mean that 5 and 6 are unaffected as well?

Yes.

(In reply to Daniel Veditz from comment #21) > (In reply to Gary Kwong [:gkw, :nth10sd] from comment #18) > > Preliminary testing shows that this patch does fix most of the issues for me. > > Are there bugs filed on the remaining issues? Turns out the other issues were TI or unrelated ones.

qa- as nothing to do for QA verification -- please reset to qa+ if this is not the case.

comment 0, comment 1, and comment 4 contain testcases: this can be verified.

I have not tested this in FF3.6, and I can’t now get the 1.9.2 tree to build locally, but given that the bug is not in FF5, FF6, or FF7, I'm pretty sure it isn’t in FF3.6 either. Also presumably unaffected are FF4, FF3.5, FF1-3, Mosaic, IE3, iTunes, Minesweeper, Altair Basic, French, subspace oscillations, preparedness, the digits of pi, and our Purity of Essence. However there are no guarantees.

Unable to reproduce this crash on Firefox 9.0 Debug from 2011-12-08.

A testcase for this bug was automatically identified at js/src/tests/js1_8_5/regress/regress-672892.js.