386478 - Invalid read in js_PopStatement

Status: RESOLVED DUPLICATE of bug 410852

(Core :: JavaScript Engine, defect)

Description

[Oleg Romashin (:romaxa)]

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

Comment 1

[Igor Bukanov]

I will look at it later today.

Comment 2

[Igor Bukanov]

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.

Comment 3

[Igor Bukanov]

Attachment: fix v1

For the fix I changed FunctionBody to use "goto bad" on errors to avoid js_PopStatement.

Comment 4

[Igor Bukanov]

Attachment: diff-b version of fix v1

Comment 5

[Igor Bukanov]

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.  

Comment 6

[Brendan Eich [:brendan]]

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.

/be

Comment 7

[Igor Bukanov]

(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);
        frame.fun = 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 JS_HAS_EXPR_CLOSURES
    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,
                            JSMSG_BAD_ANON_GENERATOR_RETURN);
            goto bad;
        }
        pn->pn_type = TOK_RETURN;
        pn->pn_op = JSOP_RETURN;
        pn->pn_pos.end = pn->pn_kid->pn_pos.end;
    }
#else
    pn = Statements(cx, ts, tc);
    if (!pn)
        goto bad;
#endif

    JS_ASSERT(pn);
    js_PopStatement(tc);

    /* 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;
        }
     }

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

  bad:
    pn = NULL;
    goto out;
}

Comment 8

[Reed Loden [:reed]]

Is this still an issue?

Comment 9

[Brendan Eich [:brendan]]

Blake, I thought you patched this recently -- in another bug?

/be

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 11

[Igor Bukanov]

The patch here would require to sync with CVS so lets go with bug 410852.