Closed Bug 386478 Opened 13 years ago Closed 12 years ago

Invalid read in js_PopStatement


(Core :: JavaScript Engine, defect)

Not set





(Reporter: romaxa, Assigned: igor)





(3 files)

Attached file Full Valgrind log
Open Trunk ff on URL with valgrind

==30698== Invalid read of size 4
==30698==    at 0x4F007FA: js_PopStatement (jsemit.c:1495)
==30698==    by 0x4F43BFA: FunctionBody (jsparse.c:770)
==30698==    by 0x4F43ED3: js_CompileFunctionBody (jsparse.c:850)
==30698==    by 0x4EE6BF6: JS_CompileUCFunctionForPrincipals (jsapi.c:4598)

See full memcheck log in attachment
I will look at it later today.
Assignee: general → igor
The problem is that FunctionBody calls js_PopStatement even in case of errors when the statement stack can be invalid and should be touched.

The consequence of the bug is assignments in js_PopStatement to JSTokenStream fields of values read through a pointer chain originated in the already released stack frame. AFAICS due to the error propogation the results of these asignments are never accessed since JSTokenStream is released itself. Thus on the first impression this is not exploitable.

Still I mark the bug as sensitive pending extended review of PopStatement usages.
Group: security
Attached patch fix v1Splinter Review
For the fix I changed FunctionBody to use "goto bad" on errors to avoid js_PopStatement.
Attachment #270545 - Flags: review?(brendan)
The bug is safe as in the worst case it can lead to a crash. But in reality it is rather harmless. When the code accesses the value allocated on already returned C stack frame that value remains valid as at that moment the stack slots for the returned frame has not yet been overwritten. This is a consequence of immediate-return-on-failure code style used in jsparse.c.  
Group: security
Comment on attachment 270545 [details] [diff] [review]
fix v1

It's good to restore cx->fp correctly, but this patch no longer always arranges to call js_PopStatement after it calls js_PushStatement. Or so it appears from the diffs.

Attachment #270545 - Flags: review?(brendan) → review-
(In reply to comment #6)
> ... this patch no longer always arranges
> to call js_PopStatement after it calls js_PushStatement. 

But this is precisely the point of the patch! It makes sure that js_PopStatement is never called on errors. The rest of the code never does that so on errors JSTreeContext.topStmt may point to JSStmtInfo allocated on already returned C stack frame.

Here for references patched FunctionBody:

static JSParseNode *
FunctionBody(JSContext *cx, JSTokenStream *ts, JSFunction *fun,
             JSTreeContext *tc)
    JSStackFrame *fp, frame;
    JSObject *funobj;
    JSStmtInfo stmtInfo;
    uintN oldflags, firstLine;
    JSParseNode *pn;

    fp = cx->fp;
    funobj = fun->object;
    if (!fp || fp->fun != fun || fp->varobj != funobj ||
        fp->scopeChain != funobj) {
        memset(&frame, 0, sizeof frame); = fun;
        frame.varobj = frame.scopeChain = funobj;
        frame.down = fp;
        if (fp)
            frame.flags = fp->flags & JSFRAME_COMPILE_N_GO;
        cx->fp = &frame;

     * Set interpreted early so js_EmitTree can test it to decide whether to
     * eliminate useless expressions.
    fun->flags |= JSFUN_INTERPRETED;

    oldflags = tc->flags;
    tc->flags &= ~(TCF_RETURN_EXPR | TCF_RETURN_VOID);
    tc->flags |= TCF_IN_FUNCTION;

    /* From this point the control must flow through out: or bad:. */
    js_PushStatement(tc, &stmtInfo, STMT_BLOCK, -1);
    stmtInfo.flags = SIF_BODY_BLOCK;

     * Save the body's first line, and store it in pn->pn_pos.begin.lineno
     * later, because we may have not peeked in ts yet, so Statements won't
     * acquire a valid pn->pn_pos.begin from the current token.
    firstLine = ts->lineno;
    if (CURRENT_TOKEN(ts).type == TOK_LC) {
        pn = Statements(cx, ts, tc);
        if (!pn)
            goto bad;
    } else {
        pn = NewParseNode(cx, ts, PN_UNARY, tc);
        if (!pn)
            goto bad;
        pn->pn_kid = AssignExpr(cx, ts, tc);
        if (!pn->pn_kid)
            goto bad;
        if (tc->flags & TCF_FUN_IS_GENERATOR) {
            ReportBadReturn(cx, ts, JSREPORT_ERROR, JSMSG_BAD_GENERATOR_RETURN,
            goto bad;
        pn->pn_type = TOK_RETURN;
        pn->pn_op = JSOP_RETURN;
        pn->pn_pos.end = pn->pn_kid->pn_pos.end;
    pn = Statements(cx, ts, tc);
    if (!pn)
        goto bad;


    /* Check for falling off the end of a function that returns a value. */
    if (JS_HAS_STRICT_OPTION(cx) && (tc->flags & TCF_RETURN_EXPR)) {
        if (!CheckFinalReturn(cx, ts, pn))
            goto bad;

     * We have a parse tree in pn and a code generator in tc, emit this
     * function's code.  We must do this here, not in js_CompileFunctionBody,
     * in order to detect TCF_IN_FUNCTION among tc->flags.
    pn->pn_pos.begin.lineno = firstLine;
    if ((tc->flags & TCF_COMPILING)) {
        JSCodeGenerator *cg = (JSCodeGenerator *) tc;

        if (!js_FoldConstants(cx, pn, tc) ||
            !js_EmitFunctionBytecode(cx, cg, pn)) {
            goto bad;

    cx->fp = fp;
    tc->flags = oldflags | (tc->flags & (TCF_FUN_FLAGS | TCF_HAS_DEFXMLNS));
    return pn;

    pn = NULL;
    goto out;
Is this still an issue?
Blake, I thought you patched this recently -- in another bug?

Argh, yes. See bug 410852; the two approaches are fundamentally the same if executed slightly differently. I prefer the version without gotos, but I'll leave it up to you which patch you want to take.
The patch here would require to sync with CVS so lets go with bug 410852.
Closed: 12 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 410852
You need to log in before you can comment on or make changes to this bug.