789130 - Not removing PHal listeners on abnormal subprocess termination

Status: RESOLVED FIXED

(Core :: Hardware Abstraction Layer (HAL), defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

STR (intermittent)
 (1) Load video app or cut the rope
 (2) |adb shell kill -9 [plugin-container]|
 (3) Rapidly change device orientation

HalParent Register*()s for several notifications, but on crash it doesn't Unregister*().

Comment 1

[Justin Lebar (not reading bugmail)]

> (3) Rapidly change device orientation

Sorry...what happens after this?  Does the main process crash?

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Unregister hal observers on HalParent::ActorDestroy(), and make Unregister*() functions resilient to unregister-of-not-registered observers

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Justin Lebar [:jlebar] from comment #1)
> > (3) Rapidly change device orientation
> 
> Sorry...what happens after this?  Does the main process crash?

BOOM in main process.  uaf

Comment 5

[Justin Lebar (not reading bugmail)]

Comment on attachment 658978 [details] [diff] [review]
Unregister hal observers on HalParent::ActorDestroy(), and make Unregister*() functions resilient to unregister-of-not-registered observers

>diff --git a/hal/Hal.cpp b/hal/Hal.cpp
>@@ -180,40 +180,29 @@ public:
>     mObservers->AddObserver(aObserver);
> 
>     if (mObservers->Length() == 1) {
>       EnableNotifications();
>     }
>   }
> 
>   void RemoveObserver(Observer<InfoType>* aObserver) {
>-    // If mObservers is null, that means there are no observers.
>-    // In addition, if RemoveObserver() returns false, that means we didn't
>-    // find the observer.
>-    // In both cases, that is a logical error we want to make sure the developer
>-    // notices.
>-
>-    MOZ_ASSERT(mObservers);

All that arguing, and now we're deleting the assertion!  :)

>diff --git a/hal/sandbox/SandboxHal.cpp b/hal/sandbox/SandboxHal.cpp
>+  virtual void
>+  ActorDestroy(ActorDestroyReason aWhy) MOZ_OVERRIDE
>+  {
>+    // NB: you *must* unconditionally unregister your observer here,
>+    // if it *may* be registered below.
>+    hal::UnregisterBatteryObserver(this);
>+    hal::UnregisterNetworkObserver(this);
>+    hal::UnregisterScreenConfigurationObserver(this);
>+    for (int32_t sensor = SENSOR_UNKNOWN + 1;
>+         sensor < NUM_SENSOR_TYPE; ++sensor) {
>+      hal::UnregisterSensorObserver(SensorType(sensor), this);
>+    }
>+    hal::UnregisterWakeLockObserver(this);

What about UnregisterSwitchObserver?

(I wonder if there's some way to assert this, so we don't regress this method.)

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Justin Lebar [:jlebar] from comment #5)
> Comment on attachment 658978 [details] [diff] [review]
> Unregister hal observers on HalParent::ActorDestroy(), and make
> Unregister*() functions resilient to unregister-of-not-registered observers
> 
> >diff --git a/hal/Hal.cpp b/hal/Hal.cpp
> >@@ -180,40 +180,29 @@ public:
> >     mObservers->AddObserver(aObserver);
> > 
> >     if (mObservers->Length() == 1) {
> >       EnableNotifications();
> >     }
> >   }
> > 
> >   void RemoveObserver(Observer<InfoType>* aObserver) {
> >-    // If mObservers is null, that means there are no observers.
> >-    // In addition, if RemoveObserver() returns false, that means we didn't
> >-    // find the observer.
> >-    // In both cases, that is a logical error we want to make sure the developer
> >-    // notices.
> >-
> >-    MOZ_ASSERT(mObservers);
> 
> All that arguing, and now we're deleting the assertion!  :)
> 

We have a strong use case now where the overhead of client logic to maintain "registered or not" is higher than making the hal API softer.  We didn't have that at the time.

I don't regret the assertion.  It's caught real bugs.

> >diff --git a/hal/sandbox/SandboxHal.cpp b/hal/sandbox/SandboxHal.cpp
> >+  virtual void
> >+  ActorDestroy(ActorDestroyReason aWhy) MOZ_OVERRIDE
> >+  {
> >+    // NB: you *must* unconditionally unregister your observer here,
> >+    // if it *may* be registered below.
> >+    hal::UnregisterBatteryObserver(this);
> >+    hal::UnregisterNetworkObserver(this);
> >+    hal::UnregisterScreenConfigurationObserver(this);
> >+    for (int32_t sensor = SENSOR_UNKNOWN + 1;
> >+         sensor < NUM_SENSOR_TYPE; ++sensor) {
> >+      hal::UnregisterSensorObserver(SensorType(sensor), this);
> >+    }
> >+    hal::UnregisterWakeLockObserver(this);
> 
> What about UnregisterSwitchObserver?
> 
> (I wonder if there's some way to assert this, so we don't regress this
> method.)

Since allowing content to register for switch events is a security policy change, it will need sr and I trust sr's here to be aware of unregister.  But I don't have a strong opinion either way.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc088d2b2006

Comment 8

[Justin Lebar (not reading bugmail)]

Surely a rogue content process could call RegisterSwitchObserver()?

Anyway, it strikes me as super-weird to have a comment saying "we must unconditionally register all the things" and then intentionally leave one thing out, even if the omission is safe.

Comment 9

[Justin Lebar (not reading bugmail)]

> Surely a rogue content process could call RegisterSwitchObserver()?

Maybe the point is "I don't care about content processes safely crashing the parent process"?  I still don't get it, though.  Just unregister the thing and we're safe, right?

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

A rogue content process calling RegisterSwitchObserver() would be entirely ignored by the parent process.  In fact, the content process would be killed off.  I may not be understanding what you're referring to.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I thought you meant, "In a future where we allow that, we should add a static check now to ensure we don't fuck up the Unregister() that would be needed."

Comment 12

[Ed Morley [:emorley]]

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