Closed Bug 862606 Opened 7 years ago Closed 7 years ago

"Assertion failure: [barrier verifier] Unmarked edge: shape" with gczeal(4), adoptNode

Categories

(Core :: XPConnect, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla23
Tracking Status
firefox20 --- unaffected
firefox21 + verified
firefox22 + verified
firefox23 --- verified
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: jruderman, Assigned: billm)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [adv-main21-])

Attachments

(4 files)

1. Install https://www.squarefree.com/extensions/domFuzzLite3.xpi
2. Load the testcase

Assertion failure: [barrier verifier] Unmarked edge: shape, at js/src/gc/Verifier.cpp:597

> help(gczeal)
    4: Verify pre write barriers between instructions
Attached file stack (gdb)
CCing bz given that this looks like something related to event handlers and adoptNode...
I think I know how to fix this. I'll put a patch together tomorrow.
Assignee: nobody → wmccloskey
Attached patch fixSplinter Review
This assertion signals a missing write barrier. The problem occurs inside ReserveForTradeGuts. That function calls SetClassAndProto, which allocates a shape or something and thereby triggers a fresh GC. Later on, we trade the guts using memcpy. No write barriers are triggered by the memcpy, and so we assert later on in the write barrier verifier.

There's a special write barrier that happens in ReserveForTradeGuts, and it was supposed to take care of this problem. However, it happens too early. For one thing, it's possible to start a GC after the write barrier. Additionally, after the barrier, we change the shapes of the objects', which means it's not actually barriering the memcpy properly anyhow.

This patch moves the barrier to the end of TradeGuts. The comment explains why that's okay. One annoying problem with doing the tracing later on is that it causes us to hit an assertion in the tracing code that tries to guarantee that the CCW map is up to date with every proxy that gets traced. We hit it because we haven't had a chance to update the map after the swap. However, there's already an auto class that allows us to skip this assertion.

I also added an AutoSuppressGC inside ReserveForTradeGuts. I don't think there's any good reason why we need to do a GC here, and it seems pretty scary. We're messing with a lot of object invariants, and I think things are just a lot simpler if we avoid GC here. However, this part of the patch is optional--just defense-in-depth.
Attachment #739330 - Flags: review?(bhackett1024)
Attached patch shell testsSplinter Review
I tried to get a shell test for this, but I couldn't get it to work and I gave up. However, I think it would make sense to expose JS_TransplantObject to the fuzzers. Maybe they'll find something.
Attachment #739331 - Flags: review?(bhackett1024)
CCing fuzzers about the new testing function.
Ooh, transplant() looks useful!

Can you make it take a second argument to indicate the target compartment?  That way I don't have to do a dance of giving the target compartment a reference and then evalcx'ing in the target compartment.

Rather than adding a "$transplanted" property, which might do odd things with shapes and fail with frozen objects, how about adding a compartmentof() function for the regression test to use?
Comment on attachment 739331 [details] [diff] [review]
shell tests

Jesse pointed out that this should go in a separate bug that's not sensitive. Also, I'll need to figure out a way to avoid transplanting over the global and stuff like that.
Attachment #739331 - Flags: review?(bhackett1024)
Comment on attachment 739330 [details] [diff] [review]
fix

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

::: js/src/jsgc.h
@@ +1286,5 @@
> +
> +    ~AutoDisableProxyCheck() {
> +        count--;
> +    }
> +};

Why are the extra uses of this class necessary?

The version of this in jscompartment.cpp was #ifdef DEBUG, but this doesn't seem to be.  It looks like all the users of rt->gcDisableStrictProxyCheckingCount are also #ifdef DEBUG, though this isn't super clear from the comments on this class or that field.

For clarity, can you #ifdef DEBUG things so this class is a nop in release builds and rt->gcDisableStrictProxyCheckingCount doesn't exist? (can replace the latter with a padding field so that sizes are consistent between builds)
Attachment #739330 - Flags: review?(bhackett1024) → review+
I'm concerned about exposing transplants directly to the fuzzer because the transplant code makes a number of assumptions about what kinds of objects get passed in. In particular, it asserts against regexps and stuff like that (see JSObject::TradeGuts).

Moreover, this should only be done in the shell, because the constraints in the browser are even more complicated (and transplants need to go through xpc::TransplantObject and friends).
Comment on attachment 739330 [details] [diff] [review]
fix

It looks like this regressed in bug 832360.

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

Not easily.

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

No.

Which older supported branches are affected by this flaw?

Back to FF21.

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

Bug 832360.

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

Should be easy to backport.

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

Fairly unlikely. Some testing on nightly for a few days should be sufficient.
Attachment #739330 - Flags: sec-approval?
Comment on attachment 739330 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 832360
User impact if declined: Possible security problem. Very low likelihood of crashes.
Testing completed (on m-c, etc.): None (waiting on sec-approval)
Risk to taking this patch (and alternatives if risky): Pretty low risk.
String or IDL/UUID changes made by this patch: None.
Attachment #739330 - Flags: approval-mozilla-beta?
Attachment #739330 - Flags: approval-mozilla-aurora?
Attachment #739330 - Flags: sec-approval? → sec-approval+
Adding needsinfo on Bill, to land this on mozilla-inbound/central asap , in preparation for the uplift on branches.
Flags: needinfo?(wmccloskey)
Speculatively marking as sec-critical, since sec-approval was requested.
Keywords: sec-critical
https://hg.mozilla.org/mozilla-central/rev/ec44739db921
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Comment on attachment 739330 [details] [diff] [review]
fix

Please make sure to land on beta asap, as we are going to build today with ,Fx21 beta4.
Attachment #739330 - Flags: approval-mozilla-beta?
Attachment #739330 - Flags: approval-mozilla-beta+
Attachment #739330 - Flags: approval-mozilla-aurora?
Attachment #739330 - Flags: approval-mozilla-aurora+
Matt, can you please make sure this gets verified?
Keywords: verifyme
QA Contact: mwobensmith
Confirmed crash on 2013-04-18, m-c
Confirmed fixed on 2013-04-24, beta 21.0
Confirmed fixed on 2013-04-25, aurora 22.0a2 and m-c 23.0a1
Thank you very much Matt.
Keywords: verifyme
Whiteboard: [adv-main21-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.