1149526 - Check HeapPtrs have GC lifetime

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

The HeapPtr wrapper template is designed to be used only in object which have GC lifetime, i.e. they are only ever part of a GC thing and are destroyed when we sweep that thing.

I added some assertions to test this and found there are a few cases where HeapPtrs are deleted outside of GC sweeping.  This is potentially bad since for a HeapPtr<JSObject*> we could try and mark a store buffer entry that points at it after it has been destroyed.

I don't think we've seen any crashes like this so it's possible that the JSObject ones are only used with tenured objects, but I'm marking this security sensitive just in case.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1149526-check-heapptr-lifetime

Here's a first attempt at adding the assertions and fixing the places that broke (these were traced to things being deleted outside of the GC).

This gets the current zone by looking at the value in the HeapPtr<> and checks that we are sweeping that zone if we are on the main thread.  This doesn't work for cross compartment pointers - but maybe these are all HeapSlots?

I'm also wondering if I should add some state in the zone to say 'this zone is currently being swept in the background' to make that check more precise.

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 8586165 [details] [diff] [review]
bug1149526-check-heapptr-lifetime

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

\o/  This is a huge improvement! We should definitely add state to the zone to make this assertion even tighter; I'd even be fine landing this patch as-is and doing that as a followup, if it's more convenient that way.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: bug1149526-check-heapptr-lifetime v2

Great!  Here's the updated patch.

Comment 4

[Terrence Cole [:terrence]]

Comment on attachment 8587272 [details] [diff] [review]
bug1149526-check-heapptr-lifetime v2

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

Looks good!

::: js/src/gc/Zone.h
@@ +309,3 @@
>      bool jitUsingBarriers_;
>  
> +

Extra newline.

Comment 5

[Jon Coppeard (:jonco)]

Comment on attachment 8587272 [details] [diff] [review]
bug1149526-check-heapptr-lifetime v2

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

Very difficult.

The issue here is that a HeapPtrObject might be destroyed outside of a GC while it holds a nursery pointer.  It's not clear whether this is possible but it's hard to analyse all the paths that might lead to it so best to be safe.

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 FF32.

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

Bug 619558.

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

They will require merging, and the type inference code has probably changed a bit, but backport patches will be mostly the same.

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

Unlikely.

Comment 6

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

Comment on attachment 8587272 [details] [diff] [review]
bug1149526-check-heapptr-lifetime v2

sec-approval+. Let's get this nominated on Aurora and Beta too.

Comment 7

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2652e5cbc37f

Comment 8

[Jon Coppeard (:jonco)]

Backed out for static analysis bustage:

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

Comment 9

[Jon Coppeard (:jonco)]

Fixed and re-landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ccd83f8e32b

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/3ccd83f8e32b

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Aurora/Beta approval ASAP so we can hopefully get it landed in time for 38b3.

Comment 12

[Hannes Verschore [:h4writer]]

AWFY detected a regression/improvement:

First landing: http://arewefastyet.com/regressions/#/regression/139660 (12% regression on misc-f32-markov).
Backout: http://arewefastyet.com/regressions/#/regression/139682 (misc-f32-markov back to normal).

But I have good news. The relanding didn't trigger the regression. So you are clear ;).

Comment 13

[Jon Coppeard (:jonco)]

Comment on attachment 8587272 [details] [diff] [review]
bug1149526-check-heapptr-lifetime v2

Approval Request Comment
[Feature/regressing bug #]: Bug 619558.
[User impact if declined]: Possible crashes / security vulnerability.
[Describe test coverage new/current, TreeHerder]: On mozilla-central since 9th April.
[Risks and why]: Low.
[String/UUID change made/needed]: None.

Comment 14

[Jon Coppeard (:jonco)]

Comment on attachment 8587272 [details] [diff] [review]
bug1149526-check-heapptr-lifetime v2

Actually given that there are fuzz bugs reported caused by this, let's not do this yet.

Comment 15

[Jon Coppeard (:jonco)]

As evidenced by the fuzz bugs filed, this approach doesn't work too well because it involves trying to the zone of the containing object by examining the referent, which may be been finalized at this point.

Comment 16

[Jon Coppeard (:jonco)]

Attachment: bug1149526-use-tls-to-check-heap-ptr-lifetime

If we use TLS to work out whether the current thread is doing GC sweeping then everything becomes much simpler.  Also, it caught a couple more places where we need to use RelocatablePtr.

Comment 17

[Terrence Cole [:terrence]]

Comment on attachment 8590288 [details] [diff] [review]
bug1149526-use-tls-to-check-heap-ptr-lifetime

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

Yeah, that's much nicer.

Comment 18

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/909e17a7edb7

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/909e17a7edb7

Comment 20

[Jon Coppeard (:jonco)]

Attachment: bug1149526-aurora-patch

Comment 21

[Jon Coppeard (:jonco)]

Attachment: bug1149526-beta-patch

Comment 22

[Ryan VanderMeulen [:RyanVM]]

The branch patches still need separate approval to land.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

A b2g37 patch is going to be needed here. Not sure about the older B2G branches yet.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Jon, could you fill the uplift request to the various affected branches? Thanks

Comment 25

[Jon Coppeard (:jonco)]

Comment on attachment 8590716 [details] [diff] [review]
bug1149526-aurora-patch

Approval Request Comment
[Feature/regressing bug #]: Bug 619558.
[User impact if declined]: Possible crashes / security vulnerability.
[Describe test coverage new/current, TreeHerder]: On mozilla-central since 9th April.
[Risks and why]: Low.
[String/UUID change made/needed]: None.

Comment 26

[Jon Coppeard (:jonco)]

Comment on attachment 8590759 [details] [diff] [review]
bug1149526-beta-patch

Approval Request Comment
[Feature/regressing bug #]: Bug 619558.
[User impact if declined]: Possible crashes / security vulnerability.
[Describe test coverage new/current, TreeHerder]: On mozilla-central since 9th April.
[Risks and why]: Low.
[String/UUID change made/needed]: None.

Comment 27

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8590759 [details] [diff] [review]
bug1149526-beta-patch

Should be in 38 beta 5.

Comment 28

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c66b1101f3a9

Comment 29

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/releases/mozilla-beta/rev/7ca7e178de40

Comment 30

[Ryan VanderMeulen [:RyanVM]]

My needinfo was for B2G rebasing :)

Comment 31

[Jon Coppeard (:jonco)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #30)
Oh right, yes I missed that.  Patch coming up.

Comment 32

[Jon Coppeard (:jonco)]

Attachment: bug1149526-b2g-37-patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 619558.
User impact if declined: Possible crashes / security vulnerability.
Testing completed: On mozilla-central since 9th April.
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8592243 [details] [diff] [review]
bug1149526-b2g-37-patch

sec-high bugs don't need extra approval for B2G so long as the affected Fx branches are taken care of :)

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/d4c303a490b6

Comment 35

[Ryan VanderMeulen [:RyanVM]]

I made it about halfway through rebasing this for b2g34 before I hit enough conflicts to call it quits. Might as well look at b2g32 while you're at it since I'm sure that won't be any better.

Comment 36

[Jon Coppeard (:jonco)]

Attachment: bug1149526-b2g-34-patch

Comment 37

[Jon Coppeard (:jonco)]

Attachment: bug1149526-b2g-32-patch

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/bf4ca22510a4
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/bf4ca22510a4
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/8338fade105d
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/8338fade105d