1561404 - AddressSanitizer: attempting to call malloc_usable_size() for pointer which is not owned: 0x60c00854c800 [@ __interceptor_malloc_usable_size] through [@ mozilla::MediaCacheStream::SizeOfExcludingThis]

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 69.0a1-20190618041042-https://hg.mozilla.org/mozilla-central/rev/9b4c8fb46d850db196b0ed6aad0a35b85b178745.

For detailed crash information, see attachment.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 3

[Nils Ohlmeier [:drno]]

Jean-Yves can you please have a look at this or redirect to someone else if you are too swamped right now?

Comment 4

[Jean-Yves Avenard [:jya]]

It seems that the MediaCacheStream member are accessed for memory accounting in a non-threadsafe fashion

MediaCache methods requires a "proof of lock" since bug 1420798. The threading model was rather simple previously, all was happening on the main thread. However following project quantum this is now mostly done in a dedicated thread.

The MediaCache access the MediaCacheStream::BlockList on MediaCache::sThread and with a lock.

However, when attempting to report the memory use, it's all done without.

Here we need to make MediaCache::SizeOfExcludingThis only ever called on the MediaCache::sThread.

This code is already called from an asynchronous method. Needs to extend on that so that we don't attempt to read MediaCache elements outside its thread. Most of the infrastucture is already in place.

This bug is a regression due to bug 1497124

Comment 5

[Tarek Ziadé (:tarek)]

Hello, can I have access to bug 1479399 please?

Comment 6

[Jean-Yves Avenard [:jya]]

A bit silly if you can be assigned a bug, yet still require to be CCed on it

Comment 7

[Tarek Ziadé (:tarek)]

Attachment: Bug 1561404 - Ensure thread safety

Adds a lock in MediaCacheStream::SizeOfExcludingThis to prevent a race
condition with MediaCache::NoteSeek

Comment 8

[Tarek Ziadé (:tarek)]

try https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f9e3a22db44a7b7a9e1cb35cd3d7a7352e7251d

Comment 9

[Tarek Ziadé (:tarek)]

I guess I can't land myself?

Christian I guess I'll let you take it from here

Comment 10

[Tarek Ziadé (:tarek)]

Comment on attachment 9075900 [details]
Bug 1561404 - Ensure thread safety

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I don't know
• 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?:
• If not all supported branches, which bug introduced the flaw?: Bug 1497124
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: The crash should go away with this patch and we should not be affected by a new regression. ChromeUtils::requestPerformanceMetrics() (via about:performance) is the only place where this race condition might occur.

Comment 11

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

Marking affected flags as Firefox 65 and higher based on this being broken by Bug 1497124.

Sec-approval+ for trunk. We'll want this on Beta as well.

Comment 12

[Christian Holler (:decoder)]

Marking for sheriffs to land.

Comment 13

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

https://hg.mozilla.org/integration/autoland/rev/e0daf57c45af2396ed431f3912af17c02b77a1bb

Comment 14

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

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

Comment 15

[Julien Cristau [:jcristau]]

Please request beta and esr68 approval when you get a chance.

Comment 16

[Tarek Ziadé (:tarek)]

Comment on attachment 9075900 [details]
Bug 1561404 - Ensure thread safety

Beta/Release Uplift Approval Request

• User impact if declined:
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• 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): ony called via about:performance and this potential race condition was only detected by asan
• String changes made/needed:

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): ony called via about:performance and this potential race condition was only detected by asan
• String or UUID changes made by this patch:

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9075900 [details]
Bug 1561404 - Ensure thread safety

Fixes a sec issue in about:performance. Approved for 69.0b6 and 68.1esr.

Comment 18

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

https://hg.mozilla.org/releases/mozilla-beta/rev/83fbec55d5b9

Comment 19

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

https://hg.mozilla.org/releases/mozilla-esr68/rev/d1e0829f0a00