1358073 - Crash in js::TenuringTracer::traverse<T>

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

This bug was filed from the Socorro interface and is 
report bp-171e9a25-2ff4-4330-8fd6-1b55f0170419.
=============================================================

This has been getting ~500 crashes a day for a long time.

Comment 1

[Jon Coppeard (:jonco)]

These crashes all happen while checking if a pointer is inside the nursery, which is done by checking the chunk trailer.  

Many of these show a crash at address 0xffff0 which indicates a null pointer or pointer into the first 1MiB of address space - an invalid pointer. So these would probably be bad RAM / heap corruption and not directly actionable.

A few of them show traces of 'e5' in the crash address which suggests that these could be use after free though.  The stacks for these show one of:
 - StoreBuffer::MonoTypeBuffer<js::gc::StoreBuffer::CellPtrEdge>::trace
 - jit::MRootList::trace
 - OrderedHashTableRef<js::MapObject>::trace

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

The e5 crashes are use-after-frees (almost any crash with a ...e5e5... address is)

Comment 3

[Stephanie Ouillon [:arroway] (needinfo me)]

Sec-high triage: Jon, we need somebody to be assigned to investigate and fix this bug.

Comment 4

[Jon Coppeard (:jonco)]

Looking at the crashes that have e5 in the address, the ones associated with the most recent builds are all during tracing the cell pointer store buffer.

This store buffer is used by NativeObject::privateWriteBarrierPost and JSObject::writeBarrierPost (which is used by GCPtr, HeapPtr, JS::Heap).  So things like constructing a HeapPtr / JS::Heap and failing to destroy it properly, or memcpy'ing the memory could be to blame here.

Of course, this is pretty hard to track down without STR.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: bug1358073-scope-gcptrs

One thing I found is that we can free scope data in error cases without going via JS::DeletePolicy, which is specialised for some Scope::Data classes to free the data later at a safe time (using GCManagedDeletePolicy).  This could cause crashes like these.

Also we copy Scope data around with PodCopy, which bypasses constructors.  Calling constructors for embedded GCPtrs is necessary to trigger the postbarrier so this is a potential source of crashes if this data contains nursery objects.

Comment 6

[Shu-yu Guo [:shu]]

Comment on attachment 8868172 [details] [diff] [review]
bug1358073-scope-gcptrs

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

Thanks for the patch, and yikes, this was pretty scary oversight on my part. We really should backport this.

Comment 7

[Jon Coppeard (:jonco)]

Comment on attachment 8868172 [details] [diff] [review]
bug1358073-scope-gcptrs

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

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? Everything back to 51.

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

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

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

Comment 8

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk.
Once it lands, we'll want ESR52 and Beta patches nominated so we can uplift this.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/df21c5eed7c11b531228a02d66624960b59c7c5e

Comment 10

[Jon Coppeard (:jonco)]

Attachment: bug1358073-scope-gcptrs part 2

Sorry, I messed up and missed a couple of places where I should have called DeleteScopeData.  This actually makes no difference in practice because these are not for scope kinds that use GCManagedDeletePolicy but for we should do this for consistency with the everything else.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)

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

Comment 12

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec6edd4fec8f62db57c1f075d1d3b536c74af5f8

Comment 13

[Jon Coppeard (:jonco)]

Attachment: bug1358073-beta

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1263355
[User impact if declined]: Possible crash / security vulnerability
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: This isn't a trivial change but I think the risk is low.
[Why is the change risky/not risky?]: It splits moving scope data objects into two parts and so involves pointer arithmetic which can be risky.  But conceptually it's quite simple and it's a small change.
[String changes made/needed]: None

Comment 14

[Jon Coppeard (:jonco)]

Attachment: bug1358073-esr52

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: sec-high bug
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: None

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Is this still leave-open?

Comment 16

[Jon Coppeard (:jonco)]

(In reply to Wes Kocher (:KWierso) from comment #15)
> Is this still leave-open?

Yes, that patch won't fix all of these crashes.

Comment 17

[Julien Cristau [:jcristau]]

Comment on attachment 8870113 [details] [diff] [review]
bug1358073-beta

sec-high fix for beta54, should be in 54.0b12

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6f7e81f34df3

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8870115 [details] [diff] [review]
bug1358073-esr52

Sec-high that was taken in 54, let's uplift to ESR52

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/a5a7fc1e6027

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Backed out from ESR52 for AutoEnterOOMUnsafeRegion crashes.
https://hg.mozilla.org/releases/mozilla-esr52/rev/ae9e08eaf6dc

https://treeherder.mozilla.org/logviewer.html#?job_id=104668239&repo=mozilla-esr52

Comment 22

[Jon Coppeard (:jonco)]

Bug 1370869 will probably fix this.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Sounds like we should punt on this for ESR52 until 52.3 then (and change tracking to 55+). From an IRC conversation with Jon, it doesn't sound like we need to panic for 54, but we should keep an eye out for any uptick in OOM crashes that might result.

Comment 24

[Al Billings [:abillings - ex-MoCo]]

I'm not going to disclose this in 54 since we didn't take it (yet) in 52.2.

Comment 25

[Al Billings [:abillings - ex-MoCo]]

Can we get an ESR52 patch landed now?

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Bug 1370869 needs to be approved first.

Comment 27

[Al Billings [:abillings - ex-MoCo]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #26)
> Bug 1370869 needs to be approved first.

Done.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/8d6e685d061b

Comment 29

[Liz Henry (:lizzard) (relman/hg->git project)]

Is this fixed in 55 and 56? Or is there still something left to land for beta and nightly?

Comment 30

[Jon Coppeard (:jonco)]

There's nothing waiting to land, but the patch that did land will only fix some of the crashes.

Comment 31

[Liz Henry (:lizzard) (relman/hg->git project)]

OK, I'll drop tracking if this isn't something we're expecting to change for 55/56.

Comment 32

[Randell Jesup [:jesup] (needinfo me)]

The patch seems to have halved weekday crashes.  55/56 have 84 crashes in the last week (random ptr/UAFs).  Mixture of writes and reads.

What's needed to get info that would help us diagnose this?  Assertions elsewhere?

Comment 33

[Jon Coppeard (:jonco)]

This isn't actionable as it is, but I'm going to investigate how we can track down crashes like this in bug 1399944.

Comment 34

[Daniel Veditz [:dveditz]]

In the remaining crashes I see only one (ESR 52.4) crash that looks like an obvious UAF. Whatever remains is something different from what was fixed. Would it make more sense to clone this into a new bug and take a fresh look at the remaining, and call this one "FIXED" because we did in fact fix a bunch of crashes?

Comment 35

[Jon Coppeard (:jonco)]

Sure.

Comment 36

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.