Hi Hendrik,
On Fri, Sep 16, 2011 at 4:35 AM, Hendrik Sattler
<post@xxxxxxxxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
