Closed Bug 688974 Opened 8 years ago Closed 8 years ago

Assertion failure: lastProp->hasSlot() && getSlot(lastProp->slot).isUndefined(), at jsscope.cpp:1151

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
Tracking Status
firefox6 + unaffected
firefox7 + unaffected
firefox8 + unaffected
firefox9 + fixed
firefox10 + fixed
status1.9.2 --- unaffected

People

(Reporter: decoder, Assigned: bhackett)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase, Whiteboard: [sg:critical?][qa-])

Attachments

(1 file, 1 obsolete file)

The following test asserts on mozilla-central revision fecae145d884 (options -m -n -a):


gczeal(2);
Object.defineProperty(Object.prototype, "b", {set: function() {}});
function C(a, b) {
    this.a=a;
    this.b=b;
}
var f = C.bind(0x2004, 2);
for (var i=0; i<10000; ++i)
    new f;


S-s for now because test is GC-related (requires gczeal).
Whiteboard: [sg:critical?]
Attached patch patch (obsolete) — Splinter Review
Bug when fixing up objects that add setters to their prototypes during construction.  If we pregenerate a shape for a newly constructed object, we fixup that shape according to the properties added at the current point of execution of the constructor.

function foo() {
  Object.defineProperty(foo.prototype, "x", {set: function() {...}});
  this.x = x;
}

While in this testcase the setter is added outside the constructor, we don't detect that until analyzing the script during compilation, and when fixing the stack read the uninitialized 'this' slot out of the topmost frame.  The fix ignores frames that are at the start of execution.  Once the 'this' object is constructed the pregenerated shape info will have been cleared and the object will have an empty shape.

I'm tempted to rip this stack walking out entirely (and a fair amount of supporting complexity) and just be unsound with respect to code that does such an absurd thing as the example above.  This would manifest as generating the 'x' property on the new object before the 'this.x = x', and not calling the setter.
Assignee: general → bhackett1024
Attachment #564854 - Flags: review?(luke)
I don't understand this fix.  I ran the test case to hit the assert and for the offending fp->thisValue, fp was initialized in C++ through InvokeConstructorKernel+InvokeKernel.  How does the this-value get corrupted in jit code (and only when pc == script->code)?
There is no jitcode change/corruption here.  CallOrConstructBoundFunction does not initialize args.thisv() when calling InvokeConstructor, which goes to InvokeConstructorKernel and tries to compile the method with a still-uninitialized fp->thisValue().

I don't think there is currently a requirement that fp->thisValue() be initialized in the topmost frame when invoking a constructor.  This could be made a requirement, but it would be more involved, especially if we want to let the JIT keep the thisValue() uninitialized when calling scripted constructors.  This fix sidesteps the issue by making sure the stack walk triggered during compilation does not inspect the 'this' values of frames which may not have constructed their 'this' object yet.
(In reply to Brian Hackett from comment #3)

Wow, I was not aware that we allowed 'this' to be unitialized when calling the constructor.  I guess that's valid because we conservatively mark the VM stack.  Well, when/if we get closer to ripping out conservative stack scanning, we can change it back.  (The fix is hardly involve, though; pushInvokeArgs just needs to do MakeValueRangeGCSafe on the pushed values.)

> This fix sidesteps the issue by making sure the stack walk
> triggered during compilation does not inspect the 'this' values of frames
> which may not have constructed their 'this' object yet.

What if fp->thisValue has been initialized but pc still points to script->code?  After initializing thisValue, is it necessary to that pc is incremented before the first chance to escape into C++?
(In reply to Luke Wagner [:luke] from comment #4)
> What if fp->thisValue has been initialized but pc still points to
> script->code?  After initializing thisValue, is it necessary to that pc is
> incremented before the first chance to escape into C++?

The hazard here is that if the prototype gets a setter before the pc is incremented, the fixup pass might think the 'this' value is uninitialized and not change its shape.  This would not be a crash risk but could affect correctness (the added setter would not get called as the object finishes initialization).  Creating the 'this' object is the last thing done before a script starts executing, but it *is* possible to add a setter at the first opcode, in limited/stupid circumstances:

function foo(x) {
  x++;
  this.f = x;
}

new foo({valueOf: function() {
                      Object.defineProperty(foo.prototype, "f", {set:function() {...}});
                  }});

This could be fully fixed by having constraints ensuring the 'this' value will be initialized when the VM reads it.  This would be a more involved fix.
Brian, based on your last comment this sounds like it's not a security vulnerability at all, is that the case? If so, we can open this bug up.
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #6)
> Brian, based on your last comment this sounds like it's not a security
> vulnerability at all, is that the case? If so, we can open this bug up.

No, comment 5 is describing a potential correctness problem with the patch applied (it's hard to do things exactly right the way object construction is currently structured).  Without the patch, there is a use of uninitialized memory here which could potentially see a torn/invalid value.
Ok, thanks for clarifying!
(In reply to Brian Hackett from comment #5)
> but it *is* possible to add a setter at the first
> opcode, in limited/stupid circumstances:

Well, then that seems to make this patch undesirable as the permanent resolution to the bug.

> This could be fully fixed by having constraints ensuring the 'this' value
> will be initialized when the VM reads it.  This would be a more involved fix.

In C++ I think it would be a fix in one place.  Is it more than one place to fix in mjit?
Attachment #564854 - Flags: review?(luke)
Attached patch better patchSplinter Review
Actually, only the C++ call path needs to be updated --- with TI enabled the mjit ensures the 'this' slot of 'new' frames is initially a null value.
Attachment #564854 - Attachment is obsolete: true
Attachment #567611 - Flags: review?(luke)
Comment on attachment 567611 [details] [diff] [review]
better patch

I like this fix a lot more.  It definitely needs a comment explaining "Bail logic inspects 'thisv' of constructor frames, so ensure valid value."

Alternatively, I was hoping for this fix:

diff --git a/js/src/vm/Stack.cpp b/js/src/vm/Stack.cpp
@@ -648,16 +648,17 @@ ContextStack::pushInvokeArgs(JSContext *
     ImplicitCast<CallArgs>(*iag) = CallArgsFromVp(argc, firstUnused);
+    MakeRangeGCSafe(iag->base(), iag->end());

since that's the real "bug" here (only not a bug b/c of conservative GC stack scanning, and will be a bug when someone (perhaps you :) makes us not conservative).
Attachment #567611 - Flags: review?(luke) → review+
I'd rather tackle the exact scanning JS stack issue when we come to it.  That looks like the wrong place for that change anyways --- if we initialize stack memory the instant it is pushed, just to write its real values shortly afterwards with no intervening chance for a GC, then we are wasting a probably significant amount of work.
(In reply to Brian Hackett from comment #12)
> That looks like the wrong place for that change anyways --- if we initialize
> stack memory the instant it is pushed, just to write its real values shortly
> afterwards with no intervening chance for a GC, then we are wasting a
> probably significant amount of work.

Wasted work, sure, but significant, I doubt.  Remember, this is only a few (probably 2-5) values and we're about to call Invoke.  There are only a few really hot ways to call Invoke (replace, sort, map, reduce, forEach, etc) and we should be self-hosting most of those anyways.  Having an interface that says "you had better initialize this before the first possible GC" that is used as widely as pushInvokeArgs is a Bad Idea.

But if you want the local fix for now, that's fine.  I doubt we'll forget the issue now :)
Attachment #567611 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/6fe1b8214bfa
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Any reason we wouldn't want this in beta for 8? Seems like a pretty straight forward fix.
Comment on attachment 567611 [details] [diff] [review]
better patch

Approved for aurora per today's triage meeting.
Attachment #567611 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #16)
> Any reason we wouldn't want this in beta for 8? Seems like a pretty straight
> forward fix.

While this memory is also uninitialized in 8, nothing tries to read that memory except for TI related code, so this bug should only affect 9.
Could someone who is already set up to reproduce this bug please verify the fix?
Whiteboard: [sg:critical?] → [sg:critical?][qa-]
Verified fixed on Firefox 9 and 10.
Group: core-security
Status: RESOLVED → VERIFIED
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.