From fb56c4181f727658f5a78eaab825c6f95030b542 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Tue, 2 May 2023 19:38:54 +0000 Subject: [PATCH 1/6] chip-repl hits the Code is unsafe/racy assert when BLE commissioning is started --- src/platform/Linux/bluez/ChipDeviceScanner.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/platform/Linux/bluez/ChipDeviceScanner.cpp b/src/platform/Linux/bluez/ChipDeviceScanner.cpp index 0172c820c5a0cc..57183603f44639 100644 --- a/src/platform/Linux/bluez/ChipDeviceScanner.cpp +++ b/src/platform/Linux/bluez/ChipDeviceScanner.cpp @@ -136,7 +136,9 @@ CHIP_ERROR ChipDeviceScanner::StartScan(System::Clock::Timeout timeout) return CHIP_ERROR_INTERNAL; } + DeviceLayer::PlatformMgr().LockChipStack(); CHIP_ERROR err = chip::DeviceLayer::SystemLayer().StartTimer(timeout, TimerExpiredCallback, static_cast(this)); + DeviceLayer::PlatformMgr().UnlockChipStack(); if (err != CHIP_NO_ERROR) { From 22d495819fc832439d19e2dd826a66007de4ea70 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Tue, 2 May 2023 19:46:22 +0000 Subject: [PATCH 2/6] Add lock/unlock to CancelTimer --- src/platform/Linux/bluez/ChipDeviceScanner.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/platform/Linux/bluez/ChipDeviceScanner.cpp b/src/platform/Linux/bluez/ChipDeviceScanner.cpp index 57183603f44639..2ac7e613de4f1a 100644 --- a/src/platform/Linux/bluez/ChipDeviceScanner.cpp +++ b/src/platform/Linux/bluez/ChipDeviceScanner.cpp @@ -79,7 +79,9 @@ ChipDeviceScanner::~ChipDeviceScanner() StopScan(); // In case the timeout timer is still active + DeviceLayer::PlatformMgr().LockChipStack(); chip::DeviceLayer::SystemLayer().CancelTimer(TimerExpiredCallback, this); + DeviceLayer::PlatformMgr().UnlockChipStack(); g_object_unref(mManager); g_object_unref(mCancellable); From 86ad94f2d4da218fb1d6d14ce376b6ef495043b6 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Wed, 3 May 2023 20:27:18 +0000 Subject: [PATCH 3/6] Fix a few other issue --- .../python/chip/ble/library_handle.py | 4 ++-- .../python/chip/ble/scan_devices.py | 22 +++++++++++++++---- src/controller/python/chip/ble/types.py | 2 ++ .../Linux/bluez/ChipDeviceScanner.cpp | 10 ++++----- 4 files changed, 27 insertions(+), 11 deletions(-) diff --git a/src/controller/python/chip/ble/library_handle.py b/src/controller/python/chip/ble/library_handle.py index 52f83d65b180b8..2dbb52f28bf429 100644 --- a/src/controller/python/chip/ble/library_handle.py +++ b/src/controller/python/chip/ble/library_handle.py @@ -18,7 +18,7 @@ from ctypes import c_bool, c_char_p, c_uint32, c_void_p, py_object import chip.native -from chip.ble.types import DeviceScannedCallback, ScanDoneCallback +from chip.ble.types import DeviceScannedCallback, ScanDoneCallback, ScanErrorCallback # This prevents python auto-casting c_void_p to integers and @@ -58,7 +58,7 @@ def _GetBleLibraryHandle() -> ctypes.CDLL: VoidPointer, [VoidPointer]) setter.Set('pychip_ble_start_scanning', VoidPointer, [ - py_object, VoidPointer, c_uint32, DeviceScannedCallback, ScanDoneCallback + py_object, VoidPointer, c_uint32, DeviceScannedCallback, ScanDoneCallback, ScanErrorCallback ]) return handle diff --git a/src/controller/python/chip/ble/scan_devices.py b/src/controller/python/chip/ble/scan_devices.py index 700f39b7e1de62..b18307f22403e8 100644 --- a/src/controller/python/chip/ble/scan_devices.py +++ b/src/controller/python/chip/ble/scan_devices.py @@ -20,7 +20,7 @@ from typing import Generator from chip.ble.library_handle import _GetBleLibraryHandle -from chip.ble.types import DeviceScannedCallback, ScanDoneCallback +from chip.ble.types import DeviceScannedCallback, ScanDoneCallback, ScanErrorCallback @DeviceScannedCallback @@ -33,8 +33,12 @@ def ScanFoundCallback(closure, address: str, discriminator: int, vendor: int, def ScanDoneCallback(closure): closure.ScanCompleted() +@ScanErrorCallback +def ScanErrorCallback(closure, errorCode: int): + closure.ScanErrorCallback(errorCode) -def DiscoverAsync(timeoutMs: int, scanCallback, doneCallback, adapter=None): + +def DiscoverAsync(timeoutMs: int, scanCallback, doneCallback, errorCallback, adapter=None): """Initiate a BLE discovery of devices with the given timeout. NOTE: devices are not guaranteed to be unique. New entries are returned @@ -44,6 +48,7 @@ def DiscoverAsync(timeoutMs: int, scanCallback, doneCallback, adapter=None): timeoutMs: scan will complete after this time scanCallback: callback when a device is found doneCallback: callback when the scan is complete + errorCallback: callback when error occurred during scan adapter: what adapter to choose. Either an AdapterInfo object or a string with the adapter address. If None, the first adapter on the system is used. @@ -72,6 +77,9 @@ def ScanCompleted(self, *args): doneCallback(*args) ctypes.pythonapi.Py_DecRef(ctypes.py_object(self)) + def ScanErrorCallback(self, *args): + errorCallback(*args) + closure = ScannerClosure() ctypes.pythonapi.Py_IncRef(ctypes.py_object(closure)) @@ -79,7 +87,7 @@ def ScanCompleted(self, *args): ctypes.py_object(closure), handle.pychip_ble_adapter_list_get_raw_adapter( nativeList), timeoutMs, - ScanFoundCallback, ScanDoneCallback) + ScanFoundCallback, ScanDoneCallback, ScanErrorCallback) if scanner == 0: raise Exception('Failed to initiate scan') @@ -113,6 +121,12 @@ def DeviceFound(self, address, discriminator, vendor, product): def ScanCompleted(self): self.queue.put(None) + def ScanError(self, errorCode): + # TODO need to determine what we do with this error. Most of the time this + # error is just a timeout introduced in PR #24873, right before we get a + # ScanCompleted. + pass + def DiscoverSync(timeoutMs: int, adapter=None) -> Generator[DeviceInfo, None, None]: """Discover BLE devices over the specified period of time. @@ -131,7 +145,7 @@ def DiscoverSync(timeoutMs: int, adapter=None) -> Generator[DeviceInfo, None, No receiver = _DeviceInfoReceiver() DiscoverAsync(timeoutMs, receiver.DeviceFound, - receiver.ScanCompleted, adapter) + receiver.ScanCompleted, receiver.ScanError, adapter) while True: data = receiver.queue.get() diff --git a/src/controller/python/chip/ble/types.py b/src/controller/python/chip/ble/types.py index b56a2e5670b012..3dd5fe65a1b6c7 100644 --- a/src/controller/python/chip/ble/types.py +++ b/src/controller/python/chip/ble/types.py @@ -20,3 +20,5 @@ c_uint16, c_uint16) ScanDoneCallback = CFUNCTYPE(None, py_object) + +ScanErrorCallback = CFUNCTYPE(None, py_object, c_uint16) \ No newline at end of file diff --git a/src/platform/Linux/bluez/ChipDeviceScanner.cpp b/src/platform/Linux/bluez/ChipDeviceScanner.cpp index 2ac7e613de4f1a..64270d825688e1 100644 --- a/src/platform/Linux/bluez/ChipDeviceScanner.cpp +++ b/src/platform/Linux/bluez/ChipDeviceScanner.cpp @@ -184,6 +184,11 @@ CHIP_ERROR ChipDeviceScanner::StopScan() return CHIP_ERROR_INTERNAL; } + ChipDeviceScannerDelegate * delegate = this->mDelegate; + // callback is explicitly allowed to delete the scanner (hence no more + // references to this here) + delegate->OnScanComplete(); + return CHIP_NO_ERROR; } @@ -196,13 +201,8 @@ CHIP_ERROR ChipDeviceScanner::MainLoopStopScan(ChipDeviceScanner * self) ChipLogError(Ble, "Failed to stop discovery %s", error->message); g_error_free(error); } - ChipDeviceScannerDelegate * delegate = self->mDelegate; self->mIsScanning = false; - // callback is explicitly allowed to delete the scanner (hence no more - // references to 'self' here) - delegate->OnScanComplete(); - return CHIP_NO_ERROR; } From a6d999ed3353fead1cb78751e84404124cb04b91 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Wed, 3 May 2023 20:40:45 +0000 Subject: [PATCH 4/6] Restyle --- src/controller/python/chip/ble/scan_devices.py | 1 + src/controller/python/chip/ble/types.py | 2 +- src/platform/Linux/bluez/ChipDeviceScanner.cpp | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/controller/python/chip/ble/scan_devices.py b/src/controller/python/chip/ble/scan_devices.py index b18307f22403e8..5422afb7cbb88b 100644 --- a/src/controller/python/chip/ble/scan_devices.py +++ b/src/controller/python/chip/ble/scan_devices.py @@ -33,6 +33,7 @@ def ScanFoundCallback(closure, address: str, discriminator: int, vendor: int, def ScanDoneCallback(closure): closure.ScanCompleted() + @ScanErrorCallback def ScanErrorCallback(closure, errorCode: int): closure.ScanErrorCallback(errorCode) diff --git a/src/controller/python/chip/ble/types.py b/src/controller/python/chip/ble/types.py index 3dd5fe65a1b6c7..94b1789e9c0673 100644 --- a/src/controller/python/chip/ble/types.py +++ b/src/controller/python/chip/ble/types.py @@ -21,4 +21,4 @@ ScanDoneCallback = CFUNCTYPE(None, py_object) -ScanErrorCallback = CFUNCTYPE(None, py_object, c_uint16) \ No newline at end of file +ScanErrorCallback = CFUNCTYPE(None, py_object, c_uint16) diff --git a/src/platform/Linux/bluez/ChipDeviceScanner.cpp b/src/platform/Linux/bluez/ChipDeviceScanner.cpp index 64270d825688e1..0eaadc206934a9 100644 --- a/src/platform/Linux/bluez/ChipDeviceScanner.cpp +++ b/src/platform/Linux/bluez/ChipDeviceScanner.cpp @@ -201,7 +201,7 @@ CHIP_ERROR ChipDeviceScanner::MainLoopStopScan(ChipDeviceScanner * self) ChipLogError(Ble, "Failed to stop discovery %s", error->message); g_error_free(error); } - self->mIsScanning = false; + self->mIsScanning = false; return CHIP_NO_ERROR; } From c633ade70a6e17f9a5c1db99931f1502993dd760 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Wed, 3 May 2023 20:47:36 +0000 Subject: [PATCH 5/6] Small fix --- src/platform/Linux/bluez/ChipDeviceScanner.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/platform/Linux/bluez/ChipDeviceScanner.cpp b/src/platform/Linux/bluez/ChipDeviceScanner.cpp index 0eaadc206934a9..db3aadc16a243a 100644 --- a/src/platform/Linux/bluez/ChipDeviceScanner.cpp +++ b/src/platform/Linux/bluez/ChipDeviceScanner.cpp @@ -79,9 +79,7 @@ ChipDeviceScanner::~ChipDeviceScanner() StopScan(); // In case the timeout timer is still active - DeviceLayer::PlatformMgr().LockChipStack(); chip::DeviceLayer::SystemLayer().CancelTimer(TimerExpiredCallback, this); - DeviceLayer::PlatformMgr().UnlockChipStack(); g_object_unref(mManager); g_object_unref(mCancellable); From 53f54463aa660a9b41689263b7ac3dfcf116cc46 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Thu, 4 May 2023 13:55:03 +0000 Subject: [PATCH 6/6] Last few fixes --- src/platform/Linux/bluez/ChipDeviceScanner.cpp | 16 +++++++++++++--- src/platform/Linux/bluez/ChipDeviceScanner.h | 5 +++++ 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/platform/Linux/bluez/ChipDeviceScanner.cpp b/src/platform/Linux/bluez/ChipDeviceScanner.cpp index db3aadc16a243a..7c47e735d5dfc1 100644 --- a/src/platform/Linux/bluez/ChipDeviceScanner.cpp +++ b/src/platform/Linux/bluez/ChipDeviceScanner.cpp @@ -78,8 +78,16 @@ ChipDeviceScanner::~ChipDeviceScanner() { StopScan(); - // In case the timeout timer is still active - chip::DeviceLayer::SystemLayer().CancelTimer(TimerExpiredCallback, this); + // mTimerExpired should only be set to true in the TimerExpiredCallback, which means we are in that callback + // right now so there is no need to cancel the timer. Doing so would result in deadlock trying to aquire the + // chip stack lock which we already currently have. + if (!mTimerExpired) + { + // In case the timeout timer is still active + DeviceLayer::PlatformMgr().LockChipStack(); + chip::DeviceLayer::SystemLayer().CancelTimer(TimerExpiredCallback, this); + DeviceLayer::PlatformMgr().UnlockChipStack(); + } g_object_unref(mManager); g_object_unref(mCancellable); @@ -146,6 +154,7 @@ CHIP_ERROR ChipDeviceScanner::StartScan(System::Clock::Timeout timeout) StopScan(); return err; } + mTimerExpired = false; return CHIP_NO_ERROR; } @@ -153,6 +162,7 @@ CHIP_ERROR ChipDeviceScanner::StartScan(System::Clock::Timeout timeout) void ChipDeviceScanner::TimerExpiredCallback(chip::System::Layer * layer, void * appState) { ChipDeviceScanner * chipDeviceScanner = static_cast(appState); + chipDeviceScanner->MarkTimerExpired(); chipDeviceScanner->mDelegate->OnScanError(CHIP_ERROR_TIMEOUT); chipDeviceScanner->StopScan(); } @@ -184,7 +194,7 @@ CHIP_ERROR ChipDeviceScanner::StopScan() ChipDeviceScannerDelegate * delegate = this->mDelegate; // callback is explicitly allowed to delete the scanner (hence no more - // references to this here) + // references to 'self' here) delegate->OnScanComplete(); return CHIP_NO_ERROR; diff --git a/src/platform/Linux/bluez/ChipDeviceScanner.h b/src/platform/Linux/bluez/ChipDeviceScanner.h index ad12ba9b31c8d8..4dbb244c8772da 100644 --- a/src/platform/Linux/bluez/ChipDeviceScanner.h +++ b/src/platform/Linux/bluez/ChipDeviceScanner.h @@ -71,6 +71,9 @@ class ChipDeviceScanner /// Stop any currently running scan CHIP_ERROR StopScan(); + /// Should only be called by TimerExpiredCallback. + void MarkTimerExpired() { mTimerExpired = true; } + /// Create a new device scanner /// /// Convenience method to allocate any required variables. @@ -101,6 +104,8 @@ class ChipDeviceScanner gulong mInterfaceChangedSignal = 0; bool mIsScanning = false; bool mIsStopping = false; + /// Used to track if timer has alread expired and doesn't need to be canceled. + bool mTimerExpired = false; }; } // namespace Internal