1148161 - OfflineObserver seems to be racy

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

OfflineObserver seems like it's intended to be thread-safe. I don't think it is though. The way it's used, a class Foo is expected to create an OfflineObserver in Foo's constructor and call RemoveObserver in ~Foo. RemoveObserver just dispatches an event to the main thread to remove the observer. Also, it nulls out the mParent pointer. However, there's no synchronization when mParent is cleared. If the observer fires while Foo's destructor is running, the observer might read a stale value for mParent and we'd have a UAF.

I think this could be fixed by making mParent Atomic.

Comment 1

[Valentin Gosu [:valentin] (he/him)]

Attachment: OfflineObserver seems to be racy

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 8584621 [details] [diff] [review]
OfflineObserver seems to be racy

Review of attachment 8584621 [details] [diff] [review]:
-----------------------------------------------------------------

the mParent usage is a perfect example of a very badly written code...

::: netwerk/base/OfflineObserver.cpp
@@ +78,2 @@
>    RemoveOfflineObserver();
>    mParent = nullptr;

protect just mParent access, never call foreign code under a lock if you don't know what you are doing!

@@ +91,4 @@
>    if (mParent &&
>        !strcmp(aTopic, NS_IOSERVICE_APP_OFFLINE_STATUS_TOPIC)) {
>      mParent->OfflineNotification(aSubject);
>    }

swap to a local var under the lock and then check/call on that local copy outside the lock.

(unfortunately the class is not supporting referencing, so it may get released - if not ensured by the upper level code somehow - under your hand easily)

Comment 3

[Valentin Gosu [:valentin] (he/him)]

Attachment: Make OfflineObserver::mParent atomic

Comment 4

[Honza Bambas (:mayhemer)]

Comment on attachment 8751213 [details] [diff] [review]
Make OfflineObserver::mParent atomic

Review of attachment 8751213 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/OfflineObserver.cpp
@@ +82,5 @@
>  {
>    if (mParent &&
>        !strcmp(aTopic, NS_IOSERVICE_APP_OFFLINE_STATUS_TOPIC)) {
> +    DisconnectableParent* parent = mParent;
> +    parent->OfflineNotification(aSubject);

local copy makes mostly sense only when you have a lock to access mParent under but must call it's methods outside the lock.  Tho, it expects the assignment under the lock ensures lifetime of the object in this local block, outside the lock.  Which usually means to addref or ensure an ownership here.

This code is still far from perfect:

T1 (main thread):
  OfflineObserver::Observe
  if (mParent    // mParent is non null

T2: 
  OfflineObserver::RemoveObserver()
  mParent = null;

T1:
  parent = mParent;
  parent->..()  // you crash on null ptr


You may think of doing the following:

DisconnectableParent* parent = mParent; //atomic on this line
if (parent) {
  parent->...();  // this may crash on a bad ptr when an other thread already called RemoveObserver and released the object...


This code is whole wrong...  you will need to implement t-safe ref counting on DisconnectableParent base class and then use RefPtr.

Non of the comments says anything about who holds the ownership of DisconnectableParent objects, so it's hard to just use UniquePtr or something here and claim the ownership solely to this class only.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Attachment: OfflineObserver seems to be racy

Added comments regarding the proper implementation OfflineDisconnect in order to not cause a deadlock

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 8751696 [details] [diff] [review]
OfflineObserver seems to be racy

Review of attachment 8751696 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the last nit fixed.

::: netwerk/base/OfflineObserver.cpp
@@ +76,2 @@
>    RemoveOfflineObserver();
>    mParent = nullptr;

just hold the lock over the mParent = nullptr and do it before the removeofflineobserver call.

Comment 7

[Valentin Gosu [:valentin] (he/him)]

Attachment: OfflineObserver seems to be racy

r+ in comment 6

Comment 8

[Valentin Gosu [:valentin] (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e86f3f05b07184e777440a7b3f99803e8ea153d
Bug 1148161 - OfflineObserver seems to be racy r=honzab

Comment 9

[Carsten Book [:Tomcat]]

sorry had to back this out for causing assertions like https://treeherder.mozilla.org/logviewer.html#?job_id=27750857&repo=mozilla-inbound#L1591

[Parent 2602] ###!!! ASSERTION: Mismatched sizes were recorded in the memory leak logging table. The usual cause of this is having a templated class that uses MOZ_COUNT_{C,D}TOR in the constructor or destructor, respectively. As a workaround, the MOZ_COUNT_{C,D}TOR calls can be moved to a non-templated base class.: 'aInstanceSize == 0 || entry->GetClassSize() == aInstanceSize', file /home/worker/workspace/build/src/xpcom/base/nsTraceRefcnt.cpp, line 463
 12:24:50     INFO -  #01: NS_LogAddRef [xpcom/base/nsTraceRefcnt.cpp:1105]
 12:24:50     INFO -  #02: mozilla::net::OfflineObserver::AddRef [netwerk/base/OfflineObserver.cpp:16]
 12:24:50     INFO -  #03: nsCOMPtr<nsISupports>::nsCOMPtr [xpcom/glue/nsCOMPtr.h:815]
 12:24:50     INFO -  #04: nsObserverList::AddObserver [xpcom/glue/nsTArray.h:1590]
 12:24:50     INFO -  #05: nsObserverService::AddObserver [xpcom/ds/nsObserverService.cpp:247]
 12:24:50     INFO -  #06: mozilla::net::OfflineObserver::RegisterOfflineObserverMainThread [netwerk/base/OfflineObserver.cpp:50]
 12:24:50     INFO -  #07: mozilla::net::OfflineObserver::RegisterOfflineObserver [netwerk/base/OfflineObserver.cpp:22]
 12:24:50     INFO -  #08: mozilla::net::NeckoParent::NeckoParent [mfbt/RefPtr.h:52]
 12:24:50     INFO -  #09: mozilla::dom::ContentParent::AllocPNeckoParent [dom/ipc/ContentParent.cpp:3807]
 12:24:50     INFO -  #10: mozilla::dom::PContentParent::OnMessageReceived [obj-firefox/ipc/ipdl/PContentParent.cpp:4346]
 12:24:50     INFO -  #11: mozilla::ipc::MessageChannel::DispatchAsyncMessage [ipc/glue/MessageChannel.h:547]
 12:24:50     INFO -  #12: mozilla::ipc::MessageChannel::DispatchMessage [ipc/glue/MessageChannel.cpp:1595]
 12:24:50     INFO -  #13: mozilla::ipc::MessageChannel::OnMaybeDequeueOne [ipc/glue/MessageChannel.cpp:1549]
 12:24:50     INFO -  #14: nsRunnableMethodImpl<bool (mozilla::ipc::MessageChannel::*)(), false, true>::Run [xpcom/glue/nsThreadUtils.h:744]
 12:24:50     INFO -  #15: mozilla::ipc::MessageChannel::DequeueTask::Run [ipc/glue/MessageChannel.h:497]
12:24:50 INFO - #16: M

Comment 10

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c985d2ee3251

Comment 11

[Valentin Gosu [:valentin] (he/him)]

I found a slightly different trace by running ./mach test layout/reftests/bugs/508908-1.xul

Seems that the problem is that the name of net::OfflineObserver clashes with PluginModuleParent::OfflineObserver.
I need to rename one of them.

#01: GetBloatEntry(char const*, unsigned int) (/xpcom/base/nsTraceRefcnt.cpp:461 (discriminator 5))
#02: NS_LogAddRef (/xpcom/base/nsTraceRefcnt.cpp:1104)
#03: OfflineObserver::AddRef() (/dom/plugins/ipc/PluginModuleParent.cpp:2050 (discriminator 14))
#04: nsCOMPtr<nsIObserver>::assign_with_AddRef(nsISupports*) (/obj-ff-dbg/dist/include/nsCOMPtr.h:1078)
#05: nsCOMPtr<nsIObserver>::operator=(nsIObserver*) (/obj-ff-dbg/dist/include/nsCOMPtr.h:579)
#06: mozilla::plugins::PluginModuleChromeParent::RegisterSettingsCallbacks() (/dom/plugins/ipc/PluginModuleParent.cpp:2075 (discriminator 2))
#07: mozilla::plugins::PluginModuleChromeParent::OnProcessLaunched(bool) (/dom/plugins/ipc/PluginModuleParent.cpp:562)
#08: mozilla::plugins::PluginModuleChromeParent::LaunchedTask::Run() (/dom/plugins/ipc/PluginModuleParent.h:627)
#09: mozilla::plugins::PluginProcessParent::RunLaunchCompleteTask() (/dom/plugins/ipc/PluginProcessParent.cpp:218 (discriminator 1))
#10: mozilla::plugins::PluginProcessParent::WaitUntilConnected(int) (/dom/plugins/ipc/PluginProcessParent.cpp:233)
#11: mozilla::plugins::PluginModuleChromeParent::LoadModule(char const*, unsigned int, nsPluginTag*) (/dom/plug

Comment 12

[Valentin Gosu [:valentin] (he/him)]

Attachment: rename PluginModuleParent::OfflineObserver to PluginOfflineObserver

The class name clashes with mozilla::net::OfflineObserver and ends up crashing in nsTraceRefcnt::GetBloatEntry because the two classes have a different size.

Comment 13

[Valentin Gosu [:valentin] (he/him)]

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

Comment 14

[Valentin Gosu [:valentin] (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c637546f5eba53bb32de94a61980950a3d4800e4
Bug 1148161 - OfflineObserver seems to be racy r=honzab

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0210cfd50bab984df1680033542e036694a6a89
Bug 1148161 - rename PluginModuleParent::OfflineObserver to PluginOfflineObserver r=jimm

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c637546f5eba
https://hg.mozilla.org/mozilla-central/rev/b0210cfd50ba