IonMonkey: "Assertion failure: [barrier verifier] Unmarked edge: <unknown>,"

RESOLVED FIXED in Firefox 19

Status

()

defect
--
critical
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: gkw, Assigned: djvj)

Tracking

(Blocks 1 bug, 5 keywords)

Trunk
mozilla20
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox17 unaffected, firefox18- wontfix, firefox19+ fixed, firefox20+ fixed, firefox-esr10 unaffected, firefox-esr17 unaffected, b2g18 unaffected)

Details

(Whiteboard: [adv-main19+])

Attachments

(3 attachments)

(Reporter)

Description

7 years ago
Posted file stack
gcPreserveCode()
try {
    RegExp("(")
} catch (e) {}
try {
    ArrayBuffer()
    gc()
    with("") {
        t()
    }
} catch (e) {}
try {
    x = {}
    verifyprebarriers()
    r
} catch (e) {}
try {
    schedulegc(35)
    print(/(())/y)
    ArrayBuffer()
    k
} catch (e) {}
try {
    q
} catch (e) {}
try {
    RegExp("?/")
} catch (e) {}
(function() {
    for (z = 0; z < 3; z++) {
        e = z
        x.h = (function() {})
    }
})()

crashes and throws the following error message on js opt shell on IonMonkey changeset 32638e411218 with --ion-eager:

Assertion failure: [barrier verifier] Unmarked edge: <unknown>,

s-s and assuming sec-critical because gc seems related. My js opt shell was compiled with --enable-more-deterministic and --enable-gczeal.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   113229:6ba78023b367
user:        Brian Hackett
date:        Wed Nov 14 06:46:31 2012 -0800
summary:     Eagerly generate a single copy of Ion stubs and wrappers, bug 786146. r=dvander
(Reporter)

Comment 1

7 years ago
I had tested this on a 32-bit shell in Ubuntu Linux 12.10.

I do not see the assertion failure in a 32-bit debug shell.
OS: Mac OS X → Linux
Hardware: x86_64 → x86

Comment 2

7 years ago
Over to bhackett for now as he wrote the patch we think caused the regression.
Assignee: general → bhackett1024
I think the autoBisect blame here is wrong.  Bug 786146 just changes how IonCode stubs are stored, and the edge the verifier is complaining about here is between two objects (x and the empty lambda).

Bug 786146 does change the number of GC things allocated when we first try to Ion compile code in a compartment, which will perturb the point at which the GC happens due to that schedulegc(35) call.

Bill, does this look like any existing IGC bugs?
Bill, can you look at comment 3?
Assignee: bhackett1024 → general
David, this is the one where it's creating an IC for an existing property and for a new property. I'm going to look into why it's only reproducing in opt builds.

Also, David, if you add the assertion about the shape not changing, it does crash in a debug build.
Assignee: general → dvander
(Musical chairs!) Kannan - could you look at this? I think it's a pretty straightforward bug. We generate a setprop IC, but then nothing stops us from also generating an addrop IC. Probably we should only consider adding an addprop IC if the property was never found to begin with.
Assignee: dvander → kvijayan
Status: NEW → ASSIGNED
I tried to figure out why this only happens in opt builds. I found a problem where the match function for the initial shape table is triggering a read barrier. Since the set of things match is called on depends on address layout, it would be good to avoid that. The barrier is unnecessary there because we're not actually looking at the object, just at its address. We do have to be careful that we don't GC in between the hash table lookup and the actual use of the result, but that's an existing problem with all these weak tables.

After fixing this, it still doesn't reproduce properly in debug builds because of the conservative scanner. But at least this helps a little.
Attachment #692128 - Flags: review?(bhackett1024)
Also, to be totally clear, comment 7 doesn't fix the bug.
Whiteboard: [leave open]
Attachment #692128 - Flags: review?(bhackett1024) → review+
(Reporter)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Comment 9

6 years ago
Taking a look at it.
(Assignee)

Comment 12

6 years ago
Bill,

I tried reproducing this with a tip-build (x86-32, enable-optimize, enable-more-deterministic, enable-gczeal) which includes your determinism patch, and I wasn't able to.

I am able to reproduce if I update to the revision mentioned in comment 1, so I'm proceeding with that.
(Assignee)

Comment 13

6 years ago
Well I can make the error go away by fixing the unnecessary-addprop-stub addition.  However, the extra stub shouldn't be causing a marking error at all.  It should sit there, never getting used and being unnecessary, but not interfere with GC.

Trying to track down the exact cause of the GC error.
(Assignee)

Comment 14

6 years ago
Posted patch FixSplinter Review
Thanks to dvander and billm for helping me understand this.  Here's what's happening and why:

1. The logic for Ion's SETPROP ICs can add both "setprop-existing" stubs (a setprop on a property that doesn't involve a shape change), and "setprop-adding" stubs (a setprop on a property that involves adding the property and changing the shape).

2. The attaching of the setprop stub is tried before the SetProperty operation is itself called, and the attach of the addprop stub is tried afterwards.  The addprop doesn't check to see if a setprop stub has been added, and thus an addprop stub is always generated.

3. The erroneous addprop stub contains an unbarriered write to the property.  This is OK for an actual addprop since we can guarantee that no old value is being overwritten.  For an erroneous addprop, that guarantee doesn't hold.  The GC error arises from here.

4. Most of the time, this addprop stub is never hit, since the generated setprop stub comes before the addprop stub, and succeeds wherever the bad addprop stub would have succeeded.

5. In this test case, a GC occurs in between the creation of the setprop stub and the creation of the addprop stub.  The GC clears out all stubs in the cache, including the setprop stub, and the addprop stub becomes "visible" - no longer masked by the setprop stub as it usually is.

6. The next time around, the bad addprop stub gets executed, and performs an unbarriered write to an existing slot, and causes the GC invariant breakdown.

Fix attached.  Fix is against tip, which doesn't reproduce the problem, so it'll have to be applied to the revision in comment 1 to test.  I was thinking we might want a test for this bug, but it's really hard to reproduce so the test is likely to be largely useless.
Attachment #693123 - Flags: review?(dvander)
Does 9431f3821542 need to be backed out when this lands or is that OK to stay in the tree?
(Assignee)

Comment 16

6 years ago
It's OK to stay in the tree.
Attachment #693123 - Flags: review?(dvander) → review+
(Reporter)

Comment 17

6 years ago
(In reply to Kannan Vijayan [:djvj] from comment #14)
> Created attachment 693123 [details] [diff] [review]
> Fix

This landed:

http://hg.mozilla.org/integration/mozilla-inbound/rev/d3d48381d324
https://hg.mozilla.org/mozilla-central/rev/d3d48381d324

(Leaving open per the whiteboard. Please resolve and set the status flags if it was not intended)
Flags: in-testsuite?
(Reporter)

Comment 19

6 years ago
Kannan, if there's nothing else in this bug, shall it be resolved?
Flags: needinfo?(kvijayan)
(Assignee)

Comment 20

6 years ago
(In reply to Gary Kwong [:gkw] from comment #19)
> Kannan, if there's nothing else in this bug, shall it be resolved?

Minor additional fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/3030d272e4d0

Now it can be resolved :)  Sorry Gary - I had the OK for that patch earlier today but I burrowed into some baseline work and didn't get around to pushing.
Flags: needinfo?(kvijayan)
Whiteboard: [leave open]
(Assignee)

Comment 21

6 years ago
Do I need to request approval for this to make it into firefox-19?
(Reporter)

Comment 22

6 years ago
(In reply to Kannan Vijayan [:djvj] from comment #21)
> Do I need to request approval for this to make it into firefox-19?

Yes. Actually, sec-approval first because this affects the branches. I think next time we should request for sec-approval first before landing on m-c.
(Reporter)

Comment 23

6 years ago
(In reply to Kannan Vijayan [:djvj] from comment #20)
> (In reply to Gary Kwong [:gkw] from comment #19)
> > Kannan, if there's nothing else in this bug, shall it be resolved?
> 
> Minor additional fix:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3030d272e4d0

http://hg.mozilla.org/mozilla-central/rev/3030d272e4d0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to request approval-aurora or something like that on the patch, then fill out the little form that pops up. Once you get approval, you can land it on Aurora.
(Reporter)

Updated

6 years ago
Attachment #693123 - Flags: checkin+
(Reporter)

Comment 25

6 years ago
> Yes. Actually, sec-approval first because this affects the branches. I think
> next time we should request for sec-approval first before landing on m-c.

(That said, it's too late for requesting sec-approval now since it's already landed)
(Assignee)

Comment 26

6 years ago
Comment on attachment 693123 [details] [diff] [review]
Fix

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

Pretty difficult but doable - force a script to become repeatedly invalidated, and also force repeated GCs until one occurs at the right point, freeing a JS-accessible object.  Bogus object reference can now be used to write into freed memory.  Exploit writer won't be able to tell if a given object refers to free memory or not, which will force them to spam this strategy across a large number of objects in the hopes that one of them ends up hitting critical memory with code in it.


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

No.  Checkin comment does not allude in any way to the lurking security issue.


Which older supported branches are affected by this flaw?

firefox 18, 19 and 20 are affected.


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

Patch for bug 750582 (https://hg.mozilla.org/mozilla-central/rev/68777651999a)


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

Patch should apply cleanly to all affected branches.  Low risk.


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

Very unlikely.  Needs minimal testing - the additional logic is trivial and can be reasoned to be obviously better than existing logic.
Attachment #693123 - Flags: sec-approval?
(Assignee)

Comment 27

6 years ago
Comment on attachment 693123 [details] [diff] [review]
Fix

I guess sec-approval is somewhat pointless now.
Attachment #693123 - Flags: sec-approval?
(Assignee)

Comment 28

6 years ago
Browsed through the beta source tree and the problem is present there.
(Assignee)

Comment 29

6 years ago
Comment on attachment 693123 [details] [diff] [review]
Fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Bug 750582 (https://hg.mozilla.org/mozilla-central/rev/68777651999a)

User impact if declined:
Security issue.  Difficult to exploit, but if successful, attacker may be able to write arbitrary data into freed memory.

Testing completed (on m-c, etc.):
Green on m-c.

Risk to taking this patch (and alternatives if risky):
Low risk.  Additional logic is trivial.

String or UUID changes made by this patch:
None.
Attachment #693123 - Flags: approval-mozilla-beta?
Attachment #693123 - Flags: approval-mozilla-aurora?
(Reporter)

Updated

6 years ago
Target Milestone: --- → mozilla20
Comment on attachment 693123 [details] [diff] [review]
Fix

Low risk enough to take it on Aurora 19.Approving
Attachment #693123 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Reporter)

Updated

6 years ago
Keywords: checkin-needed
Comment on attachment 693123 [details] [diff] [review]
Fix

Given the lack of visibility around this issue, the difficulty to exploit, the fact that this was reported internally, and the fact that this would appear for the first time in our final beta, we won't take this for Firefox 18.
Attachment #693123 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Whiteboard: [adv-main19+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.