624547 - Assertion failure: LIR type error (start of writer pipeline): arg 1 of 'eqi' is 'i2d' which has type double (expected int): 0

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jan de Mooij [:jandem]]

Testcase:
---
function f(x) {
    delete arguments[0];
    for(var i=0; i<20; i++) {
        arguments[0] !== undefined;
    }
}
f(1);
---
Assertion failure: LIR type error (start of writer pipeline): arg 1 of 'eqi' is 'i2d' which has type double (expected int): 0 (../nanojit/LIR.cpp:3207)
Bus error

This is almost the same test case as in bug 621022, but the loop is now inside the function, after the delete.

Comment 1

[Jeff Walden [:Waldo]]

I'm eyeballing this as reading a magic value from the arguments object, and I am predicting on the basis of almost no evidence whatsoever that it's a crash but not a worrisome, security-critical one, so I'll swing back to this after blockers finish.  If someone not busy wants to spend time investigating to determine otherwise, I'm more than happy to fix sooner.

Comment 2

[Brendan Eich [:brendan]]

Nick may have a fast fix.

/be

Comment 3

[Nicholas Nethercote [inactive]]

I took a look but can't see a fast fix.  Here's the bad LIR:

00015:   4  argsub 0
    ------------------------------ # JSOP_ARGSUB
    ldi2 = ldi.sp sp[-32]
    $stack2 = i2d ldi2
    sti.sp sp[0] = ldi2

00018:   4  getgname "undefined"
    ------------------------------ # JSOP_GETGNAME
    globalObj = immi 0xf6d02028
    sti.sp sp[8] = strict/*0*/

00021:   4  strictne
    ------------------------------ # JSOP_STRICTNE
    eqi2 = eqi $stack2, strict/*0*/       ### TYPE ERROR
    eqi3 = eqi eqi2, strict/*0*/
    sti.sp sp[0] = eqi3


The problem seems to be with record_JSOP_ARGSUB().  In particular, we end up in a branch in importImpl that looks like this:

    if (t == JSVAL_TYPE_INT32) { /* demoted */
        JS_ASSERT(hasInt32Repr(*(const Value *)p));
    
        /*
         * Ok, we have a valid demotion attempt pending, so insert an integer
         * read and promote it to double since all arithmetic operations expect
         * to see doubles on entry. The first op to use this slot will emit a
         * d2i cast which will cancel out the i2d we insert here.
         */
        ins = w.ldi(addr);
        ins = w.i2d(ins);

Maybe that d2i cast isn't being performed?  Not sure.  Then again, it's surprising that 't' is JSVAL_TYPE_INT32 here, seems like it should be JSVAL_TYPE_UNDEFINED.  So maybe the importTypeMap[] still thinks that arguments[0] is an integer, ie. it hasn't been updated for the delete?

Comment 4

[Brendan Eich [:brendan]]

> the importTypeMap[] still thinks that arguments[0] is an integer, ie. it hasn't
> been updated for the delete?

Ding ding ding...

/be

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Ah, it's not that the importTypeMap must be filled, but that the tracer is missing a set() somewhere. Anyway, we should just abort when deleting arguments.

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #5)
> Ah, it's not that the importTypeMap must be filled, but that the tracer is
> missing a set() somewhere. Anyway, we should just abort when deleting
> arguments.

The loop is after the delete. We're not recording when it executes.

/be

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Ah, sorry, read too quickly. We still don't want to touch importTypeMap. We want to abort in ARGSUB. ARGSUB is probably missing a guard too.

Comment 8

[Brendan Eich [:brendan]]

Attachment: proposed fix

Good call on ARGSUB -- of course we don't want to abort it always, only if the frame already has an arguments object and the slot in question was deleted.

/be

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> Created attachment 502757 [details] [diff] [review]
> proposed fix
> 
> Good call on ARGSUB -- of course we don't want to abort it always,

which you knew, cuz you pointed out the need to guard ;-). Mental tracing page-in complete.

/be

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 502757 [details] [diff] [review]
proposed fix

Argh, this isn't guarding what's tested. We need to guard that there isn't an arguments object for the entry frame at trace trigger time. I don't see any protection against recording without an arguments object but triggering with. Will try to write a testcase.

The guardNotHole helper assumes as arguments object, so it will NPE here if we don't have one.

/be

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to comment #10)
> arguments object for the entry frame at trace trigger time. I don't see any
> protection against recording without an arguments object but triggering with.

argsobj is part of the entry typemap, so you're good - we'd build a new trace.

re: the patch, I would suggest putting the argsobj condition inside the main if, and using that to (1) abort the trace if there's a hole, and (2) guard that there's no hole.

Comment 12

[Brendan Eich [:brendan]]

(In reply to comment #11)
> (In reply to comment #10)
> > arguments object for the entry frame at trace trigger time. I don't see any
> > protection against recording without an arguments object but triggering with.
> 
> argsobj is part of the entry typemap, so you're good - we'd build a new trace.

Oh good -- but in that case:

> re: the patch, I would suggest putting the argsobj condition inside the main
> if, and using that to (1) abort the trace if there's a hole, and (2) guard that
> there's no hole.

Don't need any of that if we insist on null argsobj, indeed can't guard on hole without a non-null argsobj (comment 10 last para). Right?

/be

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 502757 [details] [diff] [review]
proposed fix


>-    if (!fp->fun()->isHeavyweight()) {
>+    if (!fp->hasArgsObj() && !fp->fun()->isHeavyweight()) {
>         uintN slot = GET_ARGNO(cx->regs->pc);
>         if (slot >= fp->numActualArgs())
>             RETURN_STOP_A("can't trace out-of-range arguments");
>+
>+        LIns *a_ins = getFrameObjPtr(fp->addressOfArgs());
>+        guardNotHole(a_ins, w.nameImmui(slot));
>+

r=me with the guard gone, Brendan says it's okay to abort on ARGSUB w/ argsobj in play.

Comment 14

[Brendan Eich [:brendan]]

Attachment: patch to commit

Comment 15

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/98dd962dab25

/be

Comment 16

[Daniel Veditz [:dveditz]]

(In reply to comment #1)
> I'm eyeballing this as reading a magic value from the arguments object, and I
> am predicting on the basis of almost no evidence whatsoever that it's a crash
> but not a worrisome, security-critical one,

After figuring out the problem and patch did this turn out to be true?

Comment 17

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/98dd962dab25

Comment 19

[Daniel Veditz [:dveditz]]

Ping?

(In reply to comment #16)
> After figuring out the problem and patch did this turn out to be [not actually a security problem]?

Comment 20

[Nicholas Nethercote [inactive]]

(In reply to comment #16)
> 
> After figuring out the problem and patch did this turn out to be true?

I don't have a good sense on this particular bug, but LIR type errors are usually considered security problems because the results are... unpredictable.  So, in the absence of other information I'd say it was a security problem.

Comment 21

[David Anderson [:dvander] - inactive, e-mail if emergency]

In this case it's safe because at worst we'll box a hole as an object, so it's a near-NULL crash.