[Linux] Verify BLE service before reporting new connection (project-c…

…hip#33893)

* Simplify logic in event posting helpers

* [Linux] Fix discovery of C3 characteristic

* [Linux] Check for spec-compliant CHIPoBLE service

* Update name convention

* Use C++ boolean type

* Simplify memory management

* Verify characteristic flags

* C2 should have indication property set only

* Restyled by clang-format

---------

Co-authored-by: Restyled.io <[email protected]>

src/app/tests/suites/certification/Test_TC_CNET_4_2.yaml

@@ -56,7 +56,7 @@ tests:
  [1698660637.937911][6429:6431] CHIP:IN: Clearing BLE pending packets.
  [1698660637.938582][6429:6431] CHIP:BLE: Auto-closing end point's BLE connection.
  [1698660637.938645][6429:6431] CHIP:DL: Closing BLE GATT connection (con 0xffff9c034bb0)
- [1698660637.938805][6429:6430] CHIP:DL: BluezDisconnect peer=F7:D4:24:D2:4A:1F
+ [1698660637.938805][6429:6430] CHIP:DL: Close BLE connection: peer=F7:D4:24:D2:4A:1F
  [1698660638.365208][6429:6431] CHIP:IN: SecureSession[0xffff9400f900]: MarkForEviction Type:1 LSID:62220
  [1698660638.365311][6429:6431] CHIP:SC: SecureSession[0xffff9400f900, LSID:62220]: State change 'kActive' --> 'kPendingEviction'
  [1698660638.365440][6429:6431] CHIP:IN: SecureSession[0xffff9400f900]: Released - Type:1 LSID:62220

src/platform/Linux/BLEManagerImpl.cpp

@@ -476,74 +476,52 @@ void BLEManagerImpl::HandleSubscribeOpComplete(BLE_CONNECTION_OBJECT conId, bool
 
 void BLEManagerImpl::HandleTXCharChanged(BLE_CONNECTION_OBJECT conId, const uint8_t * value, size_t len)
 {
- CHIP_ERROR err = CHIP_NO_ERROR;
- System::PacketBufferHandle buf = System::PacketBufferHandle::NewWithData(value, len);
+ System::PacketBufferHandle buf(System::PacketBufferHandle::NewWithData(value, len));
+ VerifyOrReturn(!buf.IsNull(), ChipLogError(DeviceLayer, "Failed to allocate packet buffer in %s", __func__));
 
  ChipLogDetail(DeviceLayer, "Indication received, conn = %p", conId);
 
  ChipDeviceEvent event{ .Type = DeviceEventType::kPlatformLinuxBLEIndicationReceived,
- .Platform = { .BLEIndicationReceived = { .mConnection = conId } } };
-
- VerifyOrExit(!buf.IsNull(), err = CHIP_ERROR_NO_MEMORY);
-
- event.Platform.BLEIndicationReceived.mData = std::move(buf).UnsafeRelease();
+ .Platform = {
+ .BLEIndicationReceived = { .mConnection = conId, .mData = std::move(buf).UnsafeRelease() } } };
  PlatformMgr().PostEventOrDie(&event);
-
-exit:
- if (err != CHIP_NO_ERROR)
- ChipLogError(DeviceLayer, "HandleTXCharChanged() failed: %s", ErrorStr(err));
 }
 
 void BLEManagerImpl::HandleRXCharWrite(BLE_CONNECTION_OBJECT conId, const uint8_t * value, size_t len)
 {
- CHIP_ERROR err = CHIP_NO_ERROR;
- System::PacketBufferHandle buf;
-
  // Copy the data to a packet buffer.
- buf = System::PacketBufferHandle::NewWithData(value, len);
- VerifyOrExit(!buf.IsNull(), err = CHIP_ERROR_NO_MEMORY);
+ System::PacketBufferHandle buf(System::PacketBufferHandle::NewWithData(value, len));
+ VerifyOrReturn(!buf.IsNull(), ChipLogError(DeviceLayer, "Failed to allocate packet buffer in %s", __func__));
 
- // Post an event to the Chip queue to deliver the data into the Chip stack.
- {
- ChipLogProgress(Ble, "Write request received debug %p", conId);
- ChipDeviceEvent event{ .Type = DeviceEventType::kCHIPoBLEWriteReceived,
- .CHIPoBLEWriteReceived = { .ConId = conId, .Data = std::move(buf).UnsafeRelease() } };
- PlatformMgr().PostEventOrDie(&event);
- }
+ ChipLogProgress(Ble, "Write request received, conn = %p", conId);
 
-exit:
- if (err != CHIP_NO_ERROR)
- {
- ChipLogError(DeviceLayer, "HandleRXCharWrite() failed: %s", ErrorStr(err));
- }
+ // Post an event to the Chip queue to deliver the data into the Chip stack.
+ ChipDeviceEvent event{ .Type = DeviceEventType::kCHIPoBLEWriteReceived,
+ .CHIPoBLEWriteReceived = { .ConId = conId, .Data = std::move(buf).UnsafeRelease() } };
+ PlatformMgr().PostEventOrDie(&event);
 }
 
-void BLEManagerImpl::CHIPoBluez_ConnectionClosed(BLE_CONNECTION_OBJECT conId)
+void BLEManagerImpl::HandleConnectionClosed(BLE_CONNECTION_OBJECT conId)
 {
- ChipLogProgress(DeviceLayer, "Bluez notify CHIPoBluez connection disconnected");
-
  // If this was a CHIPoBLE connection, post an event to deliver a connection error to the CHIPoBLE layer.
- {
- ChipDeviceEvent event{ .Type = DeviceEventType::kCHIPoBLEConnectionError,
- .CHIPoBLEConnectionError = { .ConId = conId, .Reason = BLE_ERROR_REMOTE_DEVICE_DISCONNECTED } };
- PlatformMgr().PostEventOrDie(&event);
- }
+ ChipDeviceEvent event{ .Type = DeviceEventType::kCHIPoBLEConnectionError,
+ .CHIPoBLEConnectionError = { .ConId = conId, .Reason = BLE_ERROR_REMOTE_DEVICE_DISCONNECTED } };
+ PlatformMgr().PostEventOrDie(&event);
 }
 
 void BLEManagerImpl::HandleTXCharCCCDWrite(BLE_CONNECTION_OBJECT conId)
 {
  VerifyOrReturn(conId != BLE_CONNECTION_UNINITIALIZED,
  ChipLogError(DeviceLayer, "BLE connection is not initialized in %s", __func__));
 
+ ChipLogProgress(DeviceLayer, "CHIPoBLE %s received", conId->IsNotifyAcquired() ? "subscribe" : "unsubscribe");
+
  // Post an event to the Chip queue to process either a CHIPoBLE Subscribe or Unsubscribe based on
  // whether the client is enabling or disabling indications.
  ChipDeviceEvent event{ .Type = conId->IsNotifyAcquired() ? static_cast<uint16_t>(DeviceEventType::kCHIPoBLESubscribe)
  : static_cast<uint16_t>(DeviceEventType::kCHIPoBLEUnsubscribe),
  .CHIPoBLESubscribe = { .ConId = conId } };
  PlatformMgr().PostEventOrDie(&event);
-
- ChipLogProgress(DeviceLayer, "CHIPoBLE %s received",
- (event.Type == DeviceEventType::kCHIPoBLESubscribe) ? "subscribe" : "unsubscribe");
 }
 
 void BLEManagerImpl::HandleTXComplete(BLE_CONNECTION_OBJECT conId)

src/platform/Linux/BLEManagerImpl.h

@@ -87,7 +87,7 @@ class BLEManagerImpl final : public BLEManager,
  static void HandleSubscribeOpComplete(BLE_CONNECTION_OBJECT conId, bool subscribed);
  static void HandleTXCharChanged(BLE_CONNECTION_OBJECT conId, const uint8_t * value, size_t len);
  static void HandleRXCharWrite(BLE_CONNECTION_OBJECT user_data, const uint8_t * value, size_t len);
- static void CHIPoBluez_ConnectionClosed(BLE_CONNECTION_OBJECT user_data);
+ static void HandleConnectionClosed(BLE_CONNECTION_OBJECT user_data);
  static void HandleTXCharCCCDWrite(BLE_CONNECTION_OBJECT user_data);
  static void HandleTXComplete(BLE_CONNECTION_OBJECT user_data);
 

src/platform/Linux/bluez/BluezConnection.cpp

@@ -24,6 +24,7 @@
 #include <gio/gio.h>
 #include <glib.h>
 
+#include <ble/Ble.h>
 #include <lib/support/CHIPMem.h>
 #include <lib/support/CodeUtils.h>
 #include <platform/ConnectivityManager.h>
@@ -42,29 +43,31 @@ namespace Internal {
 
 namespace {
 
-gboolean BluezIsServiceOnDevice(BluezGattService1 * aService, BluezDevice1 * aDevice)
+bool BluezIsServiceOnDevice(BluezGattService1 * aService, BluezDevice1 * aDevice)
 {
- const auto * servicePath = bluez_gatt_service1_get_device(aService);
- const auto * devicePath = g_dbus_proxy_get_object_path(reinterpret_cast<GDBusProxy *>(aDevice));
- return strcmp(servicePath, devicePath) == 0 ? TRUE : FALSE;
+ auto servicePath = bluez_gatt_service1_get_device(aService);
+ auto devicePath = g_dbus_proxy_get_object_path(reinterpret_cast<GDBusProxy *>(aDevice));
+ return strcmp(servicePath, devicePath) == 0;
 }
 
-gboolean BluezIsCharOnService(BluezGattCharacteristic1 * aChar, BluezGattService1 * aService)
+bool BluezIsCharOnService(BluezGattCharacteristic1 * aChar, BluezGattService1 * aService)
 {
- const auto * charPath = bluez_gatt_characteristic1_get_service(aChar);
- const auto * servicePath = g_dbus_proxy_get_object_path(reinterpret_cast<GDBusProxy *>(aService));
- ChipLogDetail(DeviceLayer, "Char %s on service %s", charPath, servicePath);
- return strcmp(charPath, servicePath) == 0 ? TRUE : FALSE;
+ auto charPath = bluez_gatt_characteristic1_get_service(aChar);
+ auto servicePath = g_dbus_proxy_get_object_path(reinterpret_cast<GDBusProxy *>(aService));
+ return strcmp(charPath, servicePath) == 0;
 }
 
-} // namespace
-
-BluezConnection::BluezConnection(const BluezEndpoint & aEndpoint, BluezDevice1 & aDevice) :
- mDevice(reinterpret_cast<BluezDevice1 *>(g_object_ref(&aDevice)))
+bool BluezIsFlagOnChar(BluezGattCharacteristic1 * aChar, const char * flag)
 {
- Init(aEndpoint);
+ auto charFlags = bluez_gatt_characteristic1_get_flags(aChar);
+ for (size_t i = 0; charFlags[i] != nullptr; i++)
+ if (strcmp(charFlags[i], flag) == 0)
+ return true;
+ return false;
 }
 
+} // namespace
+
 BluezConnection::IOChannel::~IOChannel()
 {
  if (mWatchSource != nullptr)
@@ -85,73 +88,69 @@ CHIP_ERROR BluezConnection::Init(const BluezEndpoint & aEndpoint)
  mService.reset(reinterpret_cast<BluezGattService1 *>(g_object_ref(aEndpoint.mService.get())));
  mC1.reset(reinterpret_cast<BluezGattCharacteristic1 *>(g_object_ref(aEndpoint.mC1.get())));
  mC2.reset(reinterpret_cast<BluezGattCharacteristic1 *>(g_object_ref(aEndpoint.mC2.get())));
+ return CHIP_NO_ERROR;
  }
- else
+
+ for (BluezObject & object : aEndpoint.mObjectManager.GetObjects())
  {
- for (BluezObject & object : aEndpoint.mObjectManager.GetObjects())
+ GAutoPtr<BluezGattService1> service(bluez_object_get_gatt_service1(&object));
+ if (service && BluezIsServiceOnDevice(service.get(), mDevice.get()))
  {
- BluezGattService1 * service = bluez_object_get_gatt_service1(&object);
- if (service != nullptr)
+ if (strcmp(bluez_gatt_service1_get_uuid(service.get()), Ble::CHIP_BLE_SERVICE_LONG_UUID_STR) == 0)
  {
- if ((BluezIsServiceOnDevice(service, mDevice.get())) == TRUE &&
- (strcmp(bluez_gatt_service1_get_uuid(service), Ble::CHIP_BLE_SERVICE_LONG_UUID_STR) == 0))
- {
- mService.reset(service);
- break;
- }
- g_object_unref(service);
+ ChipLogDetail(DeviceLayer, "CHIP service found");
+ mService.reset(service.release());
+ break;
  }
  }
+ }
 
- VerifyOrExit(mService, ChipLogError(DeviceLayer, "FAIL: NULL service in %s", __func__));
+ VerifyOrReturnError(
+ mService, BLE_ERROR_NOT_CHIP_DEVICE,
+ ChipLogError(DeviceLayer, "CHIP service (%s) not found on %s", Ble::CHIP_BLE_SERVICE_LONG_UUID_STR, GetPeerAddress()));
 
- for (BluezObject & object : aEndpoint.mObjectManager.GetObjects())
+ for (BluezObject & object : aEndpoint.mObjectManager.GetObjects())
+ {
+ GAutoPtr<BluezGattCharacteristic1> chr(bluez_object_get_gatt_characteristic1(&object));
+ if (chr && BluezIsCharOnService(chr.get(), mService.get()))
  {
- BluezGattCharacteristic1 * char1 = bluez_object_get_gatt_characteristic1(&object);
- if (char1 != nullptr)
+ if (strcmp(bluez_gatt_characteristic1_get_uuid(chr.get()), Ble::CHIP_BLE_CHAR_1_UUID_STR) == 0 &&
+  BluezIsFlagOnChar(chr.get(), "write"))
  {
- if ((BluezIsCharOnService(char1, mService.get()) == TRUE) &&
- (strcmp(bluez_gatt_characteristic1_get_uuid(char1), Ble::CHIP_BLE_CHAR_1_UUID_STR) == 0))
- {
- mC1.reset(char1);
- }
- else if ((BluezIsCharOnService(char1, mService.get()) == TRUE) &&
- (strcmp(bluez_gatt_characteristic1_get_uuid(char1), Ble::CHIP_BLE_CHAR_2_UUID_STR) == 0))
- {
- mC2.reset(char1);
- }
+ ChipLogDetail(DeviceLayer, "Valid C1 characteristic found");
+ mC1.reset(chr.release());
+ }
+ else if (strcmp(bluez_gatt_characteristic1_get_uuid(chr.get()), Ble::CHIP_BLE_CHAR_2_UUID_STR) == 0 &&
+ BluezIsFlagOnChar(chr.get(), "indicate"))
+ {
+ ChipLogDetail(DeviceLayer, "Valid C2 characteristic found");
+ mC2.reset(chr.release());
+ }
 #if CHIP_ENABLE_ADDITIONAL_DATA_ADVERTISING
- else if ((BluezIsCharOnService(char1, mService.get()) == TRUE) &&
- (strcmp(bluez_gatt_characteristic1_get_uuid(char1), Ble::CHIP_BLE_CHAR_3_UUID_STR) == 0))
- {
- mC3.reset(char1);
- }
-#endif
- else
- {
- g_object_unref(char1);
- }
- if (mC1 && mC2)
- {
- break;
- }
+ else if (strcmp(bluez_gatt_characteristic1_get_uuid(chr.get()), Ble::CHIP_BLE_CHAR_3_UUID_STR) == 0 &&
+ BluezIsFlagOnChar(chr.get(), "read"))
+ {
+ ChipLogDetail(DeviceLayer, "Valid C3 characteristic found");
+ mC3.reset(chr.release());
  }
+#endif
  }
-
- VerifyOrExit(mC1, ChipLogError(DeviceLayer, "FAIL: NULL C1 in %s", __func__));
- VerifyOrExit(mC2, ChipLogError(DeviceLayer, "FAIL: NULL C2 in %s", __func__));
  }
 
-exit:
+ VerifyOrReturnError(mC1, BLE_ERROR_NOT_CHIP_DEVICE,
+ ChipLogError(DeviceLayer, "No valid C1 (%s) on %s", Ble::CHIP_BLE_CHAR_1_UUID_STR, GetPeerAddress()));
+ VerifyOrReturnError(mC2, BLE_ERROR_NOT_CHIP_DEVICE,
+ ChipLogError(DeviceLayer, "No valid C2 (%s) on %s", Ble::CHIP_BLE_CHAR_2_UUID_STR, GetPeerAddress()));
+
  return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR BluezConnection::BluezDisconnect(BluezConnection * conn)
+CHIP_ERROR BluezConnection::CloseConnectionImpl(BluezConnection * conn)
 {
  GAutoPtr<GError> error;
  gboolean success;
 
- ChipLogDetail(DeviceLayer, "%s peer=%s", __func__, conn->GetPeerAddress());
+ ChipLogDetail(DeviceLayer, "Close BLE connection: peer=%s", conn->GetPeerAddress());
 
  success = bluez_device1_call_disconnect_sync(conn->mDevice.get(), nullptr, &error.GetReceiver());
  VerifyOrExit(success == TRUE, ChipLogError(DeviceLayer, "FAIL: Disconnect: %s", error->message));
@@ -162,7 +161,7 @@ CHIP_ERROR BluezConnection::BluezDisconnect(BluezConnection * conn)
 
 CHIP_ERROR BluezConnection::CloseConnection()
 {
- return PlatformMgrImpl().GLibMatterContextInvokeSync(BluezDisconnect, this);
+ return PlatformMgrImpl().GLibMatterContextInvokeSync(CloseConnectionImpl, this);
 }
 
 const char * BluezConnection::GetPeerAddress() const

src/platform/Linux/bluez/BluezConnection.h

@@ -39,9 +39,11 @@ class BluezEndpoint;
 class BluezConnection
 {
 public:
- BluezConnection(const BluezEndpoint & aEndpoint, BluezDevice1 & aDevice);
+ BluezConnection(BluezDevice1 & aDevice) : mDevice(reinterpret_cast<BluezDevice1 *>(g_object_ref(&aDevice))) {}
  ~BluezConnection() = default;
 
+ CHIP_ERROR Init(const BluezEndpoint & aEndpoint);
+
  const char * GetPeerAddress() const;
 
  uint16_t GetMTU() const { return mMtu; }
@@ -97,9 +99,7 @@ class BluezConnection
  GAutoPtr<GVariant> mData;
  };
 
- CHIP_ERROR Init(const BluezEndpoint & aEndpoint);
-
- static CHIP_ERROR BluezDisconnect(BluezConnection * apConn);
+ static CHIP_ERROR CloseConnectionImpl(BluezConnection * apConn);
 
  static gboolean WriteHandlerCallback(GIOChannel * aChannel, GIOCondition aCond, BluezConnection * apConn);
  static gboolean NotifyHandlerCallback(GIOChannel * aChannel, GIOCondition aCond, BluezConnection * apConn);

src/platform/Linux/bluez/BluezEndpoint.cpp

@@ -299,21 +299,21 @@ CHIP_ERROR BluezEndpoint::RegisterGattApplicationImpl()
 /// Update the table of open BLE connections whenever a new device is spotted or its attributes have changed.
 void BluezEndpoint::UpdateConnectionTable(BluezDevice1 & aDevice)
 {
- const char * objectPath  = g_dbus_proxy_get_object_path(reinterpret_cast<GDBusProxy *>(&aDevice));
- BluezConnection * connection = GetBluezConnection(objectPath);
+ const char * objectPath = g_dbus_proxy_get_object_path(reinterpret_cast<GDBusProxy *>(&aDevice));
+ BluezConnection * conn  = GetBluezConnection(objectPath);
 
- if (connection != nullptr && !bluez_device1_get_connected(&aDevice))
+ if (conn != nullptr && !bluez_device1_get_connected(&aDevice))
  {
- ChipLogDetail(DeviceLayer, "Bluez disconnected");
- BLEManagerImpl::CHIPoBluez_ConnectionClosed(connection);
+ ChipLogDetail(DeviceLayer, "BLE connection closed: conn=%p", conn);
+ BLEManagerImpl::HandleConnectionClosed(conn);
  mConnMap.erase(objectPath);
  // TODO: the connection object should be released after BLEManagerImpl finishes cleaning up its resources
  // after the disconnection. Releasing it here doesn't cause any issues, but it's error-prone.
- chip::Platform::Delete(connection);
+ chip::Platform::Delete(conn);
  return;
  }
 
- if (connection == nullptr)
+ if (conn == nullptr)
  {
  HandleNewDevice(aDevice);
  }
@@ -323,20 +323,29 @@ void BluezEndpoint::HandleNewDevice(BluezDevice1 & aDevice)
 {
  VerifyOrReturn(bluez_device1_get_connected(&aDevice));
  VerifyOrReturn(!mIsCentral || bluez_device1_get_services_resolved(&aDevice));
+ CHIP_ERROR err;
 
  const char * objectPath = g_dbus_proxy_get_object_path(reinterpret_cast<GDBusProxy *>(&aDevice));
  BluezConnection * conn = GetBluezConnection(objectPath);
  VerifyOrReturn(conn == nullptr,
  ChipLogError(DeviceLayer, "FAIL: Connection already tracked: conn=%p device=%s path=%s", conn,
  conn->GetPeerAddress(), objectPath));
 
- conn = chip::Platform::New<BluezConnection>(*this, aDevice);
+ conn = chip::Platform::New<BluezConnection>(aDevice);
+ VerifyOrExit(conn != nullptr, err = CHIP_ERROR_NO_MEMORY);
+ SuccessOrExit(err = conn->Init(*this));
+
  mpPeerDevicePath = g_strdup(objectPath);
  mConnMap[mpPeerDevicePath] = conn;
 
  ChipLogDetail(DeviceLayer, "New BLE connection: conn=%p device=%s path=%s", conn, conn->GetPeerAddress(), objectPath);
 
  BLEManagerImpl::HandleNewConnection(conn);
+ return;
+
+exit:
+ chip::Platform::Delete(conn);
+ BLEManagerImpl::HandleConnectFailed(err);
 }
 
 void BluezEndpoint::OnDeviceAdded(BluezDevice1 & device)
@@ -437,7 +446,7 @@ void BluezEndpoint::SetupGattService()
 {
 
  static const char * const c1_flags[] = { "write", nullptr };
- static const char * const c2_flags[] = { "read", "indicate", nullptr };
+ static const char * const c2_flags[] = { "indicate", nullptr };
 #if CHIP_ENABLE_ADDITIONAL_DATA_ADVERTISING
  static const char * const c3_flags[] = { "read", nullptr };
 #endif