1715026 - Crash in [@ nsCOMPtr_base::assign_with_AddRef | nsObserverList::AddObserver] called from AvailableMemoryTracker::Init()

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

UAF crash in AvailableMemoryTracker::Init that started in nightly 90. They're all 32-bit windows. First build-id with a crash is 2021-5-28

Crash report: https://crash-stats.mozilla.org/report/index/03202c81-cdaf-4d59-b79a-9b8ba0210607

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0 xul.dll nsCOMPtr_base::assign_with_AddRef xpcom/base/nsCOMPtr.cpp:38
1 xul.dll nsObserverList::AddObserver xpcom/ds/nsObserverList.cpp:16
2 xul.dll nsObserverService::AddObserver xpcom/ds/nsObserverService.cpp:224
3 xul.dll mozilla::AvailableMemoryTracker::Init xpcom/base/AvailableMemoryTracker.cpp:515
4 xul.dll XREMain::XRE_mainRun toolkit/xre/nsAppRunner.cpp:5241
5 xul.dll XREMain::XRE_main toolkit/xre/nsAppRunner.cpp:5442
6 xul.dll XRE_main toolkit/xre/nsAppRunner.cpp:5501
7 firefox.exe wmain toolkit/xre/nsWindowsWMain.cpp:138
8 firefox.exe __scrt_common_main_seh /builds/worker/workspace/obj-build/browser/app/f:/dd/vctools/crt/vcstartup/src/startup/exe_common.inl:288
9 kernel32.dll BaseThreadInitThunk 

Comment 1

[Andrew McCreight [:mccr8]]

Any ideas? I'm not sure what could be going wrong here. Everything looks properly rooted on the stack to me.

Comment 2

[Andrew McCreight [:mccr8]]

System memory use is at 95%. Maybe there's a low memory pressure event happening in the middle of AvailableMemoryTracker::Init? I still can't see how that would cause a UAF but I could imagine it leading to an unexpected situation.

Comment 3

[Andrew McCreight [:mccr8]]

I'm going to downgrade this to sec-moderate because this is during parent process startup, so I don't know how it could be exploited.

Comment 4

[Toshihito Kikuchi [:toshi]]

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

System memory use is at 95%. Maybe there's a low memory pressure event happening in the middle of AvailableMemoryTracker::Init? I still can't see how that would cause a UAF but I could imagine it leading to an unexpected situation.

I totally agree. That's the scenario which might cause this, but I still don't understand how this can happen.

What happened was nsAvailableMemoryWatcher was already freed when registering self to observe the first topic "quit-application" in nsAvailableMemoryWatcher::Init. After bug 1712084, we decrement the refcount in nsAvailableMemoryWatcher::LowMemoryCallback, but we already incremented it in RegisterMemoryResourceHandler and the callback is called only once because we specify WT_EXECUTEONLYONCE.

I tried to reproduce the issue by freezing the main thread just after RegisterMemoryResourceHandler and made a low-memory situation, but no luck to get repro.

We can move the call to RegisterMemoryResourceHandler() in nsAvailableMemoryWatcher::Init after registering the watcher to the observer service, but not sure it helps. Gabriele, do you think of any other scenario?

Comment 5

[Gabriele Svelto [:gsvelto]]

(In reply to Toshihito Kikuchi [:toshi] from comment #4)

We can move the call to RegisterMemoryResourceHandler() in nsAvailableMemoryWatcher::Init after registering the watcher to the observer service, but not sure it helps. Gabriele, do you think of any other scenario?

Yes, the same issue can happen during shutdown. Every time we call ListenForLowMemory() the callback might trigger before we bump up the refcount. The only truly robust solution I can think of is the following:

1. Move the AddRef() call before RegisterWaitForSingleObject().
2. Instead of bumping up the refcount if RegisterWaitForSingleObject() suceeds we decrease it if it fails. Basically replacing this block.

Everything else should work as advertised. The worst that can happen is that we retain the object a little longer.

Comment 6

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1715026 - Properly transfer ownership of the memory pressure watcher to the memory resource callback r=tkikuchi

Comment 7

[Toshihito Kikuchi [:toshi]]

(In reply to Gabriele Svelto [:gsvelto] from comment #5)

Yes, the same issue can happen during shutdown. Every time we call ListenForLowMemory() the callback might trigger before we bump up the refcount. The only truly robust solution I can think of is the following:

1. Move the AddRef() call before RegisterWaitForSingleObject().
2. Instead of bumping up the refcount if RegisterWaitForSingleObject() suceeds we decrease it if it fails. Basically replacing this block.

Everything else should work as advertised. The worst that can happen is that we retain the object a little longer.

Ah, that makes sense. The callback can be triggered between RegisterWaitForSingleObject and AddRef in RegisterMemoryResourceHandler, not after. When I created that situation, I confirmed nsAvailableMemoryWatcher was freed, and the address was reused for a different object, which was RunnableTask in my case, so it can pass the line of this->AddRef(). Thanks!

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Gabriele Svelto [:gsvelto] from comment #5)

Yes, the same issue can happen during shutdown. Every time we call ListenForLowMemory() the callback might trigger before we bump up the refcount.

If this can happen in shutdown, should we upgrade it back to sec-high?

Comment 9

[Gabriele Svelto [:gsvelto]]

I don't know, the race is very narrow and relies on the machine's memory being low in order to be triggered. We don't have crashes on record at shutdown and that's probably because with more thread activity the window is too narrow. At startup on the other hand it's common because we haven't spun up a single thread yet.

Comment 10

[Gabriele Svelto [:gsvelto]]

Comment on attachment 9225689 [details]
Bug 1715026 - Properly transfer ownership of the memory pressure watcher to the memory resource callback r=tkikuchi

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The patch touches the code where the race is happening but doesn't clarify the conditions under which it happens (low-memory situation during startup/shutdown).
• 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 1712084
• 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?: Not likely but we need to test the patch under the given conditions. I tested it on a low-memory machine that is similar to those that were experiencing this problem.

Comment 12

[Tom Ritter [:tjr]]

Comment on attachment 9225689 [details]
Bug 1715026 - Properly transfer ownership of the memory pressure watcher to the memory resource callback r=tkikuchi

sec-approved to land and if needed uplift

Comment 13

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

Properly transfer ownership of the memory pressure watcher to the memory resource callback r=tkikuchi
https://hg.mozilla.org/integration/autoland/rev/0a5c47d4a336cfffcfd09465ff6d699a4b364dc4
https://hg.mozilla.org/mozilla-central/rev/0a5c47d4a336

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:gsvelto, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 15

[Gabriele Svelto [:gsvelto]]

Comment on attachment 9225689 [details]
Bug 1715026 - Properly transfer ownership of the memory pressure watcher to the memory resource callback r=tkikuchi

Beta/Release Uplift Approval Request

• User impact if declined: Firefox may crash on startup or shutdown
• Is this code covered by automated tests?: No
• 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): The patch fixes a crash that was accidentally introduced when we tried fixing the reference counting the first time, as such it carries no more risk than the original faulty fix
• String changes made/needed: none

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 9225689 [details]
Bug 1715026 - Properly transfer ownership of the memory pressure watcher to the memory resource callback r=tkikuchi

approved for 90.0b8, thanks

Comment 17

[Julien Cristau [:jcristau]]

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