[Linux] Fix memory leaks when handling BluezAdapter1 objects (#32263)

* Convert glib object to GAutoPtr<> in Bluez * Do not keep a copy of BluezAdapter1 properties * Use GAutoPtr to manage BluezAdapter1 life cycle * Simplify adapter retrieval from BlueZ object * Properly release objects obtained with bluez_object_get_*() * Use reinterpret_cast when using g_object_ref() * Use on-the-stack strings to simplify memory management * Manage BluezLEAdvertisement1 with GAutoPtr * Do not leak GDBusObjectManagerServer in case of re-initialization * Fix life cycle of exported advertisement D-Bus object * Restart BLE advertisement in case of premature release --------- Co-authored-by: Damian Michalak-Szmaciński <[email protected]>
project-chip · Feb 23, 2024 · 003c478 · 003c478
1 parent 233c177
commit 003c478
Show file tree

Hide file tree

Showing 14 changed files with 194 additions and 210 deletions.
diff --git a/src/platform/GLibTypeDeleter.h b/src/platform/GLibTypeDeleter.h
@@ -120,6 +120,12 @@ struct GAutoPtrDeleter<GDBusConnection>
     using deleter = GObjectDeleter;
 };
 
+template <>
+struct GAutoPtrDeleter<GDBusObjectManagerServer>
+{
+    using deleter = GObjectDeleter;
+};
+
 template <>
 struct GAutoPtrDeleter<GError>
 {

diff --git a/src/platform/Linux/BLEManagerImpl.cpp b/src/platform/Linux/BLEManagerImpl.cpp
@@ -289,8 +289,13 @@ void BLEManagerImpl::HandlePlatformSpecificBLEEvent(const ChipDeviceEvent * apEv
     case DeviceEventType::kPlatformLinuxBLEPeripheralAdvStartComplete:
         VerifyOrExit(apEvent->Platform.BLEPeripheralAdvStartComplete.mIsSuccess, err = CHIP_ERROR_INCORRECT_STATE);
         sInstance.mFlags.Clear(Flags::kControlOpInProgress).Clear(Flags::kAdvertisingRefreshNeeded);
-        // Start a timer to make sure that the fast advertising is stopped after specified timeout.
-        SuccessOrExit(err = DeviceLayer::SystemLayer().StartTimer(kFastAdvertiseTimeout, HandleAdvertisingTimer, this));
+        // Do not restart the timer if it is still active. This is to avoid the timer from being restarted
+        // if the advertising is stopped due to a premature release.
+        if (!DeviceLayer::SystemLayer().IsTimerActive(HandleAdvertisingTimer, this))
+        {
+            // Start a timer to make sure that the fast advertising is stopped after specified timeout.
+            SuccessOrExit(err = DeviceLayer::SystemLayer().StartTimer(kFastAdvertiseTimeout, HandleAdvertisingTimer, this));
+        }
         sInstance.mFlags.Set(Flags::kAdvertising);
         break;
     case DeviceEventType::kPlatformLinuxBLEPeripheralAdvStopComplete:
@@ -305,6 +310,11 @@ void BLEManagerImpl::HandlePlatformSpecificBLEEvent(const ChipDeviceEvent * apEv
             ChipLogProgress(DeviceLayer, "CHIPoBLE advertising stopped");
         }
         break;
+    case DeviceEventType::kPlatformLinuxBLEPeripheralAdvReleased:
+        // If the advertising was stopped due to a premature release, check if it needs to be restarted.
+        sInstance.mFlags.Clear(Flags::kAdvertising);
+        DriveBLEState();
+        break;
     case DeviceEventType::kPlatformLinuxBLEPeripheralRegisterAppComplete:
         VerifyOrExit(apEvent->Platform.BLEPeripheralRegisterAppComplete.mIsSuccess, err = CHIP_ERROR_INCORRECT_STATE);
         mFlags.Set(Flags::kAppRegistered);
@@ -780,30 +790,34 @@ CHIP_ERROR BLEManagerImpl::CancelConnection()
     return CHIP_NO_ERROR;
 }
 
-void BLEManagerImpl::NotifyBLEPeripheralRegisterAppComplete(bool aIsSuccess, void * apAppstate)
+void BLEManagerImpl::NotifyBLEPeripheralRegisterAppComplete(bool aIsSuccess)
 {
     ChipDeviceEvent event;
     event.Type                                                 = DeviceEventType::kPlatformLinuxBLEPeripheralRegisterAppComplete;
     event.Platform.BLEPeripheralRegisterAppComplete.mIsSuccess = aIsSuccess;
-    event.Platform.BLEPeripheralRegisterAppComplete.mpAppstate = apAppstate;
     PlatformMgr().PostEventOrDie(&event);
 }
 
-void BLEManagerImpl::NotifyBLEPeripheralAdvStartComplete(bool aIsSuccess, void * apAppstate)
+void BLEManagerImpl::NotifyBLEPeripheralAdvStartComplete(bool aIsSuccess)
 {
     ChipDeviceEvent event;
     event.Type                                              = DeviceEventType::kPlatformLinuxBLEPeripheralAdvStartComplete;
     event.Platform.BLEPeripheralAdvStartComplete.mIsSuccess = aIsSuccess;
-    event.Platform.BLEPeripheralAdvStartComplete.mpAppstate = apAppstate;
     PlatformMgr().PostEventOrDie(&event);
 }
 
-void BLEManagerImpl::NotifyBLEPeripheralAdvStopComplete(bool aIsSuccess, void * apAppstate)
+void BLEManagerImpl::NotifyBLEPeripheralAdvStopComplete(bool aIsSuccess)
 {
     ChipDeviceEvent event;
     event.Type                                             = DeviceEventType::kPlatformLinuxBLEPeripheralAdvStopComplete;
     event.Platform.BLEPeripheralAdvStopComplete.mIsSuccess = aIsSuccess;
-    event.Platform.BLEPeripheralAdvStopComplete.mpAppstate = apAppstate;
+    PlatformMgr().PostEventOrDie(&event);
+}
+
+void BLEManagerImpl::NotifyBLEPeripheralAdvReleased()
+{
+    ChipDeviceEvent event;
+    event.Type = DeviceEventType::kPlatformLinuxBLEPeripheralAdvReleased;
     PlatformMgr().PostEventOrDie(&event);
 }
 

diff --git a/src/platform/Linux/BLEManagerImpl.h b/src/platform/Linux/BLEManagerImpl.h
@@ -92,9 +92,10 @@ class BLEManagerImpl final : public BLEManager,
     static void HandleTXCharCCCDWrite(BLE_CONNECTION_OBJECT user_data);
     static void HandleTXComplete(BLE_CONNECTION_OBJECT user_data);
 
-    static void NotifyBLEPeripheralRegisterAppComplete(bool aIsSuccess, void * apAppstate);
-    static void NotifyBLEPeripheralAdvStartComplete(bool aIsSuccess, void * apAppstate);
-    static void NotifyBLEPeripheralAdvStopComplete(bool aIsSuccess, void * apAppstate);
+    static void NotifyBLEPeripheralRegisterAppComplete(bool aIsSuccess);
+    static void NotifyBLEPeripheralAdvStartComplete(bool aIsSuccess);
+    static void NotifyBLEPeripheralAdvStopComplete(bool aIsSuccess);
+    static void NotifyBLEPeripheralAdvReleased();
 
 private:
     // ===== Members that implement the BLEManager internal interface.

diff --git a/src/platform/Linux/CHIPDevicePlatformEvent.h b/src/platform/Linux/CHIPDevicePlatformEvent.h
@@ -54,7 +54,8 @@ enum InternalPlatformSpecificEventTypes
     kPlatformLinuxBLEOutOfBuffersEvent,
     kPlatformLinuxBLEPeripheralRegisterAppComplete,
     kPlatformLinuxBLEPeripheralAdvStartComplete,
-    kPlatformLinuxBLEPeripheralAdvStopComplete
+    kPlatformLinuxBLEPeripheralAdvStopComplete,
+    kPlatformLinuxBLEPeripheralAdvReleased,
 };
 
 } // namespace DeviceEventType
@@ -91,22 +92,14 @@ struct ChipDevicePlatformEvent
         struct
         {
             bool mIsSuccess;
-            void * mpAppstate;
         } BLEPeripheralRegisterAppComplete;
         struct
         {
             bool mIsSuccess;
-            void * mpAppstate;
-        } BLEPeripheralAdvConfiguredComplete;
-        struct
-        {
-            bool mIsSuccess;
-            void * mpAppstate;
         } BLEPeripheralAdvStartComplete;
         struct
         {
             bool mIsSuccess;
-            void * mpAppstate;
         } BLEPeripheralAdvStopComplete;
     };
 };

diff --git a/src/platform/Linux/bluez/AdapterIterator.cpp b/src/platform/Linux/bluez/AdapterIterator.cpp
@@ -37,12 +37,6 @@ AdapterIterator::~AdapterIterator()
     {
         g_list_free_full(mObjectList, g_object_unref);
     }
-
-    if (mCurrent.adapter != nullptr)
-    {
-        g_object_unref(mCurrent.adapter);
-        mCurrent.adapter = nullptr;
-    }
 }
 
 CHIP_ERROR AdapterIterator::Initialize(AdapterIterator * self)
@@ -90,30 +84,7 @@ bool AdapterIterator::Advance()
             continue;
         }
 
-        // PATH is of the for  BLUEZ_PATH / hci<nr>, i.e. like
-        // '/org/bluez/hci0'
-        // Index represents the number after hci
-        const char * path = g_dbus_proxy_get_object_path(G_DBUS_PROXY(adapter));
-        unsigned index    = 0;
-
-        if (sscanf(path, BLUEZ_PATH "/hci%u", &index) != 1)
-        {
-            ChipLogError(DeviceLayer, "Failed to extract HCI index from '%s'", StringOrNullMarker(path));
-            index = 0;
-        }
-
-        if (mCurrent.adapter != nullptr)
-        {
-            g_object_unref(mCurrent.adapter);
-            mCurrent.adapter = nullptr;
-        }
-
-        mCurrent.index   = index;
-        mCurrent.address = bluez_adapter1_get_address(adapter);
-        mCurrent.alias   = bluez_adapter1_get_alias(adapter);
-        mCurrent.name    = bluez_adapter1_get_name(adapter);
-        mCurrent.powered = bluez_adapter1_get_powered(adapter);
-        mCurrent.adapter = adapter;
+        mCurrentAdapter.reset(adapter);
 
         mCurrentListItem = mCurrentListItem->next;
 
@@ -123,6 +94,22 @@ bool AdapterIterator::Advance()
     return false;
 }
 
+uint32_t AdapterIterator::GetIndex() const
+{
+    // PATH is of the for  BLUEZ_PATH / hci<nr>, i.e. like '/org/bluez/hci0'
+    // Index represents the number after hci
+    const char * path = g_dbus_proxy_get_object_path(G_DBUS_PROXY(mCurrentAdapter.get()));
+    unsigned index    = 0;
+
+    if (sscanf(path, BLUEZ_PATH "/hci%u", &index) != 1)
+    {
+        ChipLogError(DeviceLayer, "Failed to extract HCI index from '%s'", StringOrNullMarker(path));
+        index = 0;
+    }
+
+    return index;
+}
+
 bool AdapterIterator::Next()
 {
     if (mManager == nullptr)

diff --git a/src/platform/Linux/bluez/AdapterIterator.h b/src/platform/Linux/bluez/AdapterIterator.h
@@ -56,12 +56,12 @@ class AdapterIterator
 
     // Information about the current value. Safe to call only after
     // "Next" has returned true.
-    uint32_t GetIndex() const { return mCurrent.index; }
-    const char * GetAddress() const { return mCurrent.address.c_str(); }
-    const char * GetAlias() const { return mCurrent.alias.c_str(); }
-    const char * GetName() const { return mCurrent.name.c_str(); }
-    bool IsPowered() const { return mCurrent.powered; }
-    BluezAdapter1 * GetAdapter() const { return mCurrent.adapter; }
+    uint32_t GetIndex() const;
+    const char * GetAddress() const { return bluez_adapter1_get_address(mCurrentAdapter.get()); }
+    const char * GetAlias() const { return bluez_adapter1_get_alias(mCurrentAdapter.get()); }
+    const char * GetName() const { return bluez_adapter1_get_name(mCurrentAdapter.get()); }
+    bool IsPowered() const { return bluez_adapter1_get_powered(mCurrentAdapter.get()); }
+    BluezAdapter1 * GetAdapter() const { return mCurrentAdapter.get(); }
 
 private:
     /// Sets up the DBUS manager and loads the list
@@ -73,23 +73,11 @@ class AdapterIterator
     /// iterate through.
     bool Advance();
 
-    static constexpr size_t kMaxAddressLength = 19; // xx:xx:xx:xx:xx:xx
-    static constexpr size_t kMaxNameLength    = 64;
-
     GDBusObjectManager * mManager = nullptr; // DBus connection
     GList * mObjectList           = nullptr; // listing of objects on the bus
     GList * mCurrentListItem      = nullptr; // current item viewed in the list
-
-    // data valid only if Next() returns true
-    struct
-    {
-        uint32_t index;
-        std::string address;
-        std::string alias;
-        std::string name;
-        bool powered;
-        BluezAdapter1 * adapter;
-    } mCurrent = { 0 };
+    // Data valid only if Next() returns true
+    GAutoPtr<BluezAdapter1> mCurrentAdapter;
 };
 
 } // namespace Internal