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

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
7 years ago
5 years ago

People

(Reporter: decoder, Assigned: jorendorff)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
assertion, crash, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox5- wontfix, firefox6- wontfix, firefox7- unaffected, firefox8+ fixed, firefox9+ fixed, status1.9.2 unaffected)

Details

(Whiteboard: [sg:critical?][js-triage-done][qa!], crash signature)

Attachments

(3 attachments)

(Reporter)

Description

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

Updated

7 years ago
Whiteboard: js-triage-needed → [js-triage-needed][mentor=jorendorff]

Updated

7 years ago
Assignee: general → 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?
(Assignee)

Comment 3

7 years ago
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.
Group: core-security
Crash Signature: [@ JSParseNode::append]
OS: Linux → All
Hardware: x86_64 → All
Summary: TM: Assertion failure: !pn->pn_defn, at jsparse.cpp:382 → Crash [@ JSParseNode::append] or "Assertion failure: !pn->pn_defn,"
Created attachment 549662 [details]
stack

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.
Blocks: 648175
(Assignee)

Comment 7

7 years ago
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?
(Reporter)

Updated

7 years ago
Blocks: 676763
tracking-firefox8: --- → ?
(Assignee)

Comment 11

7 years ago
Created attachment 552146 [details] [diff] [review]
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
Assignee: ejpbruel → jorendorff
(Assignee)

Comment 12

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

Comment 13

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

Comment 14

7 years ago
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.
Attachment #552243 - Flags: review?(dvander)
Blocks: 673954
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?
Whiteboard: [js-triage-needed][mentor=jorendorff] → [sg:critical?][js-triage-needed][mentor=jorendorff]
Keywords: crash, regression
Duplicate of this bug: 673954
(Assignee)

Comment 17

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

Updated

7 years ago
status-firefox5: --- → wontfix
status-firefox6: --- → wontfix
status-firefox7: --- → affected
status-firefox8: --- → affected
tracking-firefox5: --- → -
tracking-firefox6: --- → -
tracking-firefox7: --- → +
tracking-firefox8: ? → +
(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.
Attachment #552243 - Flags: review?(dvander) → review+
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
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical?][js-triage-needed][mentor=jorendorff] → [sg:critical?][js-triage-done][mentor=jorendorff][inbound]
(Assignee)

Updated

7 years ago
Whiteboard: [sg:critical?][js-triage-done][mentor=jorendorff][inbound] → [sg:critical?][js-triage-done]
(Assignee)

Comment 20

7 years ago
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.
status-firefox8: affected → fixed
status-firefox9: --- → fixed
tracking-firefox9: --- → +
(Assignee)

Comment 23

7 years ago
The regressing changeset isn't in 7.
(Assignee)

Updated

7 years ago
status-firefox7: affected → unaffected
Does that mean that 5 and 6 are unaffected as well?
tracking-firefox7: + → -
(Assignee)

Comment 25

7 years ago
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.
Whiteboard: [sg:critical?][js-triage-done] → [sg:critical?][js-triage-done][qa-]
comment 0, comment 1, and comment 4 contain testcases: this can be verified.
Whiteboard: [sg:critical?][js-triage-done][qa-] → [sg:critical?][js-triage-done][qa+]
(Assignee)

Comment 29

7 years ago
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.
status1.9.2: --- → unaffected
Unable to reproduce this crash on Firefox 9.0 Debug from 2011-12-08.
Status: RESOLVED → VERIFIED
Whiteboard: [sg:critical?][js-triage-done][qa+] → [sg:critical?][js-triage-done][qa!]
Group: core-security
(Reporter)

Comment 31

5 years ago
A testcase for this bug was automatically identified at js/src/tests/js1_8_5/regress/regress-672892.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.