From fdd8183c533e6f71a4b1f3afbc1e4b9566721e05 Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Mon, 5 Feb 2024 21:12:23 +0100 Subject: [PATCH] [Linux] Do not reuse cancellable object (#31828) * [Linux] Do not reuse cancellable object Per documentation for g_cancellable_reset(): > Note that it is generally not a good idea to reuse an existing > cancellable for more operations after it has been cancelled once, as > this function might tempt you to do. The recommended practice is to > drop the reference to a cancellable after cancelling it, and let it > die with the outstanding async operations. * Update cancellable in ChipDeviceScanner --- src/platform/GLibTypeDeleter.h | 6 ++++ src/platform/Linux/BLEManagerImpl.cpp | 2 +- src/platform/Linux/bluez/BluezEndpoint.cpp | 33 +++++++++---------- src/platform/Linux/bluez/BluezEndpoint.h | 8 +++-- .../Linux/bluez/ChipDeviceScanner.cpp | 19 ++++++----- src/platform/Linux/bluez/ChipDeviceScanner.h | 3 +- 6 files changed, 39 insertions(+), 32 deletions(-) diff --git a/src/platform/GLibTypeDeleter.h b/src/platform/GLibTypeDeleter.h index 93ff40f61c111d..e6d9bfd0d2c1ed 100644 --- a/src/platform/GLibTypeDeleter.h +++ b/src/platform/GLibTypeDeleter.h @@ -108,6 +108,12 @@ struct GAutoPtrDeleter using deleter = GBytesDeleter; }; +template <> +struct GAutoPtrDeleter +{ + using deleter = GObjectDeleter; +}; + template <> struct GAutoPtrDeleter { diff --git a/src/platform/Linux/BLEManagerImpl.cpp b/src/platform/Linux/BLEManagerImpl.cpp index 241c13e36670ef..12232bc0fd36e4 100644 --- a/src/platform/Linux/BLEManagerImpl.cpp +++ b/src/platform/Linux/BLEManagerImpl.cpp @@ -571,7 +571,7 @@ void BLEManagerImpl::DriveBLEState() // Initializes the Bluez BLE layer if needed. if (mServiceMode == ConnectivityManager::kCHIPoBLEServiceMode_Enabled && !mFlags.Has(Flags::kBluezBLELayerInitialized)) { - SuccessOrExit(err = mEndpoint.Init(mAdapterId, mIsCentral, nullptr)); + SuccessOrExit(err = mEndpoint.Init(mIsCentral, mAdapterId)); mFlags.Set(Flags::kBluezBLELayerInitialized); } diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index 24614eba63d24f..51a418bfee384b 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -656,22 +656,14 @@ CHIP_ERROR BluezEndpoint::RegisterGattApplication() return err; } -CHIP_ERROR BluezEndpoint::Init(uint32_t aAdapterId, bool aIsCentral, const char * apBleAddr) +CHIP_ERROR BluezEndpoint::Init(bool aIsCentral, uint32_t aAdapterId) { - CHIP_ERROR err; + VerifyOrReturnError(!mIsInitialized, CHIP_ERROR_INCORRECT_STATE); mAdapterId = aAdapterId; mIsCentral = aIsCentral; - if (apBleAddr != nullptr) - mpAdapterAddr = g_strdup(apBleAddr); - - if (aIsCentral) - { - mpConnectCancellable = g_cancellable_new(); - } - - err = PlatformMgrImpl().GLibMatterContextInvokeSync( + CHIP_ERROR err = PlatformMgrImpl().GLibMatterContextInvokeSync( +[](BluezEndpoint * self) { return self->StartupEndpointBindings(); }, this); VerifyOrReturnError(err == CHIP_NO_ERROR, err, ChipLogError(DeviceLayer, "Failed to schedule endpoint initialization")); @@ -681,6 +673,13 @@ CHIP_ERROR BluezEndpoint::Init(uint32_t aAdapterId, bool aIsCentral, const char return CHIP_NO_ERROR; } +CHIP_ERROR BluezEndpoint::Init(bool aIsCentral, const char * apBleAddr) +{ + VerifyOrReturnError(!mIsInitialized, CHIP_ERROR_INCORRECT_STATE); + mpAdapterAddr = g_strdup(apBleAddr); + return Init(aIsCentral, mAdapterId); +} + void BluezEndpoint::Shutdown() { VerifyOrReturn(mIsInitialized); @@ -707,8 +706,6 @@ void BluezEndpoint::Shutdown() g_object_unref(self->mpC2); if (self->mpC3 != nullptr) g_object_unref(self->mpC3); - if (self->mpConnectCancellable != nullptr) - g_object_unref(self->mpConnectCancellable); return CHIP_NO_ERROR; }, this); @@ -744,7 +741,7 @@ void BluezEndpoint::ConnectDeviceDone(GObject * aObject, GAsyncResult * aResult, g_clear_error(&MakeUniquePointerReceiver(error).Get()); bluez_device1_call_disconnect_sync(device, nullptr, &MakeUniquePointerReceiver(error).Get()); - bluez_device1_call_connect(device, params->mEndpoint.mpConnectCancellable, ConnectDeviceDone, params); + bluez_device1_call_connect(device, params->mEndpoint.mConnectCancellable.get(), ConnectDeviceDone, params); return; } @@ -760,8 +757,7 @@ void BluezEndpoint::ConnectDeviceDone(GObject * aObject, GAsyncResult * aResult, CHIP_ERROR BluezEndpoint::ConnectDeviceImpl(ConnectParams * apParams) { - g_cancellable_reset(apParams->mEndpoint.mpConnectCancellable); - bluez_device1_call_connect(apParams->mpDevice, apParams->mEndpoint.mpConnectCancellable, ConnectDeviceDone, apParams); + bluez_device1_call_connect(apParams->mpDevice, apParams->mEndpoint.mConnectCancellable.get(), ConnectDeviceDone, apParams); return CHIP_NO_ERROR; } @@ -770,6 +766,7 @@ CHIP_ERROR BluezEndpoint::ConnectDevice(BluezDevice1 & aDevice) auto params = chip::Platform::New(*this, &aDevice); VerifyOrReturnError(params != nullptr, CHIP_ERROR_NO_MEMORY); + mConnectCancellable.reset(g_cancellable_new()); if (PlatformMgrImpl().GLibMatterContextInvokeSync(ConnectDeviceImpl, params) != CHIP_NO_ERROR) { ChipLogError(Ble, "Failed to schedule ConnectDeviceImpl() on CHIPoBluez thread"); @@ -782,8 +779,8 @@ CHIP_ERROR BluezEndpoint::ConnectDevice(BluezDevice1 & aDevice) void BluezEndpoint::CancelConnect() { - VerifyOrDie(mpConnectCancellable != nullptr); - g_cancellable_cancel(mpConnectCancellable); + g_cancellable_cancel(mConnectCancellable.get()); + mConnectCancellable.reset(); } } // namespace Internal diff --git a/src/platform/Linux/bluez/BluezEndpoint.h b/src/platform/Linux/bluez/BluezEndpoint.h index 397b5a1597168b..772b503efd300a 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.h +++ b/src/platform/Linux/bluez/BluezEndpoint.h @@ -54,6 +54,7 @@ #include #include +#include #include #include "BluezConnection.h" @@ -69,7 +70,8 @@ class BluezEndpoint BluezEndpoint() = default; ~BluezEndpoint() = default; - CHIP_ERROR Init(uint32_t aAdapterId, bool aIsCentral, const char * apBleAddr); + CHIP_ERROR Init(bool aIsCentral, uint32_t aAdapterId); + CHIP_ERROR Init(bool aIsCentral, const char * apBleAddr); void Shutdown(); BluezAdapter1 * GetAdapter() const { return mpAdapter; } @@ -148,8 +150,8 @@ class BluezEndpoint BluezGattCharacteristic1 * mpC3 = nullptr; std::unordered_map mConnMap; - GCancellable * mpConnectCancellable = nullptr; - char * mpPeerDevicePath = nullptr; + GAutoPtr mConnectCancellable; + char * mpPeerDevicePath = nullptr; // Allow BluezConnection to access our private members friend class BluezConnection; diff --git a/src/platform/Linux/bluez/ChipDeviceScanner.cpp b/src/platform/Linux/bluez/ChipDeviceScanner.cpp index 3a3115a9ae4783..5d9cbf25c9028b 100644 --- a/src/platform/Linux/bluez/ChipDeviceScanner.cpp +++ b/src/platform/Linux/bluez/ChipDeviceScanner.cpp @@ -58,9 +58,8 @@ CHIP_ERROR ChipDeviceScanner::Init(BluezAdapter1 * adapter, ChipDeviceScannerDel // Make this function idempotent by shutting down previously initialized state if any. Shutdown(); - mAdapter = BLUEZ_ADAPTER1(g_object_ref(adapter)); - mCancellable = g_cancellable_new(); - mDelegate = delegate; + mAdapter = BLUEZ_ADAPTER1(g_object_ref(adapter)); + mDelegate = delegate; // Create the D-Bus object manager client object on the glib thread, so that all D-Bus signals // will be delivered to the glib thread. @@ -73,7 +72,7 @@ CHIP_ERROR ChipDeviceScanner::Init(BluezAdapter1 * adapter, ChipDeviceScannerDel self->mManager = g_dbus_object_manager_client_new_for_bus_sync( G_BUS_TYPE_SYSTEM, G_DBUS_OBJECT_MANAGER_CLIENT_FLAGS_NONE, BLUEZ_INTERFACE, "/", bluez_object_manager_client_get_proxy_type, nullptr /* unused user data in the Proxy Type Func */, - nullptr /* destroy notify */, self->mCancellable, &MakeUniquePointerReceiver(err).Get()); + nullptr /* destroy notify */, nullptr /* cancellable */, &MakeUniquePointerReceiver(err).Get()); VerifyOrReturnError(self->mManager != nullptr, CHIP_ERROR_INTERNAL, ChipLogError(Ble, "Failed to get D-Bus object manager for device scanning: %s", err->message)); return CHIP_NO_ERROR; @@ -100,8 +99,6 @@ void ChipDeviceScanner::Shutdown() g_object_unref(self->mManager); if (self->mAdapter != nullptr) g_object_unref(self->mAdapter); - if (self->mCancellable != nullptr) - g_object_unref(self->mCancellable); return CHIP_NO_ERROR; }, this); @@ -115,6 +112,7 @@ CHIP_ERROR ChipDeviceScanner::StartScan(System::Clock::Timeout timeout) VerifyOrReturnError(mScannerState != ChipDeviceScannerState::SCANNER_SCANNING, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(mTimerState == ScannerTimerState::TIMER_CANCELED, CHIP_ERROR_INCORRECT_STATE); + mCancellable.reset(g_cancellable_new()); if (PlatformMgrImpl().GLibMatterContextInvokeSync(MainLoopStartScan, this) != CHIP_NO_ERROR) { ChipLogError(Ble, "Failed to schedule BLE scan start."); @@ -191,7 +189,9 @@ CHIP_ERROR ChipDeviceScanner::MainLoopStopScan(ChipDeviceScanner * self) { GAutoPtr error; - g_cancellable_cancel(self->mCancellable); // in case we are currently running a scan + // In case we are currently running a scan + g_cancellable_cancel(self->mCancellable.get()); + self->mCancellable.reset(); if (self->mObjectAddedSignal) { @@ -303,7 +303,7 @@ CHIP_ERROR ChipDeviceScanner::MainLoopStartScan(ChipDeviceScanner * self) g_variant_builder_add(&filterBuilder, "{sv}", "Transport", g_variant_new_string("le")); GVariant * filter = g_variant_builder_end(&filterBuilder); - if (!bluez_adapter1_call_set_discovery_filter_sync(self->mAdapter, filter, self->mCancellable, + if (!bluez_adapter1_call_set_discovery_filter_sync(self->mAdapter, filter, self->mCancellable.get(), &MakeUniquePointerReceiver(error).Get())) { // Not critical: ignore if fails @@ -312,7 +312,8 @@ CHIP_ERROR ChipDeviceScanner::MainLoopStartScan(ChipDeviceScanner * self) } ChipLogProgress(Ble, "BLE initiating scan."); - if (!bluez_adapter1_call_start_discovery_sync(self->mAdapter, self->mCancellable, &MakeUniquePointerReceiver(error).Get())) + if (!bluez_adapter1_call_start_discovery_sync(self->mAdapter, self->mCancellable.get(), + &MakeUniquePointerReceiver(error).Get())) { ChipLogError(Ble, "Failed to start discovery: %s", error->message); return CHIP_ERROR_INTERNAL; diff --git a/src/platform/Linux/bluez/ChipDeviceScanner.h b/src/platform/Linux/bluez/ChipDeviceScanner.h index 4785a737869dda..1271ec39cb241a 100644 --- a/src/platform/Linux/bluez/ChipDeviceScanner.h +++ b/src/platform/Linux/bluez/ChipDeviceScanner.h @@ -23,6 +23,7 @@ #include #include +#include #include #include @@ -106,13 +107,13 @@ class ChipDeviceScanner GDBusObjectManager * mManager = nullptr; BluezAdapter1 * mAdapter = nullptr; - GCancellable * mCancellable = nullptr; ChipDeviceScannerDelegate * mDelegate = nullptr; gulong mObjectAddedSignal = 0; gulong mInterfaceChangedSignal = 0; ChipDeviceScannerState mScannerState = ChipDeviceScannerState::SCANNER_UNINITIALIZED; /// Used to track if timer has already expired and doesn't need to be canceled. ScannerTimerState mTimerState = ScannerTimerState::TIMER_CANCELED; + GAutoPtr mCancellable; }; } // namespace Internal