[PATCH BlueZ 0/3] More attribute allocation fixes

[PATCH BlueZ 0/3] More attribute allocation fixes

[Anderson Lizardo]

Hi,

Since the commit "Fix allocation of attribute values" I submitted last week
(which was authored by Vinicius), we found a few other issues related to
attribute allocation (thanks to Hendrik Sattler for the review of that commit,
BTW). This series fixes those issues. The last commit replaces a few memcpy()
operations with assignments, as suggested by Hendrik.

The allocation fixes have been tested with valgrind to make sure there are no
obvious leaks.

Best Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

[PATCH BlueZ 1/3] Remove empty line before declaration

[Anderson Lizardo]

---
 attrib/att.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/attrib/att.c b/attrib/att.c
index 1ce0f7b..1b95c45 100644
--- a/attrib/att.c
+++ b/attrib/att.c
@@ -857,7 +857,6 @@ uint16_t enc_indication(struct attribute *a, uint8_t *pdu, int len)
 struct attribute *dec_indication(const uint8_t *pdu, int len)
 {
 	const uint16_t min_len = sizeof(pdu[0]) + sizeof(uint16_t);
-
 	struct attribute *a;
 
 	if (pdu == NULL)
-- 
1.7.0.4

[PATCH BlueZ 2/3] Fix memory allocation of struct attribute

[Anderson Lizardo]

On commit 6a6da5de107e192f62ce2ecdde96eae985181df0 the struct
attribute "data[0]" member was replaced with a dynamically allocated
"data" pointer. This commit fixes remaining places where the old
allocation scheme was still assumed.
---
 attrib/att.c        |    7 +++----
 src/attrib-server.c |   33 +++++++++++++++------------------
 2 files changed, 18 insertions(+), 22 deletions(-)

diff --git a/attrib/att.c b/attrib/att.c
index 1b95c45..c3cbf86 100644
--- a/attrib/att.c
+++ b/attrib/att.c
@@ -868,11 +868,10 @@ struct attribute *dec_indication(const uint8_t *pdu, int len)
 	if (len < min_len)
 		return NULL;
 
-	a = g_malloc0(sizeof(struct attribute) + len - min_len);
-	a->len = len - min_len;
-
+	a = g_new0(struct attribute, 1);
 	a->handle = att_get_u16(&pdu[1]);
-	memcpy(a->data, &pdu[3], a->len);
+	a->len = len - min_len;
+	a->data = g_memdup(&pdu[3], a->len);
 
 	return a;
 }
diff --git a/src/attrib-server.c b/src/attrib-server.c
index 5c86085..2e99a52 100644
--- a/src/attrib-server.c
+++ b/src/attrib-server.c
@@ -271,10 +271,9 @@ static struct attribute *client_cfg_attribute(struct gatt_channel *channel,
 		/* Create a private copy of the Client Characteristic
 		 * Configuration attribute */
 		a = g_new0(struct attribute, 1);
-		a->data = g_malloc0(vlen);
-
-		memcpy(a, orig_attr, sizeof(*a));
-		memcpy(a->data, value, vlen);
+		*a = *orig_attr;
+		a->len = vlen;
+		a->data = g_memdup(value, vlen);
 		a->write_cb = client_set_notifications;
 		a->cb_user_data = channel;
 
@@ -776,6 +775,14 @@ static uint16_t mtu_exchange(struct gatt_channel *channel, uint16_t mtu,
 	return enc_mtu_resp(old_mtu, pdu, len);
 }
 
+static void attrib_free(void *data)
+{
+	struct attribute *a = data;
+
+	g_free(a->data);
+	g_free(a);
+}
+
 static void channel_disconnect(void *user_data)
 {
 	struct gatt_channel *channel = user_data;
@@ -785,7 +792,7 @@ static void channel_disconnect(void *user_data)
 
 	g_slist_free(channel->notify);
 	g_slist_free(channel->indicate);
-	g_slist_free_full(channel->configs, g_free);
+	g_slist_free_full(channel->configs, attrib_free);
 
 	g_free(channel);
 }
@@ -1123,14 +1130,6 @@ failed:
 	return -1;
 }
 
-static void attrib_free(void *data)
-{
-	struct attribute *a = data;
-
-	g_free(a->data);
-	g_free(a);
-}
-
 void attrib_server_exit(void)
 {
 	GSList *l;
@@ -1152,7 +1151,7 @@ void attrib_server_exit(void)
 
 		g_slist_free(channel->notify);
 		g_slist_free(channel->indicate);
-		g_slist_free_full(channel->configs, g_free);
+		g_slist_free_full(channel->configs, attrib_free);
 
 		g_attrib_unref(channel->attrib);
 		g_free(channel);
@@ -1253,14 +1252,12 @@ struct attribute *attrib_db_add(uint16_t handle, bt_uuid_t *uuid, int read_reqs,
 		return NULL;
 
 	a = g_new0(struct attribute, 1);
-	a->data = g_malloc0(len);
-
+	a->len = len;
+	a->data = g_memdup(value, len);
 	a->handle = handle;
 	memcpy(&a->uuid, uuid, sizeof(bt_uuid_t));
 	a->read_reqs = read_reqs;
 	a->write_reqs = write_reqs;
-	a->len = len;
-	memcpy(a->data, value, len);
 
 	database = g_slist_insert_sorted(database, a, attribute_cmp);
 
-- 
1.7.0.4

[PATCH BlueZ 3/3] Refactor value assignments of bt_uuid_t variables

[Anderson Lizardo]

Prior to this commit, the assignments were made with memcpy(). This can
be unsafe and less readable, therefore it was replaced with code like:

<dst> = *src;

This also allows more compiler safety checks.
---
 attrib/gatt.c       |    2 +-
 lib/uuid.c          |    2 +-
 src/attrib-server.c |    4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/attrib/gatt.c b/attrib/gatt.c
index 77c96f3..a62f348 100644
--- a/attrib/gatt.c
+++ b/attrib/gatt.c
@@ -241,7 +241,7 @@ guint gatt_discover_primary(GAttrib *attrib, bt_uuid_t *uuid, gatt_cb_t func,
 	dp->user_data = user_data;
 
 	if (uuid) {
-		memcpy(&dp->uuid, uuid, sizeof(bt_uuid_t));
+		dp->uuid = *uuid;
 		cb = primary_by_uuid_cb;
 	} else
 		cb = primary_all_cb;
diff --git a/lib/uuid.c b/lib/uuid.c
index 325016a..a3e2a1a 100644
--- a/lib/uuid.c
+++ b/lib/uuid.c
@@ -74,7 +74,7 @@ void bt_uuid_to_uuid128(const bt_uuid_t *src, bt_uuid_t *dst)
 {
 	switch (src->type) {
 	case BT_UUID128:
-		memcpy(dst, src, sizeof(bt_uuid_t));
+		*dst = *src;
 		break;
 	case BT_UUID32:
 		bt_uuid32_to_uuid128(src, dst);
diff --git a/src/attrib-server.c b/src/attrib-server.c
index 2e99a52..59dddf6 100644
--- a/src/attrib-server.c
+++ b/src/attrib-server.c
@@ -1255,7 +1255,7 @@ struct attribute *attrib_db_add(uint16_t handle, bt_uuid_t *uuid, int read_reqs,
 	a->len = len;
 	a->data = g_memdup(value, len);
 	a->handle = handle;
-	memcpy(&a->uuid, uuid, sizeof(bt_uuid_t));
+	a->uuid = *uuid;
 	a->read_reqs = read_reqs;
 	a->write_reqs = write_reqs;
 
@@ -1287,7 +1287,7 @@ int attrib_db_update(uint16_t handle, bt_uuid_t *uuid, const uint8_t *value,
 	memcpy(a->data, value, len);
 
 	if (uuid != NULL)
-		memcpy(&a->uuid, uuid, sizeof(bt_uuid_t));
+		a->uuid = *uuid;
 
 	if (attr)
 		*attr = a;
-- 
1.7.0.4

Re: [PATCH BlueZ 0/3] More attribute allocation fixes

[Johan Hedberg]

Hi Lizardo,

On Tue, Sep 20, 2011, Anderson Lizardo wrote:
> Since the commit "Fix allocation of attribute values" I submitted last week
> (which was authored by Vinicius), we found a few other issues related to
> attribute allocation (thanks to Hendrik Sattler for the review of that commit,
> BTW). This series fixes those issues. The last commit replaces a few memcpy()
> operations with assignments, as suggested by Hendrik.
> 
> The allocation fixes have been tested with valgrind to make sure there are no
> obvious leaks.

All three patches have been applied. Thanks.

Johan