diff --git a/fastpair/BUILD b/fastpair/BUILD index 43c3a171..07970e44 100644 --- a/fastpair/BUILD +++ b/fastpair/BUILD @@ -95,6 +95,7 @@ cc_library( ":fast_pair_plugin", ":fast_pair_seeker", "//fastpair/internal", + "//fastpair/repository:device_repository", "//internal/platform:types", "@com_google_absl//absl/container:flat_hash_map", "@com_google_absl//absl/status", diff --git a/fastpair/dart/fast_pair_wrapper_impl.h b/fastpair/dart/fast_pair_wrapper_impl.h index 0e79e67e..f9c72ea5 100644 --- a/fastpair/dart/fast_pair_wrapper_impl.h +++ b/fastpair/dart/fast_pair_wrapper_impl.h @@ -40,7 +40,7 @@ class FastPairWrapperImpl : public FastPairWrapper { private: SingleThreadExecutor executor_; - FastPairDeviceRepository devices_; + FastPairDeviceRepository devices_{&executor_}; std::unique_ptr scanner_broker_; // True if we are currently scanning for remote devices. diff --git a/fastpair/fast_pair_service.cc b/fastpair/fast_pair_service.cc index 93823b51..5b237339 100644 --- a/fastpair/fast_pair_service.cc +++ b/fastpair/fast_pair_service.cc @@ -33,14 +33,8 @@ constexpr absl::Duration kTimeout = absl::Seconds(3); } FastPairService::FastPairService() { - seeker_ = - std::make_unique(FastPairSeekerImpl::ServiceCallbacks{ - .on_device_added = - [this](std::unique_ptr device) { - AddDevice(std::move(device)); - }, - .on_device_lost = - [this](const FastPairDevice& device) { RemoveDevice(&device); }, + seeker_ = std::make_unique( + FastPairSeekerImpl::ServiceCallbacks{ .on_initial_discovery = [this](const FastPairDevice& device, InitialDiscoveryEvent event) { @@ -66,7 +60,8 @@ FastPairService::FastPairService() { .on_ring_event = [this](const FastPairDevice& device, RingEvent event) { OnRingEvent(device, std::move(event)); - }}); + }}, + &executor_, &devices_); } absl::Status FastPairService::RegisterPluginProvider( @@ -101,25 +96,6 @@ absl::Status FastPairService::UnregisterPluginProvider(absl::string_view name) { : absl::DeadlineExceededError("Unregister plugin timeout"); } -void FastPairService::AddDevice(std::unique_ptr device) { - NEARBY_LOGS(INFO) << "Add device " << *device; - executor_.Execute("add-device", [this, device = std::move(device)]() mutable { - devices_.push_back(std::move(device)); - }); -} - -void FastPairService::RemoveDevice(const FastPairDevice* device) { - NEARBY_LOGS(INFO) << "Remove device " << *device; - executor_.Execute("remove-device", [this, device]() { - devices_.erase( - std::remove_if(devices_.begin(), devices_.end(), - [&](const std::unique_ptr& item) { - return item.get() == device; - }), - devices_.end()); - }); -} - void FastPairService::OnInitialDiscoveryEvent(const FastPairDevice& device, InitialDiscoveryEvent event) { executor_.Execute("on-initial-discovery", [this, device = &device, diff --git a/fastpair/fast_pair_service.h b/fastpair/fast_pair_service.h index 9e9286d8..d02cc2fb 100644 --- a/fastpair/fast_pair_service.h +++ b/fastpair/fast_pair_service.h @@ -25,6 +25,7 @@ #include "absl/strings/string_view.h" #include "fastpair/fast_pair_plugin.h" #include "fastpair/fast_pair_seeker.h" +#include "fastpair/repository/fast_pair_device_repository.h" #include "internal/platform/single_thread_executor.h" namespace nearby { @@ -51,8 +52,6 @@ class FastPairService { FastPairSeeker* GetSeeker() const { return seeker_.get(); } private: - void AddDevice(std::unique_ptr device); - void RemoveDevice(const FastPairDevice* device); void OnInitialDiscoveryEvent(const FastPairDevice& device, InitialDiscoveryEvent event); void OnSubsequentDiscoveryEvent(const FastPairDevice& device, @@ -65,7 +64,7 @@ class FastPairService { std::unique_ptr seeker_; absl::flat_hash_map> providers_; - std::vector> devices_; + FastPairDeviceRepository devices_{&executor_}; }; } // namespace fastpair diff --git a/fastpair/internal/BUILD b/fastpair/internal/BUILD index 553c53b4..1c77de7b 100644 --- a/fastpair/internal/BUILD +++ b/fastpair/internal/BUILD @@ -13,6 +13,8 @@ cc_library( deps = [ "//fastpair:fast_pair_events", "//fastpair:fast_pair_seeker", + "//fastpair/repository:device_repository", + "//internal/platform:types", "@com_google_absl//absl/status", ], ) diff --git a/fastpair/internal/fast_pair_seeker_impl.cc b/fastpair/internal/fast_pair_seeker_impl.cc index 2588996b..35b23b0d 100644 --- a/fastpair/internal/fast_pair_seeker_impl.cc +++ b/fastpair/internal/fast_pair_seeker_impl.cc @@ -44,15 +44,14 @@ absl::Status FastPairSeekerImpl::StartFastPairScan() { // TODO(jsobczak): Replace with actual implementation auto device = std::make_unique( "model_id", "11:22:33:44:55:66", Protocol::kFastPairInitialPairing); - test_device_ = device.get(); - callbacks_.on_device_added(std::move(device)); + test_device_ = devices_->AddDevice(std::move(device)); callbacks_.on_initial_discovery(*test_device_, {}); return absl::OkStatus(); } absl::Status FastPairSeekerImpl::StopFastPairScan() { // TODO(jsobczak): Replace with actual implementation - callbacks_.on_device_lost(*test_device_); + devices_->RemoveDevice(test_device_); return absl::OkStatus(); } diff --git a/fastpair/internal/fast_pair_seeker_impl.h b/fastpair/internal/fast_pair_seeker_impl.h index 6aa6d3c9..204ea2d5 100644 --- a/fastpair/internal/fast_pair_seeker_impl.h +++ b/fastpair/internal/fast_pair_seeker_impl.h @@ -20,6 +20,8 @@ #include "fastpair/fast_pair_events.h" #include "fastpair/fast_pair_seeker.h" +#include "fastpair/repository/fast_pair_device_repository.h" +#include "internal/platform/single_thread_executor.h" namespace nearby { namespace fastpair { @@ -35,9 +37,6 @@ class FastPairSeekerExt : public FastPairSeeker { class FastPairSeekerImpl : public FastPairSeekerExt { public: struct ServiceCallbacks { - absl::AnyInvocable)> on_device_added; - absl::AnyInvocable on_device_lost; - absl::AnyInvocable on_initial_discovery; absl::AnyInvocable @@ -50,8 +49,11 @@ class FastPairSeekerImpl : public FastPairSeekerExt { absl::AnyInvocable on_ring_event; }; - explicit FastPairSeekerImpl(ServiceCallbacks callbacks) - : callbacks_(std::move(callbacks)) {} + FastPairSeekerImpl(ServiceCallbacks callbacks, SingleThreadExecutor* executor, + FastPairDeviceRepository* devices) + : callbacks_(std::move(callbacks)), + executor_(executor), + devices_(devices) {} // From FastPairSeeker. absl::Status StartInitialPairing(FastPairDevice& device, @@ -74,6 +76,9 @@ class FastPairSeekerImpl : public FastPairSeekerExt { private: ServiceCallbacks callbacks_; + SingleThreadExecutor* executor_; + FastPairDeviceRepository* devices_; + FastPairDevice* test_device_ = nullptr; }; diff --git a/fastpair/repository/BUILD b/fastpair/repository/BUILD index 0a83d741..1b6f3db5 100644 --- a/fastpair/repository/BUILD +++ b/fastpair/repository/BUILD @@ -39,7 +39,11 @@ cc_library( ], compatible_with = ["//buildenv/target:non_prod"], visibility = ["//fastpair:__subpackages__"], - deps = ["//fastpair/common"], + deps = [ + "//fastpair/common", + "//internal/platform:logging", + "//internal/platform:types", + ], ) cc_test( @@ -50,6 +54,7 @@ cc_test( deps = [ ":device_repository", "//fastpair/common", + "//internal/platform:types", "//internal/platform/implementation/g3", # build_cleaner: keep "@com_github_protobuf_matchers//protobuf-matchers", "@com_google_googletest//:gtest_main", diff --git a/fastpair/repository/fast_pair_device_repository.cc b/fastpair/repository/fast_pair_device_repository.cc index 0f703ba0..dee735e2 100644 --- a/fastpair/repository/fast_pair_device_repository.cc +++ b/fastpair/repository/fast_pair_device_repository.cc @@ -19,12 +19,16 @@ #include #include +#include "internal/platform/logging.h" +#include "internal/platform/mutex_lock.h" + namespace nearby { namespace fastpair { FastPairDevice* FastPairDeviceRepository::AddDevice( std::unique_ptr device) { const auto& id = device->GetUniqueId(); + MutexLock lock(&mutex_); for (auto& item : devices_) { if (item->GetUniqueId() == id) { // Overwrite the existing object. @@ -38,16 +42,19 @@ FastPairDevice* FastPairDeviceRepository::AddDevice( } void FastPairDeviceRepository::RemoveDevice(const FastPairDevice* device) { - devices_.erase( - std::remove_if(devices_.begin(), devices_.end(), - [&](const std::unique_ptr& item) { - return item.get() == device; - }), - devices_.end()); + std::unique_ptr fast_pair_device = ExtractDevice(device); + if (fast_pair_device == nullptr) return; + // Tasks running in the background may still be referencing `device`. Defering + // the destruction to the background thread should prevent use-after-free + // errors. + executor_->Execute([fast_pair_device = std::move(fast_pair_device)]() { + NEARBY_LOGS(VERBOSE) << "Destroyed FP device: " << fast_pair_device; + }); } std::optional FastPairDeviceRepository::FindDevice( absl::string_view mac_address) { + MutexLock lock(&mutex_); auto it = std::find_if(devices_.begin(), devices_.end(), [&](const std::unique_ptr& device) { return device->GetBleAddress() == mac_address || @@ -60,5 +67,18 @@ std::optional FastPairDeviceRepository::FindDevice( } } +std::unique_ptr FastPairDeviceRepository::ExtractDevice( + const FastPairDevice* device) { + MutexLock lock(&mutex_); + auto it = std::find_if(devices_.begin(), devices_.end(), + [&](const std::unique_ptr& item) { + return item.get() == device; + }); + if (it == devices_.end()) return nullptr; + std::unique_ptr fast_pair_device = std::move(*it); + devices_.erase(it); + return fast_pair_device; +} + } // namespace fastpair } // namespace nearby diff --git a/fastpair/repository/fast_pair_device_repository.h b/fastpair/repository/fast_pair_device_repository.h index d97f77dc..25c31d3c 100644 --- a/fastpair/repository/fast_pair_device_repository.h +++ b/fastpair/repository/fast_pair_device_repository.h @@ -20,6 +20,8 @@ #include #include "fastpair/common/fast_pair_device.h" +#include "internal/platform/mutex.h" +#include "internal/platform/single_thread_executor.h" namespace nearby { namespace fastpair { @@ -27,6 +29,9 @@ namespace fastpair { // Owner of `FastPairDevice` instances. class FastPairDeviceRepository { public: + explicit FastPairDeviceRepository(SingleThreadExecutor* executor) + : executor_(executor) {} + // Adds device to the repository and takes over ownership. // If a device with the same MAC address is already in the repository, it is // replaced. @@ -42,7 +47,11 @@ class FastPairDeviceRepository { std::optional FindDevice(absl::string_view mac_address); private: - std::vector> devices_; + // Removes `device` from `devices_`. + std::unique_ptr ExtractDevice(const FastPairDevice* device); + Mutex mutex_; + SingleThreadExecutor* executor_; + std::vector> devices_ ABSL_GUARDED_BY(mutex_); }; } // namespace fastpair diff --git a/fastpair/repository/fast_pair_device_repository_test.cc b/fastpair/repository/fast_pair_device_repository_test.cc index 9c996955..a9d7e839 100644 --- a/fastpair/repository/fast_pair_device_repository_test.cc +++ b/fastpair/repository/fast_pair_device_repository_test.cc @@ -22,6 +22,7 @@ #include "gtest/gtest.h" #include "fastpair/common/fast_pair_device.h" #include "fastpair/common/protocol.h" +#include "internal/platform/single_thread_executor.h" namespace nearby { namespace fastpair { @@ -32,7 +33,8 @@ constexpr absl::string_view kBleAddress = "AA:BB:CC:DD:EE:FF"; constexpr absl::string_view kBtAddress = "12:34:56:78:90:AB"; TEST(FastPairDeviceRepositoryTest, AddDevice) { - FastPairDeviceRepository repo; + SingleThreadExecutor executor; + FastPairDeviceRepository repo(&executor); FastPairDevice* device = repo.AddDevice(std::make_unique( kModelId, kBleAddress, Protocol::kFastPairInitialPairing)); @@ -42,7 +44,8 @@ TEST(FastPairDeviceRepositoryTest, AddDevice) { } TEST(FastPairDeviceRepositoryTest, FindDeviceByBleAddress) { - FastPairDeviceRepository repo; + SingleThreadExecutor executor; + FastPairDeviceRepository repo(&executor); repo.AddDevice(std::make_unique( kModelId, kBleAddress, Protocol::kFastPairInitialPairing)); @@ -55,7 +58,8 @@ TEST(FastPairDeviceRepositoryTest, FindDeviceByBleAddress) { } TEST(FastPairDeviceRepositoryTest, FindDeviceByBtAddress) { - FastPairDeviceRepository repo; + SingleThreadExecutor executor; + FastPairDeviceRepository repo(&executor); auto fast_pair_device = std::make_unique(Protocol::kFastPairInitialPairing); fast_pair_device->SetPublicAddress(kBtAddress); @@ -70,7 +74,8 @@ TEST(FastPairDeviceRepositoryTest, FindDeviceByBtAddress) { } TEST(FastPairDeviceRepositoryTest, RemoveDevice) { - FastPairDeviceRepository repo; + SingleThreadExecutor executor; + FastPairDeviceRepository repo(&executor); FastPairDevice* device = repo.AddDevice(std::make_unique( kModelId, kBleAddress, Protocol::kFastPairInitialPairing)); @@ -79,6 +84,22 @@ TEST(FastPairDeviceRepositoryTest, RemoveDevice) { EXPECT_FALSE(repo.FindDevice(kBleAddress).has_value()); } +TEST(FastPairDeviceRepositoryTest, RemovingNonRegisteredDeviceIsSafe) { + SingleThreadExecutor executor; + FastPairDeviceRepository repo(&executor); + FastPairDevice* device = repo.AddDevice(std::make_unique( + kModelId, kBleAddress, Protocol::kFastPairInitialPairing)); + FastPairDevice other_device(Protocol::kFastPairInitialPairing); + repo.RemoveDevice(device); + + // `device` already removed. + repo.RemoveDevice(device); + // `other_device` was never added. + repo.RemoveDevice(&other_device); + + EXPECT_FALSE(repo.FindDevice(kBleAddress).has_value()); +} + } // namespace } // namespace fastpair diff --git a/fastpair/scanning/fastpair/fast_pair_discoverable_scanner_impl_test.cc b/fastpair/scanning/fastpair/fast_pair_discoverable_scanner_impl_test.cc index 45ccb2bd..2d213fb3 100644 --- a/fastpair/scanning/fastpair/fast_pair_discoverable_scanner_impl_test.cc +++ b/fastpair/scanning/fastpair/fast_pair_discoverable_scanner_impl_test.cc @@ -76,7 +76,7 @@ TEST(FastPairDiscoverableScannerImplTest, ValidModelId) { auto scanner = std::make_unique(); auto repository = std::make_unique(); SingleThreadExecutor executor; - FastPairDeviceRepository devices; + FastPairDeviceRepository devices(&executor); proto::Device metadata; metadata.set_device_type(proto::DeviceType::TRUE_WIRELESS_HEADPHONES); repository->SetFakeMetadata(kValidModelId, metadata); @@ -103,7 +103,7 @@ TEST(FastPairDiscoverableScannerImplTest, InvalidModelId) { auto scanner = std::make_unique(); auto repository = std::make_unique(); SingleThreadExecutor executor; - FastPairDeviceRepository devices; + FastPairDeviceRepository devices(&executor); proto::Device metadata; metadata.set_device_type(proto::DeviceType::TRUE_WIRELESS_HEADPHONES); repository->SetFakeMetadata(kValidModelId, metadata); @@ -130,7 +130,7 @@ TEST(FastPairDiscoverableScannerImplTest, NoServiceData) { auto scanner = std::make_unique(); auto repository = std::make_unique(); SingleThreadExecutor executor; - FastPairDeviceRepository devices; + FastPairDeviceRepository devices(&executor); proto::Device metadata; metadata.set_device_type(proto::DeviceType::TRUE_WIRELESS_HEADPHONES); repository->SetFakeMetadata(kValidModelId, metadata); @@ -157,7 +157,7 @@ TEST(FastPairDiscoverableScannerImplTest, UnsupportedDeviceType) { auto scanner = std::make_unique(); auto repository = std::make_unique(); SingleThreadExecutor executor; - FastPairDeviceRepository devices; + FastPairDeviceRepository devices(&executor); proto::Device metadata; metadata.set_device_type(proto::DeviceType::AUTOMOTIVE); repository->SetFakeMetadata(kValidModelId, metadata); @@ -184,7 +184,7 @@ TEST(FastPairDiscoverableScannerImplTest, UnsupportedNotifictionType) { auto scanner = std::make_unique(); auto repository = std::make_unique(); SingleThreadExecutor executor; - FastPairDeviceRepository devices; + FastPairDeviceRepository devices(&executor); proto::Device metadata; metadata.set_device_type(proto::DeviceType::HEADPHONES); metadata.set_notification_type(proto::NotificationType::APP_LAUNCH); @@ -215,7 +215,7 @@ TEST(FastPairDiscoverableScannerImplTest, UnspecifiedNotificationType) { // the notification to be safe. auto repository = std::make_unique(); SingleThreadExecutor executor; - FastPairDeviceRepository devices; + FastPairDeviceRepository devices(&executor); proto::Device metadata; metadata.set_device_type(proto::DeviceType::DEVICE_TYPE_UNSPECIFIED); metadata.set_notification_type( @@ -246,7 +246,7 @@ TEST(FastPairDiscoverableScannerImplTest, V1NotificationType) { // type and a notification type of FAST_PAIR_ONE. auto repository = std::make_unique(); SingleThreadExecutor executor; - FastPairDeviceRepository devices; + FastPairDeviceRepository devices(&executor); proto::Device metadata; metadata.set_device_type(proto::DeviceType::DEVICE_TYPE_UNSPECIFIED); metadata.set_notification_type(proto::NotificationType::FAST_PAIR_ONE); @@ -276,7 +276,7 @@ TEST(FastPairDiscoverableScannerImplTest, V2NotificationType) { // type of TRUE_WIRELESS_HEADPHONES and a notification type of FAST_PAIR. auto repository = std::make_unique(); SingleThreadExecutor executor; - FastPairDeviceRepository devices; + FastPairDeviceRepository devices(&executor); proto::Device metadata; metadata.set_device_type(proto::DeviceType::TRUE_WIRELESS_HEADPHONES); metadata.set_notification_type(proto::NotificationType::FAST_PAIR); @@ -304,7 +304,7 @@ TEST(FastPairDiscoverableScannerImplTest, NearbyShareModelId) { auto scanner = std::make_unique(); auto repository = std::make_unique(); SingleThreadExecutor executor; - FastPairDeviceRepository devices; + FastPairDeviceRepository devices(&executor); proto::Device metadata; metadata.set_device_type(proto::DeviceType::TRUE_WIRELESS_HEADPHONES); repository->SetFakeMetadata(kValidModelId, metadata); @@ -331,7 +331,7 @@ TEST(FastPairDiscoverableScannerImplTest, auto scanner = std::make_unique(); auto repository = std::make_unique(); SingleThreadExecutor executor; - FastPairDeviceRepository devices; + FastPairDeviceRepository devices(&executor); proto::Device metadata; metadata.set_device_type(proto::DeviceType::TRUE_WIRELESS_HEADPHONES); repository->SetFakeMetadata(kValidModelId, metadata); diff --git a/fastpair/scanning/scanner_broker_impl_test.cc b/fastpair/scanning/scanner_broker_impl_test.cc index 1720014a..612646df 100644 --- a/fastpair/scanning/scanner_broker_impl_test.cc +++ b/fastpair/scanning/scanner_broker_impl_test.cc @@ -75,7 +75,7 @@ TEST_F(ScannerBrokerImplTest, CanStartScanning) { std::string decoded_key; absl::Base64Unescape(kPublicAntiSpoof, &decoded_key); SingleThreadExecutor executor; - FastPairDeviceRepository devices; + FastPairDeviceRepository devices(&executor); proto::Device metadata; auto repository_ = std::make_unique(); metadata.mutable_anti_spoofing_key_pair()->set_public_key(decoded_key);