From: Vinicius Costa Gomes <[email protected]>
Now that pointers to attribute are passed as reference to functions
we cannot have they change during run time, what g_try_realloc()
could do.
---
attrib/att.h | 2 +-
src/attrib-server.c | 33 ++++++++++++++++++++++++---------
2 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/attrib/att.h b/attrib/att.h
index 3b8c098..9db8065 100644
--- a/attrib/att.h
+++ b/attrib/att.h
@@ -130,7 +130,7 @@ struct attribute {
uint8_t (*write_cb)(struct attribute *a, gpointer user_data);
gpointer cb_user_data;
int len;
- uint8_t data[0];
+ uint8_t *data;
};
struct att_data_list {
diff --git a/src/attrib-server.c b/src/attrib-server.c
index 0bda1e3..5c86085 100644
--- a/src/attrib-server.c
+++ b/src/attrib-server.c
@@ -270,7 +270,9 @@ static struct attribute *client_cfg_attribute(struct gatt_channel *channel,
/* Create a private copy of the Client Characteristic
* Configuration attribute */
- a = g_malloc0(sizeof(*a) + vlen);
+ a = g_new0(struct attribute, 1);
+ a->data = g_malloc0(vlen);
+
memcpy(a, orig_attr, sizeof(*a));
memcpy(a->data, value, vlen);
a->write_cb = client_set_notifications;
@@ -1121,11 +1123,19 @@ 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;
- g_slist_free_full(database, g_free);
+ g_slist_free_full(database, attrib_free);
if (l2cap_io) {
g_io_channel_unref(l2cap_io);
@@ -1242,7 +1252,9 @@ struct attribute *attrib_db_add(uint16_t handle, bt_uuid_t *uuid, int read_reqs,
if (g_slist_find_custom(database, GUINT_TO_POINTER(h), handle_cmp))
return NULL;
- a = g_malloc0(sizeof(struct attribute) + len);
+ a = g_new0(struct attribute, 1);
+ a->data = g_malloc0(len);
+
a->handle = handle;
memcpy(&a->uuid, uuid, sizeof(bt_uuid_t));
a->read_reqs = read_reqs;
@@ -1268,21 +1280,23 @@ int attrib_db_update(uint16_t handle, bt_uuid_t *uuid, const uint8_t *value,
if (!l)
return -ENOENT;
- a = g_try_realloc(l->data, sizeof(struct attribute) + len);
- if (a == NULL)
+ a = l->data;
+
+ a->data = g_try_realloc(a->data, len);
+ if (a->data == NULL)
return -ENOMEM;
- l->data = a;
- if (uuid != NULL)
- memcpy(&a->uuid, uuid, sizeof(bt_uuid_t));
a->len = len;
memcpy(a->data, value, len);
- attrib_notify_clients(a);
+ if (uuid != NULL)
+ memcpy(&a->uuid, uuid, sizeof(bt_uuid_t));
if (attr)
*attr = a;
+ attrib_notify_clients(a);
+
return 0;
}
@@ -1300,6 +1314,7 @@ int attrib_db_del(uint16_t handle)
a = l->data;
database = g_slist_remove(database, a);
+ g_free(a->data);
g_free(a);
return 0;
--
1.7.0.4
Hi Hendrik,
On Fri, Sep 16, 2011 at 7:05 AM, Anderson Lizardo
<[email protected]> wrote:
>>> ? ? ? ? ? ? ? ?memcpy(a->data, value, vlen);
>>
>> And here you do...hmm...hard to tell since the pointer now shows to...where?
>> Even if that pointer is valid and is assigned the right size memory, you
>> just leaked memory.
After some thought (and thanks to Johan over IRC), I now see which
issue you are referring to. I'm reviewing all other places where this
bug might happen as well. Unfortunately, looks like it slipped through
manual review and valgrind altogether :(
Thanks,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
Hi Hendrik,
On Fri, Sep 16, 2011 at 4:35 AM, Hendrik Sattler
<[email protected]> wrote:
>> ? ? ? ? ? ? ? ?/* Create a private copy of the Client Characteristic
>> ? ? ? ? ? ? ? ? * Configuration attribute */
>> - ? ? ? ? ? ? ? a = g_malloc0(sizeof(*a) + vlen);
>> + ? ? ? ? ? ? ? a = g_new0(struct attribute, 1);
>> + ? ? ? ? ? ? ? a->data = g_malloc0(vlen);
>
> Here you assign the pointer...
>
>> +
>> ? ? ? ? ? ? ? ?memcpy(a, orig_attr, sizeof(*a));
>
> ...and here your overwrite it.
> Hint: if that is written as
> ? *a = *orig_attr;
> you get a compile-time type check for free.
You are right, this is simpler and safer, I'll change it.
>
>> ? ? ? ? ? ? ? ?memcpy(a->data, value, vlen);
>
> And here you do...hmm...hard to tell since the pointer now shows to...where?
> Even if that pointer is valid and is assigned the right size memory, you
> just leaked memory.
This is the main point of this patch. a->data used to be allocated
together with struct attribute, because of this:
struct attribute {
...
uint8_t data[0];
};
This has now been changed on this commit so that the data field is
just a pointer to a separately allocated buffer:
struct attribute {
...
uint8_t *data;
};
In this new version, the "data" field is a (initially NULL) pointer
that is allocated along with its container (struct attribute). But you
reminded me that I need to optimize this:
a->data = g_malloc0(vlen);
...
memcpy(a->data, value, vlen);
Into this:
a->data = g_memdup(value, vlen);
(much more readable). Plain assignment can't be used here because
a->data is a dynamic array whose size is only known at run-time.
>
>> @@ -1242,7 +1252,9 @@ struct attribute *attrib_db_add(uint16_t handle,
>> bt_uuid_t *uuid, int read_reqs,
>> ? ? ? ?if (g_slist_find_custom(database, GUINT_TO_POINTER(h), handle_cmp))
>> ? ? ? ? ? ? ? ?return NULL;
>>
>> - ? ? ? a = g_malloc0(sizeof(struct attribute) + len);
>> + ? ? ? a = g_new0(struct attribute, 1);
>> + ? ? ? a->data = g_malloc0(len);
>> +
>> ? ? ? ?a->handle = handle;
>> ? ? ? ?memcpy(&a->uuid, uuid, sizeof(bt_uuid_t));
>
> This could also use assignement-instead-of-memcpy but that's out-of-scope of
> this patch?
Correct, but we can fix this in a separate patch as it has a few other
places on BlueZ code to be fixed as well.
>> - ? ? ? attrib_notify_clients(a);
>> + ? ? ? if (uuid != NULL)
>> + ? ? ? ? ? ? ? memcpy(&a->uuid, uuid, sizeof(bt_uuid_t));
>
> But here you could write: a->uuid = *uuid;
Yes, will change.
Thanks,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
Zitat von Anderson Lizardo <[email protected]>:
> From: Vinicius Costa Gomes <[email protected]>
>
> Now that pointers to attribute are passed as reference to functions
> we cannot have they change during run time, what g_try_realloc()
> could do.
> ---
> attrib/att.h | 2 +-
> src/attrib-server.c | 33 ++++++++++++++++++++++++---------
> 2 files changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/attrib/att.h b/attrib/att.h
> index 3b8c098..9db8065 100644
> --- a/attrib/att.h
> +++ b/attrib/att.h
> @@ -130,7 +130,7 @@ struct attribute {
> uint8_t (*write_cb)(struct attribute *a, gpointer user_data);
> gpointer cb_user_data;
> int len;
> - uint8_t data[0];
> + uint8_t *data;
> };
>
> struct att_data_list {
> diff --git a/src/attrib-server.c b/src/attrib-server.c
> index 0bda1e3..5c86085 100644
> --- a/src/attrib-server.c
> +++ b/src/attrib-server.c
> @@ -270,7 +270,9 @@ static struct attribute
> *client_cfg_attribute(struct gatt_channel *channel,
>
> /* Create a private copy of the Client Characteristic
> * Configuration attribute */
> - a = g_malloc0(sizeof(*a) + vlen);
> + a = g_new0(struct attribute, 1);
> + a->data = g_malloc0(vlen);
Here you assign the pointer...
> +
> memcpy(a, orig_attr, sizeof(*a));
...and here your overwrite it.
Hint: if that is written as
*a = *orig_attr;
you get a compile-time type check for free.
> memcpy(a->data, value, vlen);
And here you do...hmm...hard to tell since the pointer now shows to...where?
Even if that pointer is valid and is assigned the right size memory,
you just leaked memory.
> @@ -1242,7 +1252,9 @@ struct attribute *attrib_db_add(uint16_t
> handle, bt_uuid_t *uuid, int read_reqs,
> if (g_slist_find_custom(database, GUINT_TO_POINTER(h), handle_cmp))
> return NULL;
>
> - a = g_malloc0(sizeof(struct attribute) + len);
> + a = g_new0(struct attribute, 1);
> + a->data = g_malloc0(len);
> +
> a->handle = handle;
> memcpy(&a->uuid, uuid, sizeof(bt_uuid_t));
This could also use assignement-instead-of-memcpy but that's
out-of-scope of this patch?
> a->read_reqs = read_reqs;
> @@ -1268,21 +1280,23 @@ int attrib_db_update(uint16_t handle,
> bt_uuid_t *uuid, const uint8_t *value,
> if (!l)
> return -ENOENT;
>
> - a = g_try_realloc(l->data, sizeof(struct attribute) + len);
> - if (a == NULL)
> + a = l->data;
> +
> + a->data = g_try_realloc(a->data, len);
> + if (a->data == NULL)
> return -ENOMEM;
>
> - l->data = a;
> - if (uuid != NULL)
> - memcpy(&a->uuid, uuid, sizeof(bt_uuid_t));
> a->len = len;
> memcpy(a->data, value, len);
>
> - attrib_notify_clients(a);
> + if (uuid != NULL)
> + memcpy(&a->uuid, uuid, sizeof(bt_uuid_t));
But here you could write: a->uuid = *uuid;
HS