1594181 - AddressSanitizer: heap-use-after-free [@ Id] with READ of size 4 through [@ mozilla::PBenchmarkStorageChild::SendPut]

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P1)

Description

[Christian Holler (:decoder)]

The attached crash information was submitted via the ASan Nightly Reporter on mozilla-central-asan-nightly revision 72.0a1-20191028215046-https://hg.mozilla.org/mozilla-central/rev/c65ef27b6fc78ec5140068913209bad3b55f1139.

For detailed crash information, see attachment.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 3

[Andrew McCreight (out of office until 8/21) [:mccr8]]

mozilla::DecoderBenchmark::Put accesses an instance of PBenchmarkStorageChild via the static method BenchmarkStorageChild::Instance(), which gets the value via the static variable in sChild. Nothing clears this variable, so if the StorageChild goes away, then sChild is a dead pointer. At a minimum, the dtor of BenchmarkStorageChild() should clear sChild if it is equal to this.

Comment 4

[Andrew McCreight (out of office until 8/21) [:mccr8]]

ContentChild::sSingleton never gets cleared either, but maybe ContentChild is guaranteed to outlive any runnables?

Comment 5

[Nils Ohlmeier [:drno]]

P1 for sec-high.

Alex can you please have a look at this?

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Andrew McCreight [:mccr8] from comment #4)

ContentChild::sSingleton never gets cleared either, but maybe ContentChild is guaranteed to outlive any runnables?

ContentChild should live as long as the process: it has a stub Release method, and the destructor is never called (except in debug builds for leak checking).

Comment 7

[Christian Holler (:decoder)]

ASan builds also perform the shutdown like debug builds, so this could be an ASan/debug only issue. We have had a few of these already before.

Comment 8

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1594181 - Reset external variable properly on destruction. r?mccr8

Comment 9

[Alex Chronopoulos [:achronop]]

Thank you for filing. The patch above clears the static variable on the destructor of the object that is holding. I avoid adding too many details in the commit message.

Comment 10

[Alex Chronopoulos [:achronop]]

Comment on attachment 9108044 [details]
Bug 1594181 - Reset external variable properly on destruction. r?mccr8

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not easy, reproducing it must depend on timing.
• 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?: 70, 71
• If not all supported branches, which bug introduced the flaw?: Bug 1530996
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: It's very easy to create. The same patch applies cleanly in beta and release.
• How likely is this patch to cause regressions; how much testing does it need?: Not likely to cause any regression. It's a small fix that clears properly a static variable on the destruction.

Comment 11

[Tom Ritter [:tjr]]

Comment on attachment 9108044 [details]
Bug 1594181 - Reset external variable properly on destruction. r?mccr8

Let's request uplift then land.

Comment 12

[Alex Chronopoulos [:achronop]]

Comment on attachment 9108044 [details]
Bug 1594181 - Reset external variable properly on destruction. r?mccr8

Beta/Release Uplift Approval Request

• User impact if declined: Intermitten UAF
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• 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): It's very easy to create. The same patch applies cleanly in beta and release.
• String changes made/needed:

Comment 13

[Pascal Chevrel:pascalc]

(In reply to Tom Ritter [:tjr] (needinfo for responses to sec-[approval/ratings/advisories/cve's]) from comment #11)

Comment on attachment 9108044 [details]
Bug 1594181 - Reset external variable properly on destruction. r?mccr8

Let's request uplift then land.

Tom, do you want the uplift and the landing on central to happen at the same time? I am happy to take this patch in the next beta if you are waiting for this information before landing to central.

Comment 14

[Pascal Chevrel:pascalc]

Alex, see question above please, thanks.

Comment 15

[Alex Chronopoulos [:achronop]]

It's low risk, I am fine if uplift and landing in beta happen at the same time. Thank you.

Comment 16

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

https://hg.mozilla.org/integration/autoland/rev/a67caf145931c59a6471afd2dc0f7bb68f2fe6f4
https://hg.mozilla.org/mozilla-central/rev/a67caf145931

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9108044 [details]
Bug 1594181 - Reset external variable properly on destruction. r?mccr8

Fixes a sec bug, approved for 71.0b12.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e34e3ab748ad