[CrOS MultiDevice] Fix crash in BleConnectionManagerImpl.

When a SecureChannel disconnects due to authentication failures,
BleConnectionManagerImpl invokes the relevant failure callback, which
then causes ConnectionAttemptBase to cancel the connection attempt.

When the attempt is cancelled, the SecureChannel object is deleted via
BleConnectionManagerImpl::ProcessPotentialLingeringChannel(), but
BleConnectionManagerImpl::HandleSecureChannelDisconnection() assumed
that the object was still valid and tried to to remove itself as an
observer, causing a segfault.

The fix is to check whether the object has been deleted before
attempting to call RemoveObserver().

Bug: 905904
Change-Id: Ic593ef4047ff481e218c89028324eb0863131cdc
Reviewed-on: https://chromium-review.googlesource.com/c/1352855Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611781}

chromeos/services/secure_channel/ble_connection_manager_impl.cc

@@ -503,6 +503,15 @@ void BleConnectionManagerImpl::HandleSecureChannelDisconnection(
   // connection's latency should be reset and started anew.
   remote_device_id_to_timestamps_map_[remote_device_id]->Reset();
 
+  if (!DoesAuthenticatingChannelExist(remote_device_id)) {
+    PA_LOG(ERROR) << "BleConnectionManagerImpl::"
+                  << "HandleSecureChannelDisconnection(): Disconnected channel "
+                  << "not present in map. Remote device ID: "
+                  << cryptauth::RemoteDeviceRef::TruncateDeviceIdForLogs(
+                         remote_device_id);
+    NOTREACHED();
+  }
+
   for (const auto& details : GetDetailsForRemoteDevice(remote_device_id)) {
     switch (details.connection_role()) {
       // Initiator role devices are notified of authentication errors as well as
@@ -525,10 +534,19 @@ void BleConnectionManagerImpl::HandleSecureChannelDisconnection(
     }
   }
 
+  // It is possible that the NotifyBle*Failure() calls above resulted in
+  // observers responding to the failure by canceling the connection attempt.
+  // If all attempts to |remote_device_id| were cancelled, the disconnected
+  // channel will have already been cleaned up via
+  // ProcessPotentialLingeringChannel().
+  auto it = remote_device_id_to_secure_channel_map_.find(remote_device_id);
+  if (it == remote_device_id_to_secure_channel_map_.end())
+    return;
+
   // Stop observing the disconnected channel and remove it from the map.
-  remote_device_id_to_secure_channel_map_[remote_device_id]
-      .first->RemoveObserver(this);
-  remote_device_id_to_secure_channel_map_.erase(remote_device_id);
+  SecureChannelWithRole& secure_channel_with_role = it->second;
+  secure_channel_with_role.first->RemoveObserver(this);
+  remote_device_id_to_secure_channel_map_.erase(it);
 
   // Since the previous connection failed, the connection attempts that were
   // paused in SetAuthenticatingChannel() need to be started up again. Note

chromeos/services/secure_channel/ble_connection_manager_impl_unittest.cc

@@ -468,7 +468,8 @@ class SecureChannelBleConnectionManagerImplTest : public testing::Test {
 
   void AttemptBleListenerConnection(const DeviceIdPair& device_id_pair,
                                     ConnectionPriority connection_priority,
-                                    bool expected_to_add_request) {
+                                    bool expected_to_add_request,
+                                    bool should_cancel_attempt_on_failure) {
     SetInRemoteDeviceIdToMetadataMap(
         device_id_pair, ConnectionRole::kListenerRole, connection_priority);
 
@@ -480,7 +481,8 @@ class SecureChannelBleConnectionManagerImplTest : public testing::Test {
             true /* created_via_background_advertisement */),
         base::BindRepeating(
             &SecureChannelBleConnectionManagerImplTest::OnBleListenerFailure,
-            base::Unretained(this), device_id_pair));
+            base::Unretained(this), device_id_pair,
+            should_cancel_attempt_on_failure));
 
     if (expected_to_add_request) {
       EXPECT_TRUE(fake_ble_scanner()->HasScanFilter(BleScanner::ScanFilter(
@@ -576,7 +578,9 @@ class SecureChannelBleConnectionManagerImplTest : public testing::Test {
   void SimulateSecureChannelDisconnection(
       const std::string& remote_device_id,
       bool fail_during_authentication,
-      cryptauth::FakeSecureChannel* fake_secure_channel) {
+      cryptauth::FakeSecureChannel* fake_secure_channel,
+      size_t num_initiator_attempts_canceled_from_disconnection = 0u,
+      size_t num_listener_attempts_canceled_from_disconnection = 0u) {
     size_t num_ble_initiator_failures_before_call =
         ble_initiator_failures_.size();
     size_t num_ble_listener_failures_before_call =
@@ -595,8 +599,12 @@ class SecureChannelBleConnectionManagerImplTest : public testing::Test {
 
     // Iterate through all pending requests to |remote_device_id|, ensuring that
     // all expected failures have been communicated back to the client.
-    size_t initiator_failures_index = num_ble_initiator_failures_before_call;
-    size_t listener_failure_index = num_ble_listener_failures_before_call;
+    size_t initiator_failures_index =
+        num_ble_initiator_failures_before_call +
+        num_initiator_attempts_canceled_from_disconnection;
+    size_t listener_failure_index =
+        num_ble_listener_failures_before_call +
+        num_listener_attempts_canceled_from_disconnection;
     for (const auto& tuple :
          remote_device_id_to_metadata_map_[remote_device_id]) {
       switch (std::get<1>(tuple)) {
@@ -771,16 +779,18 @@ class SecureChannelBleConnectionManagerImplTest : public testing::Test {
     if (!should_cancel_attempt_on_failure)
       return;
 
-    base::PostTask(FROM_HERE,
-                   base::BindOnce(&SecureChannelBleConnectionManagerImplTest::
-                                      CancelBleInitiatorConnectionAttempt,
-                                  base::Unretained(this), device_id_pair));
+    CancelBleInitiatorConnectionAttempt(device_id_pair);
   }
 
   void OnBleListenerFailure(const DeviceIdPair& device_id_pair,
+                            bool should_cancel_attempt_on_failure,
                             BleListenerFailureType failure_type) {
     ble_listener_failures_.push_back(
         std::make_pair(device_id_pair, failure_type));
+    if (!should_cancel_attempt_on_failure)
+      return;
+
+    CancelBleListenerConnectionAttempt(device_id_pair);
   }
 
   void SetInRemoteDeviceIdToMetadataMap(
@@ -934,13 +944,7 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
   AttemptBleInitiatorConnection(pair, ConnectionPriority::kLow,
                                 true /* expected_to_add_request */,
                                 true /* should_cancel_attempt_on_failure */);
-
   SimulateBleFailureToGenerateAdvertisement(pair);
-
-  // Runs the cleanup routine that has been posted as a parallel task. It has
-  // not yet been run because |scoped_task_environment_| was instantiated with
-  // base::test::ScopedTaskEnvironment::ExecutionMode::QUEUED.
-  scoped_task_environment_.RunUntilIdle();
 }
 
 TEST_F(SecureChannelBleConnectionManagerImplTest,
@@ -962,6 +966,25 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
   CancelBleInitiatorConnectionAttempt(pair);
 }
 
+TEST_F(SecureChannelBleConnectionManagerImplTest,
+       OneRequest_Initiator_FailsAuthenticationThenCanceled_CleanupOnCallback) {
+  DeviceIdPair pair(test_devices()[1].GetDeviceId(),
+                    test_devices()[0].GetDeviceId());
+
+  AttemptBleInitiatorConnection(pair, ConnectionPriority::kLow,
+                                true /* expected_to_add_request */,
+                                true /* should_cancel_attempt_on_failure */);
+
+  cryptauth::FakeSecureChannel* fake_secure_channel =
+      SimulateConnectionEstablished(test_devices()[1],
+                                    ConnectionRole::kInitiatorRole);
+  SimulateSecureChannelDisconnection(
+      pair.remote_device_id(), true /* fail_during_authentication */,
+      fake_secure_channel,
+      1u /* num_initiator_attempts_canceled_from_disconnection */,
+      0u /* num_listener_attempts_canceled_from_disconnection */);
+}
+
 TEST_F(SecureChannelBleConnectionManagerImplTest,
        OneRequest_Initiator_GattFailureThenCanceled) {
   DeviceIdPair pair(test_devices()[1].GetDeviceId(),
@@ -1004,7 +1027,8 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
                     test_devices()[0].GetDeviceId());
 
   AttemptBleListenerConnection(pair, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */);
+                               true /* expected_to_add_request */,
+                               false /* should_cancel_attempt_on_failure */);
   UpdateBleListenerConnectionPriority(pair, ConnectionPriority::kMedium,
                                       true /* expected_to_update_priority */);
   UpdateBleListenerConnectionPriority(pair, ConnectionPriority::kHigh,
@@ -1025,7 +1049,8 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
                     test_devices()[0].GetDeviceId());
 
   AttemptBleListenerConnection(pair, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */);
+                               true /* expected_to_add_request */,
+                               false /* should_cancel_attempt_on_failure */);
 
   cryptauth::FakeSecureChannel* fake_secure_channel =
       SimulateConnectionEstablished(test_devices()[1],
@@ -1037,13 +1062,33 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
   CancelBleListenerConnectionAttempt(pair);
 }
 
+TEST_F(SecureChannelBleConnectionManagerImplTest,
+       OneRequest_Listener_FailsAuthenticationThenCanceled_CleanupOnCallback) {
+  DeviceIdPair pair(test_devices()[1].GetDeviceId(),
+                    test_devices()[0].GetDeviceId());
+
+  AttemptBleListenerConnection(pair, ConnectionPriority::kLow,
+                               true /* expected_to_add_request */,
+                               true /* should_cancel_attempt_on_failure */);
+
+  cryptauth::FakeSecureChannel* fake_secure_channel =
+      SimulateConnectionEstablished(test_devices()[1],
+                                    ConnectionRole::kListenerRole);
+  SimulateSecureChannelDisconnection(
+      pair.remote_device_id(), true /* fail_during_authentication */,
+      fake_secure_channel,
+      0u /* num_initiator_attempts_canceled_from_disconnection */,
+      1u /* num_listener_attempts_canceled_from_disconnection */);
+}
+
 TEST_F(SecureChannelBleConnectionManagerImplTest,
        OneRequest_Listener_GattFailureThenCanceled) {
   DeviceIdPair pair(test_devices()[1].GetDeviceId(),
                     test_devices()[0].GetDeviceId());
 
   AttemptBleListenerConnection(pair, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */);
+                               true /* expected_to_add_request */,
+                               false /* should_cancel_attempt_on_failure */);
 
   cryptauth::FakeSecureChannel* fake_secure_channel =
       SimulateConnectionEstablished(test_devices()[1],
@@ -1061,7 +1106,8 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
                     test_devices()[0].GetDeviceId());
 
   AttemptBleListenerConnection(pair, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */);
+                               true /* expected_to_add_request */,
+                               false /* should_cancel_attempt_on_failure */);
 
   cryptauth::FakeSecureChannel* fake_secure_channel =
       SimulateConnectionEstablished(test_devices()[1],
@@ -1149,9 +1195,11 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
                       test_devices()[0].GetDeviceId());
 
   AttemptBleListenerConnection(pair_1, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */);
+                               true /* expected_to_add_request */,
+                               false /* should_cancel_attempt_on_failure */);
   AttemptBleListenerConnection(pair_2, ConnectionPriority::kMedium,
-                               true /* expected_to_add_request */);
+                               true /* expected_to_add_request */,
+                               false /* should_cancel_attempt_on_failure */);
 
   cryptauth::FakeSecureChannel* fake_secure_channel_1 =
       SimulateConnectionEstablished(test_devices()[1],
@@ -1179,9 +1227,11 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
                       test_devices()[0].GetDeviceId());
 
   AttemptBleListenerConnection(pair_1, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */);
+                               true /* expected_to_add_request */,
+                               false /* should_cancel_attempt_on_failure */);
   AttemptBleListenerConnection(pair_2, ConnectionPriority::kMedium,
-                               true /* expected_to_add_request */);
+                               true /* expected_to_add_request */,
+                               false /* should_cancel_attempt_on_failure */);
 
   cryptauth::FakeSecureChannel* fake_secure_channel_1 =
       SimulateConnectionEstablished(test_devices()[1],
@@ -1204,7 +1254,8 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
                     test_devices()[0].GetDeviceId());
 
   AttemptBleListenerConnection(pair, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */);
+                               true /* expected_to_add_request */,
+                               false /* should_cancel_attempt_on_failure */);
   AttemptBleInitiatorConnection(pair, ConnectionPriority::kMedium,
                                 true /* expected_to_add_request */,
                                 false /* should_cancel_attempt_on_failure */);
@@ -1235,7 +1286,8 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
                     test_devices()[0].GetDeviceId());
 
   AttemptBleListenerConnection(pair, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */);
+                               true /* expected_to_add_request */,
+                               false /* should_cancel_attempt_on_failure */);
   AttemptBleInitiatorConnection(pair, ConnectionPriority::kMedium,
                                 true /* expected_to_add_request */,
                                 false /* should_cancel_attempt_on_failure */);
@@ -1254,7 +1306,8 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
                     test_devices()[0].GetDeviceId());
 
   AttemptBleListenerConnection(pair, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */);
+                               true /* expected_to_add_request */,
+                               false /* should_cancel_attempt_on_failure */);
 
   cryptauth::FakeSecureChannel* fake_secure_channel =
       SimulateConnectionEstablished(test_devices()[1],
@@ -1281,7 +1334,8 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
                     test_devices()[0].GetDeviceId());
 
   AttemptBleListenerConnection(pair, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */);
+                               true /* expected_to_add_request */,
+                               false /* should_cancel_attempt_on_failure */);
   AttemptBleInitiatorConnection(pair, ConnectionPriority::kMedium,
                                 true /* expected_to_add_request */,
                                 false /* should_cancel_attempt_on_failure */);
@@ -1305,7 +1359,9 @@ TEST_F(SecureChannelBleConnectionManagerImplTest, ConnectionMetrics) {
 
   // Start a connection attempt and advance the clock.
   AttemptBleListenerConnection(pair, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */);
+                               true /* expected_to_add_request */,
+                               false /* should_cancel_attempt_on_failure */);
+
   test_clock()->Advance(kScanToAdvertisementTime);
 
   // Simulate a connection being established, then disconnected.