Closed Bug 1077991 Opened 5 years ago Closed 5 years ago

Crash [@ GetObjectAllocKindForCopy] with poison pattern

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla36
Tracking Status
firefox33 --- unaffected
firefox34 --- unaffected
firefox35 --- verified
firefox36 --- verified
firefox-esr31 --- unaffected
b2g-v1.4 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.2 --- fixed

People

(Reporter: decoder, Assigned: sunfish)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:update])

Crash Data

Attachments

(2 files, 2 obsolete files)

The following testcase crashes on mozilla-central revision 4b3d816c21fd (run with --no-threads --fuzzing-safe --baseline-eager):


function x() {
function f(c = function(){})  {
    try {
	throw "ex1";
    } catch(e) {}
    var res = 0;
    for (var i=0; [] < 40; i++)
      res += 2;
}
f()
} x();
Crash trace:


Program received signal SIGSEGV, Segmentation fault.
GetObjectAllocKindForCopy (obj=0x7ffff54001b0, nursery=...) at js/src/gc/Nursery.cpp:369
369         if (obj->is<ArrayObject>()) {
#0  GetObjectAllocKindForCopy (obj=0x7ffff54001b0, nursery=...) at js/src/gc/Nursery.cpp:369
#1  js::Nursery::moveToTenured (this=0x1673be0, trc=0x7fffffffa2d0, src=<optimized out>) at js/src/gc/Nursery.cpp:570
#2  0x00000000004d167a in MinorGCCallback (thingp=0x7fffffff9fd0, jstrc=<optimized out>, kind=<optimized out>) at js/src/gc/Nursery.cpp:721
#3  js::Nursery::MinorGCCallback (jstrc=<optimized out>, thingp=0x7fffffff9fd0, kind=<optimized out>) at js/src/gc/Nursery.cpp:717
#4  0x00000000004b8690 in MarkInternal<JSObject> (trc=0x7fffffffa2d0, thingp=<optimized out>) at js/src/gc/Marking.cpp:317
#5  0x00000000004cc55e in MarkValueInternal (v=0x7fffffffa5d8, trc=0x7fffffffa2d0) at js/src/gc/Marking.cpp:804
#6  MarkValueInternal (v=0x7fffffffa5d8, trc=0x7fffffffa2d0) at js/src/gc/Marking.cpp:827
#7  js::gc::MarkValueRoot (trc=<optimized out>, v=0x7fffffffa5d8, name=<optimized out>) at js/src/gc/Marking.cpp:831
rax     0x2b2b2b2b      3110627432037296939
=> 0x4d0b32 <js::Nursery::moveToTenured(js::gc::MinorCollectionTracer*, JSObject*)+34>: mov    (%rax),%r8


Marked s-s due to crash at poison pattern and related to GC.
Whiteboard: [jsbugmon:update,bisect]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/8f27a48a25d5
user:        Dan Gohman
date:        Wed Sep 17 10:27:25 2014 -0700
summary:     Bug 1029830 - IonMonkey: GVN: Replace UCE with GVN r=nbp

This iteration took 10.606 seconds to run.
Needinfo from sunfish based on comment 3.
Flags: needinfo?(sunfish)
Try blocks which end in a throw are special. Since Ion doesn't compile catch blocks, we need to take special precautions to hold the blocks after the catch reachable. IonBuilder inserts an MTest(true, tryBlock, successor) to artificially hold the successor live. This depends on the optimizer (GVN) not folding this MTest into an MGoto, which would lead to the successor being discarded as unreachable.
Assignee: nobody → sunfish
Flags: needinfo?(sunfish)
Attachment #8509642 - Flags: review?(nicolas.b.pierron)
Duplicate of this bug: 1082611
Same fix, but now with a testcase.
Attachment #8509642 - Attachment is obsolete: true
Attachment #8509642 - Flags: review?(nicolas.b.pierron)
Attachment #8509656 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8509656 [details] [diff] [review]
dont-fold-mtest-to-throwing-try.patch

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

This also affect Gecko 35, you should land the test case separately.

I thought we already had similar issues with OSR blocks, that we want to keep the code reachable from the OSR block even if we can no longer enter the loop.
Can you explain me what is the difference between the two, and why we have to keep a path alive if we have a try-catch and not in the order case.

r? jandem, as he knows better about try-catch statements than I do.
Attachment #8509656 - Flags: review?(jdemooij)
Attachment #8509656 - Flags: review?(nicolas.b.pierron)
(In reply to Nicolas B. Pierron [:nbp] from comment #8)
> I thought we already had similar issues with OSR blocks, that we want to
> keep the code reachable from the OSR block even if we can no longer enter
> the loop.
> Can you explain me what is the difference between the two, and why we have
> to keep a path alive if we have a try-catch and not in the order case.

I think the key difference is that in that case, it's GVN itself which detects the problematic situation and which inserts the unreachable block. It does so in a way that allows itself to complete its run without revisiting the block.

In the testcase in this bug and bug 1082611, it's IonBuilder that needs to insert an artificial predecessor for a block. It currently does so by inserting an "MTest(true, ..., successor)", with |successor| potentially being otherwise unreachable. When GVN runs, it doesn't know there's anything special going on, so it visits this MTest, folds it into an MGoto, discovers that |successor| is now unreachable, and removes it, just like normal. This breaks the special trick that IonBuilder set up to make the try-catch work.

The attached patch lets IonBuilder set a flag to the block containing the special MTest to tell GVN to avoid folding the MTest inside it. This is somewhat crude, but it's much simpler than alternative approaches I considered.

As an alternative, we could define a new kind of MIR instruction which would act just like MTest(true, x, y), but would not have a foldsTo function. This would require more code, but it might be conceptually cleaner. Would you prefer that?
(In reply to Dan Gohman [:sunfish] from comment #9)
> (In reply to Nicolas B. Pierron [:nbp] from comment #8)
> > I thought we already had similar issues with OSR blocks, that we want to
> > keep the code reachable from the OSR block even if we can no longer enter
> > the loop.
> > Can you explain me what is the difference between the two, and why we have
> > to keep a path alive if we have a try-catch and not in the order case.
> 
> […]

Thanks for the explanation. :)

> As an alternative, we could define a new kind of MIR instruction which would
> act just like MTest(true, x, y), but would not have a foldsTo function. This
> would require more code, but it might be conceptually cleaner. Would you
> prefer that?

Indeed, I would prefer to go that way, but for the purpose of this security issue we can just add a flag on MTest, such the we skip the foldsTo of MTest.
Adding a new instruction kind for this turned out to be pretty easy, so I now think I'm just going to propose we do that. Attached is the patch.
Attachment #8509656 - Attachment is obsolete: true
Attachment #8509656 - Flags: review?(jdemooij)
Attachment #8509775 - Flags: review?(jdemooij)
Attachment #8509775 - Flags: review?(jdemooij) → review+
Comment on attachment 8509775 [details] [diff] [review]
goto-with-fake.patch

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

Not easily, I'd say.

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

No. The patch is fixing the bug in a general way, and doesn't contain any hints about the specific set of circumstances needed to actually trigger the bug.

Which older supported branches are affected by this flaw?

mozilla-aurora (gecko 35) is affected.

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

Bug 1029830.

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

I don't currently, but they would be very easy to create, and low-risk.

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

Pretty unlikely. It's conceptually very simple. I don't believe it needs any extraordinary testing.
Attachment #8509775 - Flags: sec-approval?
Comment on attachment 8509775 [details] [diff] [review]
goto-with-fake.patch

sec-approval+, land any time.
Attachment #8509775 - Flags: sec-approval? → sec-approval+
Comment on attachment 8509775 [details] [diff] [review]
goto-with-fake.patch

[Triage Comment]
Please fix this sec-high regression on Aurora as well
a=dveditz
Attachment #8509775 - Flags: approval-mozilla-aurora+
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx35
Group: core-security
You need to log in before you can comment on or make changes to this bug.