From d1d15d266dcfa971b9387ccb6bbf62d6f5ff2620 Mon Sep 17 00:00:00 2001 From: Francis Tsui Date: Fri, 17 Jul 2026 13:19:31 -0700 Subject: [PATCH] Fix UAF in WinRT event callback. PiperOrigin-RevId: 949729415 --- internal/platform/bluetooth_classic.h | 2 +- .../implementation/bluetooth_classic.h | 9 +- .../implementation/g3/bluetooth_classic.cc | 4 +- .../implementation/g3/bluetooth_classic.h | 2 +- .../windows/bluetooth_classic_medium.cc | 98 ++++++++----------- .../windows/bluetooth_classic_medium.h | 16 +-- .../bluetooth_classic_server_socket.cc | 10 +- .../windows/bluetooth_classic_server_socket.h | 25 +++-- 8 files changed, 78 insertions(+), 88 deletions(-) diff --git a/internal/platform/bluetooth_classic.h b/internal/platform/bluetooth_classic.h index a23b047a..35e711e9 100644 --- a/internal/platform/bluetooth_classic.h +++ b/internal/platform/bluetooth_classic.h @@ -157,7 +157,7 @@ class BluetoothServerSocket final { BluetoothServerSocket& operator=(const BluetoothServerSocket&) = default; ~BluetoothServerSocket() = default; explicit BluetoothServerSocket( - std::unique_ptr socket) + std::shared_ptr socket) : impl_(std::move(socket)) {} // https://developer.android.com/reference/android/bluetooth/BluetoothServerSocket.html#accept() diff --git a/internal/platform/implementation/bluetooth_classic.h b/internal/platform/implementation/bluetooth_classic.h index d5c38095..5e00c1dc 100644 --- a/internal/platform/implementation/bluetooth_classic.h +++ b/internal/platform/implementation/bluetooth_classic.h @@ -19,7 +19,6 @@ #include #include -#include "absl/base/attributes.h" #include "absl/functional/any_invocable.h" #include "absl/strings/string_view.h" #include "internal/platform/cancellation_flag.h" @@ -29,8 +28,7 @@ #include "internal/platform/mac_address.h" #include "internal/platform/output_stream.h" -namespace nearby { -namespace api { +namespace nearby::api { // https://developer.android.com/reference/android/bluetooth/BluetoothDevice.html. class BluetoothDevice { @@ -273,7 +271,7 @@ class BluetoothClassicMedium { // UUID. // // Returns nullptr error. - virtual std::unique_ptr ListenForService( + virtual std::shared_ptr ListenForService( const std::string& service_name, const std::string& service_uuid) = 0; // https://developer.android.com/reference/android/bluetooth/BluetoothDevice.html#createBond() @@ -290,7 +288,6 @@ class BluetoothClassicMedium { virtual void RemoveObserver(Observer* observer) = 0; }; -} // namespace api -} // namespace nearby +} // namespace nearby::api #endif // PLATFORM_API_BLUETOOTH_CLASSIC_H_ diff --git a/internal/platform/implementation/g3/bluetooth_classic.cc b/internal/platform/implementation/g3/bluetooth_classic.cc index ec780d33..7e100034 100644 --- a/internal/platform/implementation/g3/bluetooth_classic.cc +++ b/internal/platform/implementation/g3/bluetooth_classic.cc @@ -240,10 +240,10 @@ std::unique_ptr BluetoothClassicMedium::ConnectToService( return socket; } -std::unique_ptr +std::shared_ptr BluetoothClassicMedium::ListenForService(const std::string& service_name, const std::string& service_uuid) { - auto socket = std::make_unique(GetAdapter()); + auto socket = std::make_shared(GetAdapter()); socket->SetCloseNotifier([this, uuid = service_uuid]() { absl::MutexLock lock(mutex_); sockets_.erase(uuid); diff --git a/internal/platform/implementation/g3/bluetooth_classic.h b/internal/platform/implementation/g3/bluetooth_classic.h index 0826430d..aeb4f09f 100644 --- a/internal/platform/implementation/g3/bluetooth_classic.h +++ b/internal/platform/implementation/g3/bluetooth_classic.h @@ -195,7 +195,7 @@ class BluetoothClassicMedium : public api::BluetoothClassicMedium { // UUID. // // Returns nullptr on error. - std::unique_ptr ListenForService( + std::shared_ptr ListenForService( const std::string& service_name, const std::string& service_uuid) override ABSL_LOCKS_EXCLUDED(mutex_); diff --git a/internal/platform/implementation/windows/bluetooth_classic_medium.cc b/internal/platform/implementation/windows/bluetooth_classic_medium.cc index 87a80ffd..94994e88 100644 --- a/internal/platform/implementation/windows/bluetooth_classic_medium.cc +++ b/internal/platform/implementation/windows/bluetooth_classic_medium.cc @@ -32,6 +32,7 @@ #include "internal/platform/implementation/bluetooth_classic.h" #include "internal/platform/implementation/windows/bluetooth_adapter.h" #include "internal/platform/implementation/windows/bluetooth_classic_device.h" +#include "internal/platform/implementation/windows/bluetooth_classic_server_socket.h" #include "internal/platform/implementation/windows/bluetooth_classic_socket.h" #include "internal/platform/implementation/windows/bluetooth_pairing.h" #include "internal/platform/implementation/windows/generated/winrt/Windows.Devices.Bluetooth.Rfcomm.h" @@ -43,8 +44,7 @@ #include "internal/platform/logging.h" #include "internal/platform/mac_address.h" -namespace nearby { -namespace windows { +namespace nearby::windows { namespace { using ::winrt::Windows::Devices::Bluetooth::Rfcomm::RfcommDeviceService; using ::winrt::Windows::Devices::Bluetooth::Rfcomm::RfcommServiceId; @@ -124,7 +124,13 @@ BluetoothClassicMedium::BluetoothClassicMedium( &BluetoothClassicMedium::OnScanModeChanged, this, std::placeholders::_1)); } -BluetoothClassicMedium::~BluetoothClassicMedium() {} +BluetoothClassicMedium::~BluetoothClassicMedium() { + // Clear the close notifier to prevent UAF if the server_socket_ outlives + // the BluetoothClassicMedium. + if (raw_server_socket_ != nullptr) { + raw_server_socket_->SetCloseNotifier(nullptr); + } +} bool BluetoothClassicMedium::StartDiscovery( BluetoothClassicMedium::DiscoveryCallback discovery_callback) { @@ -259,7 +265,7 @@ std::unique_ptr BluetoothClassicMedium::ConnectToService( // UUID. // // Returns nullptr error. -std::unique_ptr +std::shared_ptr BluetoothClassicMedium::ListenForService(const std::string& service_name, const std::string& service_uuid) { VLOG(1) << "ListenForService is called with service name: " << service_name @@ -283,14 +289,24 @@ BluetoothClassicMedium::ListenForService(const std::string& service_name, bool radio_discoverable = scan_mode_ == BluetoothAdapter::ScanMode::kConnectableDiscoverable; - bool result = StartAdvertising(radio_discoverable); + if (rfcomm_provider_ != nullptr && + is_radio_discoverable_ == radio_discoverable) { + LOG(WARNING) << __func__ + << ": Ignore StartAdvertising due to no change to " + "current advertising."; + return server_socket_; + } - if (!result) { + auto server_socket = StartAdvertising(radio_discoverable); + + if (!server_socket) { LOG(ERROR) << __func__ << ": Failed to start listening."; return nullptr; } - return std::move(server_socket_); + raw_server_socket_ = server_socket.get(); + server_socket_ = std::move(server_socket); + return server_socket_; } api::BluetoothDevice* BluetoothClassicMedium::GetRemoteDevice( @@ -833,25 +849,19 @@ bool BluetoothClassicMedium::IsWatcherRunning() { (status == DeviceWatcherStatus::Stopping); } -bool BluetoothClassicMedium::StartAdvertising(bool radio_discoverable) { +std::shared_ptr +BluetoothClassicMedium::StartAdvertising(bool radio_discoverable) { LOG(INFO) << __func__ << ": StartAdvertising is called with radio_discoverable: " << radio_discoverable << "."; + std::shared_ptr server_socket; try { - if (rfcomm_provider_ != nullptr && - is_radio_discoverable_ == radio_discoverable) { - LOG(WARNING) << __func__ - << ": Ignore StartAdvertising due to no change to " - "current advertising."; - return true; - } - if (rfcomm_provider_ != nullptr && !StopAdvertising()) { LOG(WARNING) << __func__ << ": Failed to StartAdvertising due to cannot stop " "running advertising."; - return false; + return nullptr; } rfcomm_provider_ = @@ -859,79 +869,57 @@ bool BluetoothClassicMedium::StartAdvertising(bool radio_discoverable) { RfcommServiceId::FromUuid(winrt::guid(service_uuid_))) .get(); - server_socket_ = std::make_unique( + server_socket = BluetoothServerSocket::Create( winrt::to_string(rfcomm_provider_.ServiceId().AsString())); - raw_server_socket_ = server_socket_.get(); - - if (!server_socket_->listen()) { + if (!server_socket->listen()) { LOG(ERROR) << __func__ << ": Failed to StartAdvertising due to cannot start socket."; - server_socket_->Close(); - server_socket_ = nullptr; rfcomm_provider_ = nullptr; - return false; + return nullptr; } - server_socket_->SetCloseNotifier([&]() { StopAdvertising(); }); + server_socket->SetCloseNotifier([&]() { StopAdvertising(); }); // Set the SDP attributes and start Bluetooth advertising InitializeServiceSdpAttributes(rfcomm_provider_, service_name_); // Start to advertising. - rfcomm_provider_.StartAdvertising(server_socket_->stream_socket_listener(), + rfcomm_provider_.StartAdvertising(server_socket->stream_socket_listener(), radio_discoverable); is_radio_discoverable_ = radio_discoverable; LOG(INFO) << ": StartListening completed successfully."; - return true; + return server_socket; } catch (std::exception exception) { // We will log and eat the exception since the caller // expects nullptr if it fails LOG(ERROR) << __func__ << ": Exception setting up for listen: " << exception.what(); - if (server_socket_ != nullptr) { - server_socket_->Close(); - server_socket_ = nullptr; - } - if (rfcomm_provider_ != nullptr) { rfcomm_provider_ = nullptr; } - - return false; + return nullptr; } catch (const winrt::hresult_error& ex) { LOG(ERROR) << __func__ << ": Exception setting up for listen: " << ex.code() << ": " << winrt::to_string(ex.message()); - if (server_socket_ != nullptr) { - server_socket_->Close(); - server_socket_ = nullptr; - } - if (rfcomm_provider_ != nullptr) { rfcomm_provider_ = nullptr; } - - return false; + return nullptr; } catch (...) { LOG(ERROR) << __func__ << ": Unknown exception."; - if (server_socket_ != nullptr) { - server_socket_->Close(); - server_socket_ = nullptr; - } - if (rfcomm_provider_ != nullptr) { rfcomm_provider_ = nullptr; } - - return false; + return nullptr; } } bool BluetoothClassicMedium::StopAdvertising() { VLOG(1) << __func__ << ": StopAdvertising is called"; - + bool result = false; try { if (rfcomm_provider_ == nullptr) { LOG(ERROR) << __func__ @@ -940,12 +928,9 @@ bool BluetoothClassicMedium::StopAdvertising() { } rfcomm_provider_.StopAdvertising(); - rfcomm_provider_ = nullptr; - raw_server_socket_ = nullptr; - server_socket_ = nullptr; LOG(INFO) << ": StopAdvertising completed successfully."; - return true; + result = true; } catch (std::exception exception) { LOG(ERROR) << __func__ << ": StopAdvertising exception: " << exception.what(); @@ -957,9 +942,9 @@ bool BluetoothClassicMedium::StopAdvertising() { } rfcomm_provider_ = nullptr; - raw_server_socket_ = nullptr; server_socket_ = nullptr; - return false; + raw_server_socket_ = nullptr; + return result; } bool BluetoothClassicMedium::InitializeServiceSdpAttributes( @@ -988,5 +973,4 @@ bool BluetoothClassicMedium::InitializeServiceSdpAttributes( } } -} // namespace windows -} // namespace nearby +} // namespace nearby::windows diff --git a/internal/platform/implementation/windows/bluetooth_classic_medium.h b/internal/platform/implementation/windows/bluetooth_classic_medium.h index 5027e4eb..14b44c3f 100644 --- a/internal/platform/implementation/windows/bluetooth_classic_medium.h +++ b/internal/platform/implementation/windows/bluetooth_classic_medium.h @@ -34,8 +34,7 @@ #include "internal/platform/implementation/windows/generated/winrt/base.h" #include "internal/platform/mac_address.h" -namespace nearby { -namespace windows { +namespace nearby::windows { // Container of operations that can be performed over the Bluetooth Classic // medium. @@ -80,7 +79,7 @@ class BluetoothClassicMedium : public api::BluetoothClassicMedium { // UUID. // // Returns nullptr error. - std::unique_ptr ListenForService( + std::shared_ptr ListenForService( const std::string& service_name, const std::string& service_uuid) override; @@ -103,7 +102,8 @@ class BluetoothClassicMedium : public api::BluetoothClassicMedium { private: bool StartScanning(); bool StopScanning(); - bool StartAdvertising(bool radio_discoverable); + std::shared_ptr StartAdvertising( + bool radio_discoverable); bool StopAdvertising(); bool InitializeServiceSdpAttributes( ::winrt::Windows::Devices::Bluetooth::Rfcomm::RfcommServiceProvider @@ -185,13 +185,15 @@ class BluetoothClassicMedium : public api::BluetoothClassicMedium { // Used for advertising. ::winrt::Windows::Devices::Bluetooth::Rfcomm::RfcommServiceProvider rfcomm_provider_ = nullptr; - std::unique_ptr server_socket_ = nullptr; + std::shared_ptr server_socket_; + // Raw pointer to the BluetoothServerSocket impl class that is held by the + // shared_ptr server_socket_. The lifetime of this pointer is guaranteed by + // the shared_ptr. BluetoothServerSocket* raw_server_socket_ = nullptr; bool is_radio_discoverable_ = false; ObserverList observers_; }; -} // namespace windows -} // namespace nearby +} // namespace nearby::windows #endif // PLATFORM_IMPL_WINDOWS_BLUETOOTH_CLASSIC_MEDIUM_H_ diff --git a/internal/platform/implementation/windows/bluetooth_classic_server_socket.cc b/internal/platform/implementation/windows/bluetooth_classic_server_socket.cc index 63b5e1a6..f8a9cb40 100644 --- a/internal/platform/implementation/windows/bluetooth_classic_server_socket.cc +++ b/internal/platform/implementation/windows/bluetooth_classic_server_socket.cc @@ -26,10 +26,10 @@ #include "internal/platform/exception.h" #include "internal/platform/implementation/bluetooth_classic.h" #include "internal/platform/implementation/windows/bluetooth_classic_socket.h" +#include "internal/platform/implementation/windows/generated/winrt/Windows.Networking.Sockets.h" #include "internal/platform/logging.h" -namespace nearby { -namespace windows { +namespace nearby::windows { namespace { using ::winrt::Windows::Networking::Sockets::SocketProtectionLevel; using ::winrt::Windows::Networking::Sockets::SocketQualityOfService; @@ -132,7 +132,8 @@ bool BluetoothServerSocket::listen() { // Setup socket event of ConnectionReceived. listener_event_token_ = stream_socket_listener_.ConnectionReceived( - {this, &BluetoothServerSocket::Listener_ConnectionReceived}); + {shared_from_this(), + &BluetoothServerSocket::Listener_ConnectionReceived}); stream_socket_listener_ .BindServiceNameAsync(winrt::to_hstring(service_name_), @@ -167,5 +168,4 @@ bool BluetoothServerSocket::listen() { return ::winrt::fire_and_forget{}; } -} // namespace windows -} // namespace nearby +} // namespace nearby::windows diff --git a/internal/platform/implementation/windows/bluetooth_classic_server_socket.h b/internal/platform/implementation/windows/bluetooth_classic_server_socket.h index 2e3f2baf..4f1779cb 100644 --- a/internal/platform/implementation/windows/bluetooth_classic_server_socket.h +++ b/internal/platform/implementation/windows/bluetooth_classic_server_socket.h @@ -15,25 +15,30 @@ #ifndef PLATFORM_IMPL_WINDOWS_BLUETOOTH_CLASSIC_SERVER_SOCKET_H_ #define PLATFORM_IMPL_WINDOWS_BLUETOOTH_CLASSIC_SERVER_SOCKET_H_ -#include +#include #include -#include #include +#include "absl/functional/any_invocable.h" #include "absl/strings/string_view.h" #include "absl/synchronization/mutex.h" #include "internal/platform/exception.h" #include "internal/platform/implementation/bluetooth_classic.h" -#include "internal/platform/implementation/windows/bluetooth_classic_socket.h" +#include "internal/platform/implementation/windows/generated/winrt/Windows.Networking.Sockets.h" #include "internal/platform/implementation/windows/generated/winrt/base.h" -namespace nearby { -namespace windows { +namespace nearby::windows { -class BluetoothServerSocket : public api::BluetoothServerSocket { +class BluetoothServerSocket + : public api::BluetoothServerSocket, + public std::enable_shared_from_this { public: - explicit BluetoothServerSocket(absl::string_view service_name); + static std::shared_ptr Create( + absl::string_view service_name) { + return std::shared_ptr( + new BluetoothServerSocket(service_name)); + } ~BluetoothServerSocket() override; @@ -65,6 +70,9 @@ class BluetoothServerSocket : public api::BluetoothServerSocket { } private: + // BluetoothServerSocket must be created as a shared_ptr. + explicit BluetoothServerSocket(absl::string_view service_name); + // The listener is accepting incoming connections ::winrt::fire_and_forget Listener_ConnectionReceived( ::winrt::Windows::Networking::Sockets::StreamSocketListener listener, @@ -93,7 +101,6 @@ class BluetoothServerSocket : public api::BluetoothServerSocket { bool closed_ = false; }; -} // namespace windows -} // namespace nearby +} // namespace nearby::windows #endif // PLATFORM_IMPL_WINDOWS_BLUETOOTH_CLASSIC_SERVER_SOCKET_H_