commit 1057dafa3f04af76913ffb7c69b27398e040da30
author Karsten Sperling <113487422+ksperling-apple@users.noreply.github.com> Fri May 31 07:47:32 2024 +1200
committer GitHub <noreply@github.com> Thu May 30 12:47:32 2024 -0700
tree 5e42aebfdb81ad233e90780cbd11fbef13d7c198
parent a76d1624cb222749592c8cd56550ec95c5ab2b9b

Darwin: Keep MTRCommissionableBrowser around until OnBleScanStopped (#33674)

* Darwin: Keep MTRCommissionableBrowser around until OnBleScanStopped

Because the BLE platform implementation uses a separate dispatch queue,
StopBleScan() does not synchrnously stop any further use of the current
BleScannerDelegate. Keep the MTRCommissionableBrowser that contains the
delegate alive until OnBleScanStopped to avoid a UAF.

* restyle

* Retain owner in Stop() instead of Start()

* More review comments

src/darwin/Framework/CHIP/MTRCommissionableBrowser.mm

diff --git a/src/darwin/Framework/CHIP/MTRCommissionableBrowser.mm b/src/darwin/Framework/CHIP/MTRCommissionableBrowser.mm
index 2b3e281..276fb5e 100644
--- a/src/darwin/Framework/CHIP/MTRCommissionableBrowser.mm
+++ b/src/darwin/Framework/CHIP/MTRCommissionableBrowser.mm
@@ -61,6 +61,10 @@
 #endif // CONFIG_NETWORK_LAYER_BLE
 {
 public:
+#if CONFIG_NETWORK_LAYER_BLE
+    id mBleScannerDelegateOwner;
+#endif // CONFIG_NETWORK_LAYER_BLE
+
     CHIP_ERROR Start(id<MTRCommissionableBrowserDelegate> delegate, MTRDeviceController * controller, dispatch_queue_t queue)
     {
         assertChipStackLockedByCurrentThread();
@@ -90,7 +94,7 @@
             chip::Inet::InterfaceId::Null(), this);
     }
 
-    CHIP_ERROR Stop()
+    CHIP_ERROR Stop(id owner)
     {
         assertChipStackLockedByCurrentThread();
 
@@ -109,7 +113,10 @@
         mDiscoveredResults = nil;
 
 #if CONFIG_NETWORK_LAYER_BLE
-        ReturnErrorOnFailure(PlatformMgrImpl().StopBleScan());
+        mBleScannerDelegateOwner = owner; // retain the owner until OnBleScanStopped is called
+        PlatformMgrImpl().StopBleScan(); // doesn't actually fail, and if it did we'd want to carry on regardless
+#else
+        (void) owner;
 #endif // CONFIG_NETWORK_LAYER_BLE
 
         return ChipDnssdStopBrowse(this);
@@ -281,6 +288,7 @@
     void OnBleScanAdd(BLE_CONNECTION_OBJECT connObj, const ChipBLEDeviceIdentificationInfo & info) override
     {
         assertChipStackLockedByCurrentThread();
+        VerifyOrReturn(mDelegate != nil);
 
         auto result = [[MTRCommissionableBrowserResult alloc] init];
         result.instanceName = [NSString stringWithUTF8String:kBleKey];
@@ -303,6 +311,7 @@
     void OnBleScanRemove(BLE_CONNECTION_OBJECT connObj) override
     {
         assertChipStackLockedByCurrentThread();
+        VerifyOrReturn(mDelegate != nil);
 
         auto key = [NSString stringWithFormat:@"%@", connObj];
         if ([mDiscoveredResults objectForKey:key] == nil) {
@@ -319,6 +328,12 @@
             [mDelegate controller:mController didFindCommissionableDevice:result];
         });
     }
+
+    void OnBleScanStopped() override
+    {
+        mBleScannerDelegateOwner = nil;
+    }
+
 #endif // CONFIG_NETWORK_LAYER_BLE
 
 private:
@@ -360,7 +375,7 @@
 
 - (BOOL)stop
 {
-    VerifyOrReturnValue(CHIP_NO_ERROR == _browser.Stop(), NO);
+    VerifyOrReturnValue(CHIP_NO_ERROR == _browser.Stop(self), NO);
     _delegate = nil;
     _controller = nil;
     _queue = nil;