1600351 - Crash in [@ objc_release | nsOSHelperAppService::IsCurrentAppOSDefaultForProtocol]

Status: RESOLVED FIXED

(Firefox :: File Handling, defect)

Description

[[:philipp]]

[Tracking Requested - why for this release]:

This bug is for crash report bp-2a75ae0d-f247-4cfe-a80e-13de80191129.

Top 10 frames of crashing thread:

0 libobjc.A.dylib objc_release 
1 XUL nsOSHelperAppService::IsCurrentAppOSDefaultForProtocol uriloader/exthandler/mac/nsOSHelperAppService.mm:184
2 XUL nsMIMEInfoBase::LaunchWithURI uriloader/exthandler/nsMIMEInfoImpl.cpp:337
3 XUL nsExternalHelperAppService::LoadURI uriloader/exthandler/nsExternalHelperAppService.cpp:994
4 XUL mozilla::dom::ContentParent::RecvLoadURIExternal dom/ipc/ContentParent.cpp:3905
5 XUL mozilla::dom::PContentParent::OnMessageReceived ipc/ipdl/PContentParent.cpp:7714
6 XUL mozilla::ipc::MessageChannel::DispatchMessage ipc/glue/MessageChannel.cpp:2208
7 XUL mozilla::ipc::MessageChannel::MessageTask::Run ipc/glue/MessageChannel.cpp:2003
8 XUL nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1250
9 XUL NS_ProcessPendingEvents xpcom/threads/nsThreadUtils.cpp:434

this macos crash signature is starting to show up on nightly after the patch for bug 1496380 landed. the crashing address of most reports indicates that this is a security sensitive issue (UAF).

Comment 1

[:Gijs (out for now; he/him)]

Sometimes I figure, some day this codebase is gonna run out of ways of showing I'm an idiot, but clearly today is not that day. One lives in hope.

Comment 2

[:Gijs (out for now; he/him)]

Attachment: Bug 1600351 - don't release things we don't own, r?barret

We get the bundle using CFBundleGetMainBundle(), which uses the
"Get" ownership rule:

https://developer.apple.com/documentation/corefoundation/1537085-cfbundlegetmainbundle?language=objc

See also https://developer.apple.com/library/archive/documentation/CoreFoundation/Conceptual/CFMemoryMgmt/Concepts/Ownership.html#//apple_ref/doc/uid/20001148-SW1

So we should not be calling CFRelease on it.

Comment 3

[:Gijs (out for now; he/him)]

(I'm landing this because it's nightly only so I don't need sec-approval.)

Comment 4

[:Gijs (out for now; he/him)]

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

Comment 5

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

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

Comment 6

[Julien Cristau [:jcristau]]

Thanks for the super quick fix!