1676812 - Crash in [@ mozilla::telemetry::Timers::AnnotateHang]

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

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

The code used here got added in bug 1661304.

4 crashes on 4 different machines.

Crash report: https://crash-stats.mozilla.org/report/index/85b7778a-b557-4678-aee2-fc28e0201030

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 9 frames of crashing thread:

0 xul.dll mozilla::telemetry::Timers::AnnotateHang toolkit/components/telemetry/core/Stopwatch.cpp:540
1 xul.dll mozilla::BackgroundHangAnnotators::GatherAnnotations toolkit/components/backgroundhangmonitor/HangAnnotations.cpp:83
2 xul.dll static mozilla::BackgroundHangManager::MonitorThread toolkit/components/backgroundhangmonitor/BackgroundHangMonitor.cpp:83
3 nss3.dll _PR_NativeRunThread nsprpub/pr/src/threads/combined/pruthr.c:399
4 nss3.dll pr_root nsprpub/pr/src/md/windows/w95thred.c:139
5 ucrtbase.dll thread_start<unsigned int , 1> 
6 kernel32.dll BaseThreadInitThunk 
7 ntdll.dll RtlUserThreadStart 
8 kernelbase.dll TerminateProcessOnMemoryExhaustion 

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Making this a security bug out of an abundance of caution for now, since this might be a (Nightly-only) UAF.

If I'm reading this stack correctly, it looks like the BHR monitor thread is trying to access the mBHRAnnotationTimers LinkedList, and this is causing a read access violation. Hey nika, do you remember how the mutex locking is supposed to work here for annotations? I would have thought that the monitored thread (in this case, the main thread) would stay locked while the monitor thread does its job collecting the annotations... how could the LinkedList be freed unless the Timers object had gone away? Or am I misinterpreting this, and the crash happening because an element from the LinkedList has been freed but remained in the list somehow?

Comment 2

[Nika Layzell [:nika] (ni? for response)]

BHR annotations are collected here: https://searchfox.org/mozilla-central/source/toolkit/components/backgroundhangmonitor/BackgroundHangMonitor.cpp#418, which is done while the BackgroundHangMonitor's lock and the BackgroundHangAnnotators lock are being held, but not when the main thread is paused. The main thread is only paused when calling the profiler to collect backtrace information, as we can't even allocate memory while the main thread is paused, in-case jemalloc is holding a global lock or similar. If you want to collect hang annotation information, you need to make sure that the AnnotateHang method is threadsafe, as BHR doesn't actually provide you any synchronization on it.

I think you need a mutex around this linked list.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Alright, looks like I'm adding a mutex. Thanks, nika!

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1676812 - Use a DataMutex to wrap the BHR HangAnnotations LinkedList. r?dthayer!

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 9189279 [details]
Bug 1676812 - Use a DataMutex to wrap the BHR HangAnnotations LinkedList. r?dthayer!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I don't know. This is a UAF in our part of our Telemetry code that's currently only exercised in Nightly builds (where BHR is enabled). It also only runs in the parent process.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• Which older supported branches are affected by this flaw?:
• If not all supported branches, which bug introduced the flaw?: Bug 1661304
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: I don't think this is necessary to backport. Like I mentioned above, the codepath is only ever entered on the Nightly channel.
• How likely is this patch to cause regressions; how much testing does it need?: I'm basically adding a mutex lock here to do some synchronization. I don't believe this will require more testing than what we already get from our Nightly population.

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 9189279 [details]
Bug 1676812 - Use a DataMutex to wrap the BHR HangAnnotations LinkedList. r?dthayer!

sec-moderate bugs don't need sec-approval.

Comment 7

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

https://hg.mozilla.org/integration/autoland/rev/7688d75b1d558f85becb080ecd81bad7e62be56e
https://hg.mozilla.org/mozilla-central/rev/7688d75b1d55