Skip to content

Commit 1057daf

Browse files
authoredMay 30, 2024
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
1 parent a76d162 commit 1057daf

File tree

1 file changed

+18
-3
lines changed

1 file changed

+18
-3
lines changed
 

‎src/darwin/Framework/CHIP/MTRCommissionableBrowser.mm

+18-3
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,10 @@ @implementation MTRCommissionableBrowserResult
6161
#endif // CONFIG_NETWORK_LAYER_BLE
6262
{
6363
public:
64+
#if CONFIG_NETWORK_LAYER_BLE
65+
id mBleScannerDelegateOwner;
66+
#endif // CONFIG_NETWORK_LAYER_BLE
67+
6468
CHIP_ERROR Start(id<MTRCommissionableBrowserDelegate> delegate, MTRDeviceController * controller, dispatch_queue_t queue)
6569
{
6670
assertChipStackLockedByCurrentThread();
@@ -90,7 +94,7 @@ CHIP_ERROR Start(id<MTRCommissionableBrowserDelegate> delegate, MTRDeviceControl
9094
chip::Inet::InterfaceId::Null(), this);
9195
}
9296

93-
CHIP_ERROR Stop()
97+
CHIP_ERROR Stop(id owner)
9498
{
9599
assertChipStackLockedByCurrentThread();
96100

@@ -109,7 +113,10 @@ CHIP_ERROR Stop()
109113
mDiscoveredResults = nil;
110114

111115
#if CONFIG_NETWORK_LAYER_BLE
112-
ReturnErrorOnFailure(PlatformMgrImpl().StopBleScan());
116+
mBleScannerDelegateOwner = owner; // retain the owner until OnBleScanStopped is called
117+
PlatformMgrImpl().StopBleScan(); // doesn't actually fail, and if it did we'd want to carry on regardless
118+
#else
119+
(void) owner;
113120
#endif // CONFIG_NETWORK_LAYER_BLE
114121

115122
return ChipDnssdStopBrowse(this);
@@ -281,6 +288,7 @@ void OnBrowseStop(CHIP_ERROR error) override
281288
void OnBleScanAdd(BLE_CONNECTION_OBJECT connObj, const ChipBLEDeviceIdentificationInfo & info) override
282289
{
283290
assertChipStackLockedByCurrentThread();
291+
VerifyOrReturn(mDelegate != nil);
284292

285293
auto result = [[MTRCommissionableBrowserResult alloc] init];
286294
result.instanceName = [NSString stringWithUTF8String:kBleKey];
@@ -303,6 +311,7 @@ void OnBleScanAdd(BLE_CONNECTION_OBJECT connObj, const ChipBLEDeviceIdentificati
303311
void OnBleScanRemove(BLE_CONNECTION_OBJECT connObj) override
304312
{
305313
assertChipStackLockedByCurrentThread();
314+
VerifyOrReturn(mDelegate != nil);
306315

307316
auto key = [NSString stringWithFormat:@"%@", connObj];
308317
if ([mDiscoveredResults objectForKey:key] == nil) {
@@ -319,6 +328,12 @@ void OnBleScanRemove(BLE_CONNECTION_OBJECT connObj) override
319328
[mDelegate controller:mController didFindCommissionableDevice:result];
320329
});
321330
}
331+
332+
void OnBleScanStopped() override
333+
{
334+
mBleScannerDelegateOwner = nil;
335+
}
336+
322337
#endif // CONFIG_NETWORK_LAYER_BLE
323338

324339
private:
@@ -360,7 +375,7 @@ - (BOOL)start
360375

361376
- (BOOL)stop
362377
{
363-
VerifyOrReturnValue(CHIP_NO_ERROR == _browser.Stop(), NO);
378+
VerifyOrReturnValue(CHIP_NO_ERROR == _browser.Stop(self), NO);
364379
_delegate = nil;
365380
_controller = nil;
366381
_queue = nil;

0 commit comments

Comments
 (0)