TM: "Assertion failure: !argsobj.getPrivate(),"

RESOLVED FIXED in mozilla2.0b9

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: jandem, Assigned: Waldo)

Tracking

Trunk
mozilla2.0b9
Points:
---

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

7 years ago
This test case:
---
function f() {
    "use strict";
    eval("");
    var a = 0;
    for (var i = 0; i < 20; i++) {
        a--;
    }
}
f();
---
Asserts with -j: 
Assertion failure: !argsobj.getPrivate(), at ../jsfun.cpp:272
Assignee: general → jwalden+bmo
blocking2.0: --- → ?
Assertions should be critical.
Severity: normal → critical
Summary: Assertion failure: !argsobj.getPrivate(), at ../jsfun.cpp:272 → TM: "Assertion failure: !argsobj.getPrivate(),"
Version: unspecified → Trunk
Created attachment 498939 [details] [diff] [review]
tentative patch

The cause here seems obvious -- js_PutArgsObject() isn't allowing for the fact that the arguments object may be strict.

The attached patch is my guess at a fix.  It does the obvious thing of allowing the arguments object to be normal or strict.  I fully admit I don't know what I'm doing, though.
Attachment #498939 - Flags: review?(jwalden+bmo)
Comment on attachment 498939 [details] [diff] [review]
tentative patch

Ok, ignore that patch, it's completely bogus.
Attachment #498939 - Flags: review?(jwalden+bmo) → review-
Waldo and I determined that this revision is the culprit:

  changeset:   53418:8721b595e7ab
  user:        Luke Wagner <lw@mozilla.com>
  date:        Mon Aug 09 22:43:33 2010 -0700
  summary:     Bug 539144 - Make formal args a jit-time const offset from fp; rm argv/argc/thisv/script/callobj (r=brendan,dvander)

Strict mode arguments objects shouldn't have their private slot set.  StrictArgumentsClass does has JSCLASS_HAS_PRIVATE, but it's meant to be always NULL and that's all that it's checked for.

However, the private slot gets set here:

#0 JSObject::setPrivate (this=0x7ffff0e10058, data=0x7ffff11010b0) at ../jsobj.h:722
#1 0x00000000005ee616 in js::FlushNativeStackFrameVisitor::visitFrameObjPtr (this=0x7fffffffc0e0, p=0x7ffff11010c0, fp=0x7ffff11010b0) at ../jstracer.cpp:3059
#2 0x00000000005e351f in js::VisitFrameSlots<js::FlushNativeStackFrameVisitor> (visitor=..., cx=0xa901a0, depth=0, fp=0x7ffff11010b0, next=0x0) at ../jstracer.cpp:1806
#3 0x00000000005e2863 in js::VisitStackSlots<js::FlushNativeStackFrameVisitor> (visitor=..., cx=0xa901a0, callDepth=0) at ../jstracer.cpp:1826
#4 0x00000000005af708 in js::FlushNativeStackFrame (cx=0xa901a0, callDepth=0, mp=0xada540, np=0xa754b0) at ../jstracer.cpp:3415

This shouldn't happen.  I'm not sure what the fix is, perhaps Luke or Waldo
can work it out from here?

I do know that it would be nice if we could remove JSCLASS_HAS_PRIVATE from
StrictArgumentsClass so we can't accidentally set it.  To do that would
require removing the |JS_ASSERT(!argsobj.getPrivate());| assertion from
js_PutArgsObject().  stubs::GetElem() also can call getPrivate() on strict
mode arguments objects, but this looks like a NULL test that could be
replaced with something similar.

Also, the comments around jsobj.h:729 should be updated to explain that
strict arguments objects don't use the private slot (and any other
differences between strict and non-strict arguments should be explained
there).
Seems maybe that setPrivate call should be guarded by |if (frameobj->isNormalArguments())|, probably also add in a |else assert(isStrict)| too.  But I don't fully understand what the stack-frame-visiting pattern implemented here does, so conceivably that's not actually right.

Comment 6

7 years ago
(In reply to comment #4)
> Waldo and I determined that this revision is the culprit:
> 
>   changeset:   53418:8721b595e7ab
>   user:        Luke Wagner <lw@mozilla.com>
>   date:        Mon Aug 09 22:43:33 2010 -0700
>   summary:     Bug 539144 - Make formal args a jit-time const offset from fp;
> rm argv/argc/thisv/script/callobj (r=brendan,dvander)

Heh, only insofar as it didn't predict the future (this patch precedes strict args) ;-)
Er, strikes.
Jeff, Nick, how serious is this? Can it cause crashes or security issues?
(In reply to comment #9)
> Jeff, Nick, how serious is this? Can it cause crashes or security issues?

I'd guess no, but I don't really know.
I'm not sure.  It depends on whether we have places that grab out a frame pointer from strict mode arguments and then that affects control flow.  I don't know if such places exist.  They shouldn't.  But then again, this situation shouldn't have existed either.
(In reply to comment #11)
> I'm not sure.  It depends on whether we have places that grab out a frame
> pointer from strict mode arguments and then that affects control flow.  I don't
> know if such places exist.  They shouldn't.  But then again, this situation
> shouldn't have existed either.

This code in stubs::GetElem() is a concern:

        } else if (obj->isArguments()) {
            uint32 arg = uint32(i);

            if (arg < obj->getArgsInitialLength()) {
                copyFrom = obj->addressOfArgsElement(arg);
                if (!copyFrom->isMagic()) {
                    if (JSStackFrame *afp = (JSStackFrame *) obj->getPrivate())
                        copyFrom = &afp->canonicalActualArg(arg);
                    goto end_getelem;
                }
            }
        }

Updated

7 years ago
No longer blocks: 539144
Marking blocking for now, but don't hesitate to suggest unblocking if you decide it's not important.
blocking2.0: ? → betaN+
Created attachment 499983 [details] [diff] [review]
Patch and test per comment 5's understanding
Attachment #498939 - Attachment is obsolete: true
Attachment #499983 - Flags: review?(lw)

Updated

7 years ago
Attachment #499983 - Flags: review?(lw) → review+
(In reply to comment #4)
> 
> I do know that it would be nice if we could remove JSCLASS_HAS_PRIVATE from
> StrictArgumentsClass so we can't accidentally set it.  To do that would
> require removing the |JS_ASSERT(!argsobj.getPrivate());| assertion from
> js_PutArgsObject().  stubs::GetElem() also can call getPrivate() on strict
> mode arguments objects, but this looks like a NULL test that could be
> replaced with something similar.

I can live without this...
 
> Also, the comments around jsobj.h:729 should be updated to explain that
> strict arguments objects don't use the private slot (and any other
> differences between strict and non-strict arguments should be explained
> there).

...but this is dead easy.  Waldo, can you add this to your patch before landing?  You admitted yourself on IRC that you should have done this earlier :)
Er.  Right.  I was going to do that, wasn't I?  Will do before push.  If I forget, someone castigate me harshly even more!
http://hg.mozilla.org/tracemonkey/rev/37ebdb3e7617

...and the comment fixes:

http://hg.mozilla.org/tracemonkey/rev/25908114259b
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: fixed-in-tracemonkey
Target Milestone: --- → mozilla2.0b9
http://hg.mozilla.org/mozilla-central/rev/37ebdb3e7617
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.