Closed Bug 1182060 Opened 4 years ago Closed 4 years ago

Assertion failure: this->is<T>(), at c:\users\mozilla\debug-builds\mozilla-centr al\js\src\jsobj.h:545

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox41 + fixed
firefox42 --- fixed

People

(Reporter: cbook, Assigned: nbp)

References

(Blocks 1 open bug, )

Details

(Keywords: assertion, sec-other, Whiteboard: [post-critsmash-triage][adv-main41-])

Attachments

(4 files, 1 obsolete file)

Attached file bughunter stack
found via bughunter and reproduced with a Windows 7 Trunk build based on m-c build 

Steps to reproduce:

-> http://www.kakprosto.ru/kak-72550-kak-vosstanovit-vse-nastroyki-windows

--> Assertion failure: this->is<T>(), at c:\users\mozilla\debug-builds\mozilla-central\js\src\jsobj.h:545
jan: can you take a look at this, shu told me you would be a contact person for this
Flags: needinfo?(jdemooij)
It's in scalar replacement so forwarding to nbp.
Flags: needinfo?(jdemooij) → needinfo?(nicolas.b.pierron)
I am already investigating another browser crash related to Scalar Replacement.
I will look at this one after. ( I hope by Tuesday )
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
I might be wrong, but I thought that unboxed objects had no shape, and that
the shape was only set as part of the layout.  This patch should fix this
scalar replacement issue, but I do think that the problem is that we do
generate these GuardShape in the first place.

If I am wrong, then we should probably also update the
CodeGeneratorX86::visitGuardShape to handle unboxed objects.
Attachment #8632141 - Flags: review?(bhackett1024)
Flags: needinfo?(nicolas.b.pierron)
Comment on attachment 8632141 [details] [diff] [review]
IsObjectEscaped: Handle UnboxedPlainObject in guard shape.

Review of attachment 8632141 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/ScalarReplacement.cpp
@@ +269,5 @@
> +                    obj->as<UnboxedPlainObject>().layoutDontCheckGeneration();
> +                if (layout.nativeShape() != guard->shape()) {
> +                    JitSpewDef(JitSpew_Escape, "(unboxed plain object) has a non-matching guard shape\n", guard);
> +                    return true;
> +                }

Unboxed plain objects (and arrays) don't have shapes; the nativeShape() member on the unboxed layout is just caching the shape to give the object if it is converted to a native object, and isn't something that should be used in other situations.

You can call JSObject::maybeShape on any object, and will just get null for unboxed objects, which is maybe what you want here.  I'm having trouble understanding what the point of this piece of code is, however.  Why is the shape in an MGuardShape relevant to whether the object escapes or not?  If the shape has a mismatch then the instruction will definitely bail out, but that seems more like a micro-optimization rather than something necessary for correctness.
Attachment #8632141 - Flags: review?(bhackett1024)
The problem here is that the MNewObject has an unboxed plain object as a template object, and we still have a Shape guard on the MNewObject.

Attached is the last step (use dot to render it) before Scalar Replacement which highlights that the instruction MNewObject (id=201 block=54) has a MGuardShape (id=445 block=117), while the object is manipulated as an unboxed plain object (id=429 block=115).

So I think the first issue is that we generate such guard in the first place, otherwise we are probably not generating the right code in the CodeGen.  Am I wrong?
Flags: needinfo?(bhackett1024)
(In reply to Brian Hackett (:bhackett) from comment #6)
> […]  I'm having trouble
> understanding what the point of this piece of code is, however.  Why is the
> shape in an MGuardShape relevant to whether the object escapes or not?  If
> the shape has a mismatch then the instruction will definitely bail out, but
> that seems more like a micro-optimization rather than something necessary
> for correctness.

This code is made to ensure that the template object that we use for the object creation remains the same, and thus that we can use the same MObjectState layout without checking for this guard.

Thus if the shape is different, then we probably don't know something about the object representation, and we might have some hard time merging the MObjectState from 2 different branches.

  var o = {};
  if (…) {
    o.a = 1;
  } else {
    o.b = 2;
  }
  // o is not escaped, but we no longer have the same shape.

GuardShape is used to ensure that we are in a branch which has a specific shape, which might not be the one that we used for constructing the object.  IsObjectEscaped is a conservative approach, which only allow what can be optimized after.
(In reply to Nicolas B. Pierron [:nbp] from comment #7)
> So I think the first issue is that we generate such guard in the first
> place, otherwise we are probably not generating the right code in the
> CodeGen.  Am I wrong?

MGuardShape will behave correctly on unboxed objects, it will just always miss.  (The second word in an UnboxedPlainObject is a maybe-null expando object pointer, which will never have the same value as a shape pointer.)  It sounds like what you want is to call maybeShape() instead of assuming the object is native.
Flags: needinfo?(bhackett1024)
(In reply to Brian Hackett (:bhackett) from comment #9)
> (In reply to Nicolas B. Pierron [:nbp] from comment #7)
> > So I think the first issue is that we generate such guard in the first
> > place, otherwise we are probably not generating the right code in the
> > CodeGen.  Am I wrong?
> 
> MGuardShape will behave correctly on unboxed objects, it will just always
> miss.  (The second word in an UnboxedPlainObject is a maybe-null expando
> object pointer, which will never have the same value as a shape pointer.) 

Is that on purpose?
Or is this a bug which should be fixed by not generating such MGuardShape?
MGuardShape is an instruction that can work on any object, and associated scalar replacement or recovery code etc. should be able to tolerate any object as well.  Adding an MGuardShape that definitely goes on an unboxed object doesn't really make much sense (maybe we got some inconsistent information from TI and baseline, who knows?), but it shouldn't lead to a crash.
This patch replace the multiple conditional, and add an assertion such that
we can later investigate why we are generating a shap guard for an unboxed
object.  These are not desirable as they might prevent scalar replacement
optimizations.
Attachment #8632141 - Attachment is obsolete: true
Attachment #8632804 - Flags: review?(bhackett1024)
Comment on attachment 8632804 [details] [diff] [review]
IsObjectEscaped: Handle UnboxedPlainObject in guard shape.

Review of attachment 8632804 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/ScalarReplacement.cpp
@@ +259,5 @@
>            case MDefinition::Op_GuardShape: {
>              MGuardShape* guard = def->toGuardShape();
>              MOZ_ASSERT(!ins->isGuardShape());
> +            if (obj->maybeShape() != guard->shape()) {
> +                MOZ_ASSERT(obj->maybeShape(), "UnboxedPlainObject should not have MGuardShape.");

Can you remove this assert?  This code will crash on the same testcases as before, just in a different way.  Asserting conditions that we know aren't always true is kind of abusing MOZ_ASSERT.
Attachment #8632804 - Flags: review?(bhackett1024) → review+
[Tracking Requested - why for this release]:
This issue is caused by Bug 1166711.
Comment on attachment 8632804 [details] [diff] [review]
IsObjectEscaped: Handle UnboxedPlainObject in guard shape.

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

No idea, but I don't think this is even exploitable.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

yes, the assert, but it is going away.

> Which older supported branches are affected by this flaw?

Gecko 41

> If not all supported branches, which bug introduced the flaw?

Bug 1166711

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

The same patch should work fine.

> How likely is this patch to cause regressions; how much testing does it need?

Unlikely.
Attachment #8632804 - Flags: sec-approval?
This is not exploitable, and won't lead to any bad behavior except the assertion failure.  If an unboxed object flows in to this spot then the lastProperty() call will fetch the expando pointer from the object and compare it against a shape pointer, which will always fail (this is part of the unboxed objects design --- we can do shape checks in jit code on an object even if the object does not have a shape).
Clearing the sec-approval? flag and marking this as "sec-other." This can just be checked in.
Keywords: sec-other
Attachment #8632804 - Flags: sec-approval?
https://hg.mozilla.org/mozilla-central/rev/c0afcf6ed324
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Tracked as it is a security bug.
Nicolas, given that this issue is also affecting Aurora41, do you want to request uplift to Aurora? Thanks.
Flags: needinfo?(nicolas.b.pierron)
(In reply to Ritu Kothari (:ritu) from comment #20)
> Nicolas, given that this issue is also affecting Aurora41, do you want to
> request uplift to Aurora? Thanks.

I don't think it is necessary unless we are fuzzing on Aurora, and this could prevent false-positive, as Brian commented (comment 16)
Flags: needinfo?(nicolas.b.pierron)
(In reply to Nicolas B. Pierron [:nbp] from comment #21)
> (In reply to Ritu Kothari (:ritu) from comment #20)
> > Nicolas, given that this issue is also affecting Aurora41, do you want to
> > request uplift to Aurora? Thanks.
> 
> I don't think it is necessary unless we are fuzzing on Aurora, and this
> could prevent false-positive, as Brian commented (comment 16)

I am wondering if we need to mark status-firefox41 as "won't fix" in that case.
Flags: needinfo?(nicolas.b.pierron)
Comment on attachment 8632804 [details] [diff] [review]
IsObjectEscaped: Handle UnboxedPlainObject in guard shape.

Approval Request Comment
[Feature/regressing bug #]: Bug 1166711
[User impact if declined]: None, might reduce fuzzing noise.
[Describe test coverage new/current, TreeHerder]: On m-c since a month
[Risks and why]: Low, use a type-safe variant of the same generated code.
[String/UUID change made/needed]: N/A
Flags: needinfo?(nicolas.b.pierron)
Attachment #8632804 - Flags: approval-mozilla-aurora?
This landed prior to the uplift. The only place it would need to go is Beta if your so chose.
N-i nbp given comment 24.
Flags: needinfo?(nicolas.b.pierron)
Comment on attachment 8632804 [details] [diff] [review]
IsObjectEscaped: Handle UnboxedPlainObject in guard shape.

The ship for Aurora sailed a long time ago.
Attachment #8632804 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Comment on attachment 8632804 [details] [diff] [review]
IsObjectEscaped: Handle UnboxedPlainObject in guard shape.

Patch has been in Nightly and Aurora for over a month. Beta41+
Attachment #8632804 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [post-critsmash-triage]
Group: core-security → core-security-release
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main41-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.