sensors: Add locking when passing sensor updates to the client

This change updates the Win32 and WinRT sensor backends to acquire the
lock when calling back into the client. This is important because the
client_ variable is set to nullptr when the sensor reader is destroyed
and so synchronization is needed to prevent a null pointer dereference
or use after free.

Bug: 1023503
Change-Id: Ie677c7a7376e1b01bacaad66264439c5f5af6a0e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2911135
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Auto-Submit: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Chris Mumford <cmumford@google.com>
Cr-Commit-Position: refs/heads/master@{#885336}

services/device/generic_sensor/platform_sensor_reader_win.cc

@@ -490,7 +490,8 @@ bool PlatformSensorReaderWin32::SetReportingInterval(
 
 HRESULT PlatformSensorReaderWin32::SensorReadingChanged(
     ISensorDataReport* report,
-    SensorReading* reading) const {
+    SensorReading* reading) {
+  base::AutoLock autolock(lock_);
   if (!client_)
     return E_FAIL;
 
@@ -501,6 +502,7 @@ HRESULT PlatformSensorReaderWin32::SensorReadingChanged(
 }
 
 void PlatformSensorReaderWin32::SensorError() {
+  base::AutoLock autolock(lock_);
   if (client_)
     client_->OnSensorError();
 }

services/device/generic_sensor/platform_sensor_reader_win.h

@@ -8,6 +8,8 @@
 #include <SensorsApi.h>
 #include <wrl/client.h>
 
+#include "base/synchronization/lock.h"
+#include "base/thread_annotations.h"
 #include "services/device/generic_sensor/platform_sensor_reader_win_base.h"
 #include "services/device/public/mojom/sensor.mojom.h"
 
@@ -52,7 +54,7 @@ class PlatformSensorReaderWin32 final : public PlatformSensorReaderWinBase {
       WARN_UNUSED_RESULT;
   void ListenSensorEvent();
   HRESULT SensorReadingChanged(ISensorDataReport* report,
-                               SensorReading* reading) const WARN_UNUSED_RESULT;
+                               SensorReading* reading) WARN_UNUSED_RESULT;
   void SensorError();
 
  private:
@@ -64,8 +66,8 @@ class PlatformSensorReaderWin32 final : public PlatformSensorReaderWinBase {
   // StartSensor and StopSensor are called from another thread by
   // PlatformSensorWin that can modify internal state of the object.
   base::Lock lock_;
-  bool sensor_active_;
-  Client* client_;
+  bool sensor_active_ GUARDED_BY(lock_);
+  Client* client_ GUARDED_BY(lock_);
   Microsoft::WRL::ComPtr<ISensor> sensor_;
   Microsoft::WRL::ComPtr<ISensorEvents> event_listener_;
   base::WeakPtrFactory<PlatformSensorReaderWin32> weak_factory_{this};

services/device/generic_sensor/platform_sensor_reader_winrt.cc

@@ -344,6 +344,11 @@ HRESULT PlatformSensorReaderWinrtLightSensor::OnReadingChangedCallback(
   if (!has_received_first_sample_ ||
       (abs(lux - last_reported_lux_) >=
        (last_reported_lux_ * kLuxPercentThreshold))) {
+    base::AutoLock autolock(lock_);
+    if (!client_) {
+      return S_OK;
+    }
+
     SensorReading reading;
     reading.als.value = lux;
     reading.als.timestamp = timestamp_delta.InSecondsF();
@@ -417,6 +422,11 @@ HRESULT PlatformSensorReaderWinrtAccelerometer::OnReadingChangedCallback(
       (abs(x - last_reported_x_) >= kAxisThreshold) ||
       (abs(y - last_reported_y_) >= kAxisThreshold) ||
       (abs(z - last_reported_z_) >= kAxisThreshold)) {
+    base::AutoLock autolock(lock_);
+    if (!client_) {
+      return S_OK;
+    }
+
     // Windows.Devices.Sensors.Accelerometer exposes acceleration as
     // proportional and in the same direction as the force of gravity.
     // The generic sensor interface exposes acceleration simply as
@@ -497,6 +507,11 @@ HRESULT PlatformSensorReaderWinrtGyrometer::OnReadingChangedCallback(
       (abs(x - last_reported_x_) >= kDegreeThreshold) ||
       (abs(y - last_reported_y_) >= kDegreeThreshold) ||
       (abs(z - last_reported_z_) >= kDegreeThreshold)) {
+    base::AutoLock autolock(lock_);
+    if (!client_) {
+      return S_OK;
+    }
+
     // Windows.Devices.Sensors.Gyrometer exposes angular velocity as degrees,
     // but the generic sensor interface uses radians so the data must be
     // converted.
@@ -576,6 +591,11 @@ HRESULT PlatformSensorReaderWinrtMagnetometer::OnReadingChangedCallback(
       (abs(x - last_reported_x_) >= kMicroteslaThreshold) ||
       (abs(y - last_reported_y_) >= kMicroteslaThreshold) ||
       (abs(z - last_reported_z_) >= kMicroteslaThreshold)) {
+    base::AutoLock autolock(lock_);
+    if (!client_) {
+      return S_OK;
+    }
+
     SensorReading reading;
     reading.magn.x = x;
     reading.magn.y = y;
@@ -654,6 +674,11 @@ PlatformSensorReaderWinrtAbsOrientationEulerAngles::OnReadingChangedCallback(
       (abs(x - last_reported_x_) >= kDegreeThreshold) ||
       (abs(y - last_reported_y_) >= kDegreeThreshold) ||
       (abs(z - last_reported_z_) >= kDegreeThreshold)) {
+    base::AutoLock autolock(lock_);
+    if (!client_) {
+      return S_OK;
+    }
+
     SensorReading reading;
     reading.orientation_euler.x = x;
     reading.orientation_euler.y = y;
@@ -762,6 +787,11 @@ PlatformSensorReaderWinrtAbsOrientationQuaternion::OnReadingChangedCallback(
   auto angle =
       abs(GetAngleBetweenOrientationSamples(reading, last_reported_sample));
   if (!has_received_first_sample_ || (angle >= kRadianThreshold)) {
+    base::AutoLock autolock(lock_);
+    if (!client_) {
+      return S_OK;
+    }
+
     client_->OnReadingUpdated(reading);
 
     last_reported_sample = reading;

services/device/generic_sensor/platform_sensor_reader_winrt.h

@@ -14,6 +14,7 @@
 
 #include "base/callback.h"
 #include "base/synchronization/lock.h"
+#include "base/thread_annotations.h"
 #include "services/device/generic_sensor/platform_sensor_reader_win_base.h"
 #include "services/device/public/cpp/generic_sensor/sensor_reading.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"
@@ -95,7 +96,7 @@ class PlatformSensorReaderWinrtBase : public PlatformSensorReaderWinBase {
   // threads by PlatformSensorWin.
   base::Lock lock_;
   // Null if there is no client to notify, non-null otherwise.
-  Client* client_;
+  Client* client_ GUARDED_BY(lock_);
 
   // Always report the first sample received after starting the sensor.
   bool has_received_first_sample_ = false;