1540166 - Don't run forgetSkippable during SnowWhiteFreeing

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

This is related to bug https://bugzilla.mozilla.org/show_bug.cgi?id=1318810
The theory is that event loop spins in some destructor. That isn't too good practice, but also we shouldn't try to run forgetSkippable.

When manually forcing forgetSkippable to happen during snowwhitefreeing, we're crashing at the same place as in https://bugzilla.mozilla.org/show_bug.cgi?id=1318810 on android.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1540166, avoid some useless ForgetSkippable handling while we're already dealing with snow-white objects, r=mccr8

Comment 2

[Andrew McCreight [:mccr8]]

Thanks for looking at this. I looked over the other places where we use the purple buffer, and the rest are either guarded like this, are deep inside the CC, or are inside Suspect methods, which I think the code intends to handle.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9054508 [details]
Bug 1540166, avoid some useless ForgetSkippable handling while we're already dealing with snow-white objects, r=mccr8

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I don't know how to trigger the crash (expect manually injecting forgetSkippable calls). But the theory is that event loop spins at unexpected time.
• 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, assuming the event loop spinning happens on all the branches
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: The same patch seem to applies at least to esr60.
• How likely is this patch to cause regressions; how much testing does it need?: Should be safe, since we just don't run CC optimization phase if event loop spins at odd time.

Comment 4

[Daniel Veditz [:dveditz]]

this is to fix (some of?) the crashes in bug 1318810, and in those signatures I see a bunch of EXCEPTION_ACCESS_VIOLATION_EXEC crashes at exploitable-looking addresses like bp-31fce5d1-4f6c-41f6-8c27-856f60190329

We probably should be making 1318810 a security bug, but in the meantime calling this sec-high because at least these seem to be in the sandboxed content process.

Comment 5

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

Comment on attachment 9054508 [details]
Bug 1540166, avoid some useless ForgetSkippable handling while we're already dealing with snow-white objects, r=mccr8

sec-approval+ for trunk. Let's get a beta patch and ESR60 one nominated as well.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/73ebc8384c0f3790b23bad2a133ba4338a7a8816

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9054508 [details]
Bug 1540166, avoid some useless ForgetSkippable handling while we're already dealing with snow-white objects, r=mccr8

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: NA
• User impact if declined: See comment 3
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):
• String changes made/needed: NA

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined:
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):
• String or UUID changes made by this patch:

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Ah, no, this doesn't need QE.

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/73ebc8384c0f3790b23bad2a133ba4338a7a8816
https://hg.mozilla.org/mozilla-central/rev/73ebc8384c0f

Comment 10

[Andrew McCreight [:mccr8]]

It is probably still too early to tell, but there have been no nsPurpleBufferEntry::Swap crashes since this landed in the Android Nightly. (20190330093331 is the last Android Nightly without this patch.)

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Still looking good.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

... 20190330093331 is the last build with crashes.

Comment 13

[Pascal Chevrel:pascalc]

Comment on attachment 9054508 [details]
Bug 1540166, avoid some useless ForgetSkippable handling while we're already dealing with snow-white objects, r=mccr8

Crashes stopped over the last week after landing on mozilla-central on the depending bugs, approved for 67 beta, thanks.

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/1cdd67f491bf

Comment 15

[Liz Henry (:lizzard)]

Comment on attachment 9054508 [details]
Bug 1540166, avoid some useless ForgetSkippable handling while we're already dealing with snow-white objects, r=mccr8

Fix for sec-high crash, let's take it for 60.7 esr.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/7ac782fad9d2