1301343 - Investigate tracing of data belong to helper threads

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

As noted in bug 1292590, off thread ion compilations hold GC pointers and the current design (cancel compilation when we start sweeping) may be insufficient.

Comment 1

[Jon Coppeard (:jonco)]

Ideas from Jan on IRC:

we could maintain a linked list of GC pointers stored in MIR instructions. If we only add to the list in IonBuilder (main thread), it should be safe for the GC to trace that list if the builder is off-thread

There are also TI things to look at, and we may duplicate MIR instructions off-thread, that wouldn't work well with moving GC

Comment 2

[Jan de Mooij [:jandem]]

Hiding this just to be sure.

I think an IonGCPointer<T> class makes sense. We would use that at least for the script pointer stored in CompileInfo. This class could maintain a linked list stored in TempAllocator. (We used to have something like that at some point, but then we removed it.)

A lot of pointers in MIR instructions come directly from one of the scripts (atoms in particular). Maybe we could use a special type for these and not add them to the list, but that optimization might not be worth it.

Comment 3

[Jan de Mooij [:jandem]]

A linked list doesn't work or a number of reasons: MConstant uses a union, other MIR instructions use a (resizable) Vector, etc.

There's actually a much nicer and more efficient design I want to work out today.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch

I tried a few different designs today. I think this one is the most simple.

This patch:

(1) Ensures we also trace IonBuilders that are currently being compiled or are in the lazy link list.

(2) Adds a separate pass to append scripts and all GC pointers stored in MIR instructions to an MRootList class. IonBuilder::trace then traces these pointers.

I verified this fixes the fuzz tests in bug 1295039 comment 5 and bug 1297062.

Comment 5

[Jon Coppeard (:jonco)]

Comment on attachment 8789831 [details] [diff] [review]
Patch

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

This looks really nice.  Thanks for fixing this!

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8789831 [details] [diff] [review]
Patch

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

nit: MLoadTypedArrayElementStatic::appendRoots  and  MNewNamedLambdaObject::appendRoots  are missing.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Updated patch

Updated patch. Good eye, Nicolas! :)

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8794191 [details] [diff] [review]
Updated patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Not easily... Maybe with some effort.

> 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?
All.

> If not all supported branches, which bug introduced the flaw?
Something exposed this recently. I think we should just land this everywhere to be safe.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Shouldn't be too hard to backport.

> How likely is this patch to cause regressions; how much testing does it need?
Need to keep an eye on benchmark performance on AWFY, but other than that it should be pretty safe.

Comment 11

[Jan de Mooij [:jandem]]

Bug 1292590 should land everywhere first.

Comment 12

[Marcia Knous [:marcia]]

Makes sense to track this for 52.

Comment 13

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

Comment on attachment 8794191 [details] [diff] [review]
Updated patch

As a sec-audit issue, this doesn't need to have sec-approval to go in. We should just check this into trunk with any other necessary bugs.

Comment 14

[Jan de Mooij [:jandem]]

(In reply to Al Billings [:abillings] from comment #13)
> As a sec-audit issue, this doesn't need to have sec-approval to go in. We
> should just check this into trunk with any other necessary bugs.

Sorry, I missed that this was sec-audit. It's actually sec-high/sec-crit because it fixes actual crashes (bug 1295039, bug 1298135, and others).

Comment 15

[Jan de Mooij [:jandem]]

Comment on attachment 8794191 [details] [diff] [review]
Updated patch

See comment 10.

Comment 16

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

sec-approval+ for trunk. Please nominate Aurora and Beta patches to go in once this is fixed. 

My one question is whether we should really take this on ESR45 since that is an awful big patch.

Comment 17

[Jan de Mooij [:jandem]]

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

(In reply to Al Billings [:abillings] from comment #16)
> My one question is whether we should really take this on ESR45 since that is
> an awful big patch.

It is a big patch but most of it is safe/mechanical. There's a small part that we should definitely land on ESR45, the other changes we could probably do without as they're more audit-ish. First, let's see if this affects performance on AWFY..

Comment 18

[Phil Ringnalda (:philor)]

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

Comment 19

[Jan de Mooij [:jandem]]

Comment on attachment 8794191 [details] [diff] [review]
Updated patch

Approval Request Comment
[Feature/regressing bug #]: Older bug.
[User impact if declined]: Crashes, security bugs.
[Describe test coverage new/current, TreeHerder]: Fixes some fuzz tests.
[Risks and why]: On m-c for a while. Large patch but most of it is very mechanical and safe.
[String/UUID change made/needed]: None.

Comment 20

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

Comment on attachment 8794191 [details] [diff] [review]
Updated patch

Sec-crit, Aurora51+, Beta50+, ESR45+

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Needs rebasing for Beta (and presumably ESR45).

Comment 22

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/releases/mozilla-beta/rev/568a9ce78d2f816af262945b40b888a0e6d98951

Comment 23

[Jan de Mooij [:jandem]]

Attachment: Patch for ESR45

Rebasing this for ESR45 is very painful. Here's a much smaller patch that doesn't trace the pointers stored in individual MIR instructions. It still ensures we trace all scripts and all builders, and should fix the issues the fuzzers found.

Comment 24

[Jon Coppeard (:jonco)]

Comment on attachment 8798064 [details] [diff] [review]
Patch for ESR45

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

LGTM.

Comment 25

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/e5d51ca7a3c039c84175580623855d10671fafad