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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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).

Comment 1

[Brian Hackett [Laid off!]]

Attachment: patch

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.

Comment 2

[Luke Wagner [: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)?

Comment 3

[Brian Hackett [Laid off!]]

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.

Comment 4

[Luke Wagner [:luke]]

(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++?

Comment 5

[Brian Hackett [Laid off!]]

(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.

Comment 6

[Johnny Stenback (:jst)]

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.

Comment 7

[Brian Hackett [Laid off!]]

(In reply to Johnny Stenback (:jst, [e-mail]) 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.

Comment 8

[Johnny Stenback (:jst)]

Ok, thanks for clarifying!

Comment 9

[Luke Wagner [:luke]]

(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?

Comment 10

[Brian Hackett [Laid off!]]

Attachment: better patch

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.

Comment 11

[Luke Wagner [: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).

Comment 12

[Brian Hackett [Laid off!]]

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.

Comment 13

[Luke Wagner [:luke]]

(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 :)

Comment 14

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6fe1b8214bfa

Comment 15

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/mozilla-central/rev/6fe1b8214bfa

Comment 16

[Johnny Stenback (:jst)]

Any reason we wouldn't want this in beta for 8? Seems like a pretty straight forward fix.

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 567611 [details] [diff] [review]
better patch

Approved for aurora per today's triage meeting.

Comment 18

[Brian Hackett [Laid off!]]

(In reply to Johnny Stenback (:jst, [e-mail]) 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.

Comment 19

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b8492fce41a0

Comment 20

[Johnny Stenback (:jst)]

Updating flags per previous comment.

Comment 21

[u279076]

Could someone who is already set up to reproduce this bug please verify the fix?

Comment 22

[Christian Holler (:decoder)]

Verified fixed on Firefox 9 and 10.

Comment 23

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929