[Linux] Fix memory leaks in the BLE module (#28067)
* Do not duplicate string when passing to const argument
* Convert NULL to nullptr
* Do not duplicate string when adding to mpConnMap
* Free buffer used for reading characteristic data
* Clear stack-allocated dict variants
* Fix memory leak when creating unix FD list
* Change new to g_new to be consistent with the rest of the code
* Replace stack-alloc variant dict with heap-alloc
In our case, it's easier to handle memory free for heap-allocated variant dicts.
* Free memory returned by g_variant_dict_lookup_value
diff --git a/src/platform/Linux/bluez/Helper.cpp b/src/platform/Linux/bluez/Helper.cpp
index 6a1966c..d071f3b 100644
--- a/src/platform/Linux/bluez/Helper.cpp
+++ b/src/platform/Linux/bluez/Helper.cpp
@@ -335,15 +335,15 @@
uint8_t * buf;
size_t len;
bool isSuccess = false;
- BluezConnection * conn = NULL;
+ BluezConnection * conn = nullptr;
BluezEndpoint * endpoint = static_cast<BluezEndpoint *>(apEndpoint);
- VerifyOrExit(endpoint != NULL, ChipLogError(DeviceLayer, "endpoint is NULL in %s", __func__));
+ VerifyOrExit(endpoint != nullptr, ChipLogError(DeviceLayer, "endpoint is NULL in %s", __func__));
- VerifyOrExit(aValue != NULL, ChipLogError(DeviceLayer, "aValue is NULL in %s", __func__));
+ VerifyOrExit(aValue != nullptr, ChipLogError(DeviceLayer, "aValue is NULL in %s", __func__));
conn = GetBluezConnectionViaDevice(endpoint);
- VerifyOrExit(conn != NULL,
+ VerifyOrExit(conn != nullptr,
g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.Failed", "No CHIP Bluez connection"));
bluez_gatt_characteristic1_set_value(aChar, g_variant_ref(aValue));
@@ -372,9 +372,8 @@
static gboolean BluezCharacteristicWriteFD(GIOChannel * aChannel, GIOCondition aCond, gpointer apEndpoint)
{
GVariant * newVal;
- gchar * buf;
+ uint8_t * buf = nullptr;
ssize_t len;
- int fd;
bool isSuccess = false;
BluezConnection * conn = static_cast<BluezConnection *>(apEndpoint);
@@ -387,33 +386,29 @@
ChipLogDetail(DeviceLayer, "c1 %s mtu, %d", __func__, conn->mMtu);
- buf = static_cast<gchar *>(g_malloc(conn->mMtu));
- fd = g_io_channel_unix_get_fd(aChannel);
-
- len = read(fd, buf, conn->mMtu);
-
+ buf = g_new(uint8_t, conn->mMtu);
+ len = read(g_io_channel_unix_get_fd(aChannel), buf, conn->mMtu);
VerifyOrExit(len > 0, ChipLogError(DeviceLayer, "FAIL: short read in %s (%zd)", __func__, len));
// Casting len to size_t is safe, since we ensured that it's not negative.
newVal = g_variant_new_fixed_array(G_VARIANT_TYPE_BYTE, buf, static_cast<size_t>(len), sizeof(uint8_t));
bluez_gatt_characteristic1_set_value(conn->mpC1, newVal);
- BLEManagerImpl::HandleRXCharWrite(conn, reinterpret_cast<uint8_t *>(buf), static_cast<size_t>(len));
+ BLEManagerImpl::HandleRXCharWrite(conn, buf, static_cast<size_t>(len));
isSuccess = true;
exit:
+ g_free(buf);
return isSuccess ? G_SOURCE_CONTINUE : G_SOURCE_REMOVE;
}
static void Bluez_gatt_characteristic1_complete_acquire_write_with_fd(GDBusMethodInvocation * invocation, int fd, guint16 mtu)
{
GUnixFDList * fd_list = g_unix_fd_list_new();
- int index;
-
- index = g_unix_fd_list_append(fd_list, fd, nullptr);
-
+ int index = g_unix_fd_list_append(fd_list, fd, nullptr);
g_dbus_method_invocation_return_value_with_unix_fd_list(invocation, g_variant_new("(@hq)", g_variant_new_handle(index), mtu),
fd_list);
+ g_object_unref(fd_list);
}
static gboolean bluezCharacteristicDestroyFD(GIOChannel * aChannel, GIOCondition aCond, gpointer apEndpoint)
@@ -430,9 +425,10 @@
#if CHIP_ERROR_LOGGING
char * errStr;
#endif // CHIP_ERROR_LOGGING
- GVariantDict options;
- bool isSuccess = false;
BluezConnection * conn = nullptr;
+ GVariantDict * options = nullptr;
+ GVariant * option_mtu = nullptr;
+ bool isSuccess = false;
BluezEndpoint * endpoint = static_cast<BluezEndpoint *>(apEndpoint);
VerifyOrExit(endpoint != nullptr, ChipLogError(DeviceLayer, "endpoint is NULL in %s", __func__));
@@ -453,18 +449,12 @@
goto exit;
}
- g_variant_dict_init(&options, aOptions);
- if (g_variant_dict_contains(&options, "mtu") == TRUE)
- {
- GVariant * v = g_variant_dict_lookup_value(&options, "mtu", G_VARIANT_TYPE_UINT16);
- conn->mMtu = g_variant_get_uint16(v);
- }
- else
- {
- ChipLogError(DeviceLayer, "FAIL: no MTU in options in %s", __func__);
- g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.InvalidArguments", "MTU negotiation failed");
- goto exit;
- }
+ options = g_variant_dict_new(aOptions);
+ option_mtu = g_variant_dict_lookup_value(options, "mtu", G_VARIANT_TYPE_UINT16);
+ VerifyOrExit(
+ option_mtu != nullptr, ChipLogError(DeviceLayer, "FAIL: No MTU in options in %s", __func__);
+ g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.InvalidArguments", "MTU negotiation failed"));
+ conn->mMtu = g_variant_get_uint16(option_mtu);
channel = g_io_channel_unix_new(fds[0]);
g_io_channel_set_encoding(channel, nullptr, nullptr);
@@ -484,6 +474,14 @@
isSuccess = true;
exit:
+ if (options != nullptr)
+ {
+ g_variant_dict_unref(options);
+ }
+ if (option_mtu != nullptr)
+ {
+ g_variant_unref(option_mtu);
+ }
return isSuccess ? TRUE : FALSE;
}
@@ -505,28 +503,31 @@
#if CHIP_ERROR_LOGGING
char * errStr;
#endif // CHIP_ERROR_LOGGING
- GVariantDict options;
BluezConnection * conn = nullptr;
+ GVariantDict * options = nullptr;
+ GVariant * option_mtu = nullptr;
bool isSuccess = false;
BluezEndpoint * endpoint = static_cast<BluezEndpoint *>(apEndpoint);
VerifyOrExit(endpoint != nullptr, ChipLogError(DeviceLayer, "endpoint is NULL in %s", __func__));
+ if (bluez_gatt_characteristic1_get_notifying(aChar))
+ {
+ g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.NotPermitted", "Already notifying");
+ goto exit;
+ }
+
conn = GetBluezConnectionViaDevice(endpoint);
VerifyOrExit(conn != nullptr,
g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.Failed", "No Chipoble connection"));
- g_variant_dict_init(&options, aOptions);
- if ((g_variant_dict_contains(&options, "mtu") == TRUE))
- {
- GVariant * v = g_variant_dict_lookup_value(&options, "mtu", G_VARIANT_TYPE_UINT16);
- conn->mMtu = g_variant_get_uint16(v);
- }
+ options = g_variant_dict_new(aOptions);
+ option_mtu = g_variant_dict_lookup_value(options, "mtu", G_VARIANT_TYPE_UINT16);
+ VerifyOrExit(
+ option_mtu != nullptr, ChipLogError(DeviceLayer, "FAIL: No MTU in options in %s", __func__);
+ g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.InvalidArguments", "MTU negotiation failed"));
+ conn->mMtu = g_variant_get_uint16(option_mtu);
- if (bluez_gatt_characteristic1_get_notifying(aChar))
- {
- g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.NotPermitted", "Already notifying");
- }
if (socketpair(AF_UNIX, SOCK_SEQPACKET | SOCK_NONBLOCK | SOCK_CLOEXEC, 0, fds) < 0)
{
#if CHIP_ERROR_LOGGING
@@ -559,6 +560,14 @@
isSuccess = true;
exit:
+ if (options != nullptr)
+ {
+ g_variant_dict_unref(options);
+ }
+ if (option_mtu != nullptr)
+ {
+ g_variant_unref(option_mtu);
+ }
return isSuccess ? TRUE : FALSE;
}
@@ -801,8 +810,8 @@
static BluezGattCharacteristic1 * BluezCharacteristicCreate(BluezGattService1 * aService, const char * aCharName,
const char * aUUID, GDBusObjectManagerServer * aRoot)
{
- char * servicePath = g_strdup(g_dbus_object_get_object_path(g_dbus_interface_get_object(G_DBUS_INTERFACE(aService))));
- char * charPath = g_strdup_printf("%s/%s", servicePath, aCharName);
+ const char * servicePath = g_dbus_object_get_object_path(g_dbus_interface_get_object(G_DBUS_INTERFACE(aService)));
+ char * charPath = g_strdup_printf("%s/%s", servicePath, aCharName);
BluezObjectSkeleton * object;
BluezGattCharacteristic1 * characteristic;
@@ -817,7 +826,6 @@
g_dbus_object_manager_server_export(aRoot, G_DBUS_OBJECT_SKELETON(object));
g_free(charPath);
- g_free(servicePath);
g_object_unref(object);
return characteristic;
@@ -868,10 +876,10 @@
return CHIP_NO_ERROR;
}
-/// Update the table of open BLE connections whevener a new device is spotted or its attributes have changed.
+/// Update the table of open BLE connections whenever a new device is spotted or its attributes have changed.
static void UpdateConnectionTable(BluezDevice1 * apDevice, BluezEndpoint & aEndpoint)
{
- const gchar * objectPath = g_dbus_proxy_get_object_path(G_DBUS_PROXY(apDevice));
+ const char * objectPath = g_dbus_proxy_get_object_path(G_DBUS_PROXY(apDevice));
BluezConnection * connection = static_cast<BluezConnection *>(g_hash_table_lookup(aEndpoint.mpConnMap, objectPath));
if (connection != nullptr && !bluez_device1_get_connected(apDevice))
@@ -901,7 +909,7 @@
aEndpoint.mpPeerDevicePath = g_strdup(objectPath);
g_hash_table_insert(aEndpoint.mpConnMap, aEndpoint.mpPeerDevicePath, connection);
- ChipLogDetail(DeviceLayer, "New BLE connection %p, device %s, path %s", connection, connection->mpPeerAddress,
+ ChipLogDetail(DeviceLayer, "New BLE connection: conn %p, device %s, path %s", connection, connection->mpPeerAddress,
aEndpoint.mpPeerDevicePath);
BLEManagerImpl::HandleNewConnection(connection);
@@ -932,7 +940,7 @@
}
// We need to handle device connection both this function and BluezSignalInterfacePropertiesChanged
- // When a device is connected for first time, this function will be triggerred.
+ // When a device is connected for first time, this function will be triggered.
// The future connections for the same device will trigger ``Connect'' property change.
// TODO: Factor common code in the two function.
BluezConnection * conn;
@@ -950,8 +958,10 @@
conn->mpEndpoint = apEndpoint;
BluezConnectionInit(conn);
apEndpoint->mpPeerDevicePath = g_strdup(g_dbus_proxy_get_object_path(G_DBUS_PROXY(device)));
- ChipLogDetail(DeviceLayer, "Device %s (Path: %s) Connected", conn->mpPeerAddress, apEndpoint->mpPeerDevicePath);
- g_hash_table_insert(apEndpoint->mpConnMap, g_strdup(g_dbus_proxy_get_object_path(G_DBUS_PROXY(device))), conn);
+ g_hash_table_insert(apEndpoint->mpConnMap, apEndpoint->mpPeerDevicePath, conn);
+
+ ChipLogDetail(DeviceLayer, "BLE device connected: conn %p, device %s, path %s", conn, conn->mpPeerAddress,
+ apEndpoint->mpPeerDevicePath);
exit:
return;
@@ -1072,52 +1082,51 @@
static BluezConnection * BluezCharacteristicGetBluezConnection(BluezGattCharacteristic1 * aChar, GVariant * aOptions,
BluezEndpoint * apEndpoint)
{
- BluezConnection * retval = NULL;
- const gchar * path = NULL;
- GVariantDict options;
- GVariant * v;
+ BluezConnection * retval = nullptr;
+ GVariantDict * options = nullptr;
+ GVariant * option_device = nullptr;
- VerifyOrExit(apEndpoint != NULL, ChipLogError(DeviceLayer, "endpoint is NULL in %s", __func__));
+ VerifyOrExit(apEndpoint != nullptr, ChipLogError(DeviceLayer, "endpoint is NULL in %s", __func__));
VerifyOrExit(apEndpoint->mIsCentral, );
/* TODO Unfortunately StartNotify/StopNotify doesn't provide info about
* peer device in call params so we need look this up ourselves.
*/
- if (aOptions == NULL)
+ if (aOptions == nullptr)
{
GList * objects;
GList * l;
GList * ll;
objects = g_dbus_object_manager_get_objects(apEndpoint->mpObjMgr);
- for (l = objects; l != NULL; l = l->next)
+ for (l = objects; l != nullptr; l = l->next)
{
BluezDevice1 * device = bluez_object_get_device1(BLUEZ_OBJECT(l->data));
- if (device != NULL)
+ if (device != nullptr)
{
if (BluezIsDeviceOnAdapter(device, apEndpoint->mpAdapter))
{
- for (ll = objects; ll != NULL; ll = ll->next)
+ for (ll = objects; ll != nullptr; ll = ll->next)
{
BluezGattService1 * service = bluez_object_get_gatt_service1(BLUEZ_OBJECT(ll->data));
- if (service != NULL)
+ if (service != nullptr)
{
if (BluezIsServiceOnDevice(service, device))
{
if (BluezIsCharOnService(aChar, service))
{
- retval = (BluezConnection *) g_hash_table_lookup(
- apEndpoint->mpConnMap, g_dbus_proxy_get_object_path(G_DBUS_PROXY(device)));
+ retval = static_cast<BluezConnection *>(g_hash_table_lookup(
+ apEndpoint->mpConnMap, g_dbus_proxy_get_object_path(G_DBUS_PROXY(device))));
}
}
g_object_unref(service);
- if (retval != NULL)
+ if (retval != nullptr)
break;
}
}
}
g_object_unref(device);
- if (retval != NULL)
+ if (retval != nullptr)
break;
}
}
@@ -1126,18 +1135,23 @@
}
else
{
- g_variant_dict_init(&options, aOptions);
+ options = g_variant_dict_new(aOptions);
+ option_device = g_variant_dict_lookup_value(options, "device", G_VARIANT_TYPE_OBJECT_PATH);
+ VerifyOrExit(option_device != nullptr, ChipLogError(DeviceLayer, "FAIL: No device in options in %s", __func__););
- v = g_variant_dict_lookup_value(&options, "device", G_VARIANT_TYPE_OBJECT_PATH);
-
- VerifyOrExit(v != NULL, ChipLogError(DeviceLayer, "FAIL: No device option in dictionary (%s)", __func__));
-
- path = g_variant_get_string(v, NULL);
-
- retval = (BluezConnection *) g_hash_table_lookup(apEndpoint->mpConnMap, path);
+ retval = static_cast<BluezConnection *>(
+ g_hash_table_lookup(apEndpoint->mpConnMap, g_variant_get_string(option_device, nullptr)));
}
exit:
+ if (options != nullptr)
+ {
+ g_variant_dict_unref(options);
+ }
+ if (option_device != nullptr)
+ {
+ g_variant_unref(option_device);
+ }
return retval;
}
#endif // CHIP_BLUEZ_CENTRAL_SUPPORT
@@ -1260,24 +1274,22 @@
endpoint->mpService = BluezServiceCreate(apClosure);
// C1 characteristic
- endpoint->mpC1 =
- BluezCharacteristicCreate(endpoint->mpService, g_strdup("c1"), g_strdup(CHIP_PLAT_BLE_UUID_C1_STRING), endpoint->mpRoot);
+ endpoint->mpC1 = BluezCharacteristicCreate(endpoint->mpService, "c1", CHIP_PLAT_BLE_UUID_C1_STRING, endpoint->mpRoot);
bluez_gatt_characteristic1_set_flags(endpoint->mpC1, c1_flags);
g_signal_connect(endpoint->mpC1, "handle-read-value", G_CALLBACK(BluezCharacteristicReadValue), apClosure);
- g_signal_connect(endpoint->mpC1, "handle-write-value", G_CALLBACK(BluezCharacteristicWriteValueError), NULL);
+ g_signal_connect(endpoint->mpC1, "handle-write-value", G_CALLBACK(BluezCharacteristicWriteValueError), nullptr);
g_signal_connect(endpoint->mpC1, "handle-acquire-write", G_CALLBACK(BluezCharacteristicAcquireWrite), apClosure);
- g_signal_connect(endpoint->mpC1, "handle-acquire-notify", G_CALLBACK(BluezCharacteristicAcquireNotifyError), NULL);
- g_signal_connect(endpoint->mpC1, "handle-start-notify", G_CALLBACK(BluezCharacteristicStartNotifyError), NULL);
- g_signal_connect(endpoint->mpC1, "handle-stop-notify", G_CALLBACK(BluezCharacteristicStopNotifyError), NULL);
- g_signal_connect(endpoint->mpC1, "handle-confirm", G_CALLBACK(BluezCharacteristicConfirmError), NULL);
+ g_signal_connect(endpoint->mpC1, "handle-acquire-notify", G_CALLBACK(BluezCharacteristicAcquireNotifyError), nullptr);
+ g_signal_connect(endpoint->mpC1, "handle-start-notify", G_CALLBACK(BluezCharacteristicStartNotifyError), nullptr);
+ g_signal_connect(endpoint->mpC1, "handle-stop-notify", G_CALLBACK(BluezCharacteristicStopNotifyError), nullptr);
+ g_signal_connect(endpoint->mpC1, "handle-confirm", G_CALLBACK(BluezCharacteristicConfirmError), nullptr);
- endpoint->mpC2 =
- BluezCharacteristicCreate(endpoint->mpService, g_strdup("c2"), g_strdup(CHIP_PLAT_BLE_UUID_C2_STRING), endpoint->mpRoot);
+ endpoint->mpC2 = BluezCharacteristicCreate(endpoint->mpService, "c2", CHIP_PLAT_BLE_UUID_C2_STRING, endpoint->mpRoot);
bluez_gatt_characteristic1_set_flags(endpoint->mpC2, c2_flags);
g_signal_connect(endpoint->mpC2, "handle-read-value", G_CALLBACK(BluezCharacteristicReadValue), apClosure);
- g_signal_connect(endpoint->mpC2, "handle-write-value", G_CALLBACK(BluezCharacteristicWriteValueError), NULL);
- g_signal_connect(endpoint->mpC2, "handle-acquire-write", G_CALLBACK(BluezCharacteristicAcquireWriteError), NULL);
+ g_signal_connect(endpoint->mpC2, "handle-write-value", G_CALLBACK(BluezCharacteristicWriteValueError), nullptr);
+ g_signal_connect(endpoint->mpC2, "handle-acquire-write", G_CALLBACK(BluezCharacteristicAcquireWriteError), nullptr);
g_signal_connect(endpoint->mpC2, "handle-acquire-notify", G_CALLBACK(BluezCharacteristicAcquireNotify), apClosure);
g_signal_connect(endpoint->mpC2, "handle-start-notify", G_CALLBACK(BluezCharacteristicStartNotify), apClosure);
g_signal_connect(endpoint->mpC2, "handle-stop-notify", G_CALLBACK(BluezCharacteristicStopNotify), apClosure);
@@ -1289,12 +1301,11 @@
#if CHIP_ENABLE_ADDITIONAL_DATA_ADVERTISING
ChipLogDetail(DeviceLayer, "CHIP_ENABLE_ADDITIONAL_DATA_ADVERTISING is TRUE");
// Additional data characteristics
- endpoint->mpC3 =
- BluezCharacteristicCreate(endpoint->mpService, g_strdup("c3"), g_strdup(CHIP_PLAT_BLE_UUID_C3_STRING), endpoint->mpRoot);
+ endpoint->mpC3 = BluezCharacteristicCreate(endpoint->mpService, "c3", CHIP_PLAT_BLE_UUID_C3_STRING, endpoint->mpRoot);
bluez_gatt_characteristic1_set_flags(endpoint->mpC3, c3_flags);
g_signal_connect(endpoint->mpC3, "handle-read-value", G_CALLBACK(BluezCharacteristicReadValue), apClosure);
- g_signal_connect(endpoint->mpC3, "handle-write-value", G_CALLBACK(BluezCharacteristicWriteValueError), NULL);
- g_signal_connect(endpoint->mpC3, "handle-acquire-write", G_CALLBACK(BluezCharacteristicAcquireWriteError), NULL);
+ g_signal_connect(endpoint->mpC3, "handle-write-value", G_CALLBACK(BluezCharacteristicWriteValueError), nullptr);
+ g_signal_connect(endpoint->mpC3, "handle-acquire-write", G_CALLBACK(BluezCharacteristicAcquireWriteError), nullptr);
g_signal_connect(endpoint->mpC3, "handle-acquire-notify", G_CALLBACK(BluezCharacteristicAcquireNotify), apClosure);
g_signal_connect(endpoint->mpC3, "handle-start-notify", G_CALLBACK(BluezCharacteristicStartNotify), apClosure);
g_signal_connect(endpoint->mpC3, "handle-stop-notify", G_CALLBACK(BluezCharacteristicStopNotify), apClosure);
