672892 - Crash [@ JSParseNode::append] or "Assertion failure: !pn->pn_defn,"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following test asserts on mozilla-central revision c9cdc5df55f4 (no options required):

with(startTest){
    for(var TITLE=0 in startTest)
        SECTION('huh');
}

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

There's another testcase in bug 673954.

Comment 2

[Eddy Bruel [:ejpbruel]]

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?

Comment 3

[Jason Orendorff [:jorendorff]]

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

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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.

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

Tested on Mac OS X 10.6 js opt shell.

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(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.

Comment 7

[Jason Orendorff [:jorendorff]]

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)
	;

Comment 8

[Eddy Bruel [:ejpbruel]]

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.

Comment 9

[Brendan Eich [:brendan]]

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

/be

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(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?

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: Assert earlier!

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

Comment 12

[Jason Orendorff [:jorendorff]]

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.

Comment 13

[Jason Orendorff [:jorendorff]]

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.

Comment 14

[Jason Orendorff [:jorendorff]]

Attachment: 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.

Comment 15

[Daniel Veditz [:dveditz]]

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?

Comment 17

[Jason Orendorff [:jorendorff]]

(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.

Comment 18

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(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.

Comment 19

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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

Comment 20

[Jason Orendorff [:jorendorff]]

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

Comment 21

[Daniel Veditz [:dveditz]]

(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?

Comment 22

[Johnny Stenback (:jst)]

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

Comment 23

[Jason Orendorff [:jorendorff]]

The regressing changeset isn't in 7.

Comment 24

[Johnny Stenback (:jst)]

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

Comment 25

[Jason Orendorff [:jorendorff]]

Yes.

Comment 26

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(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.

Comment 27

[u279076]

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

Comment 28

[Daniel Veditz [:dveditz]]

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

Comment 29

[Jason Orendorff [:jorendorff]]

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.

Comment 30

[u279076]

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

Comment 31

[Christian Holler (:decoder)]

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