[Qemu-devel] [PATCH] hw/bt/sdp: Fix resource leak detect by coverity

[Qemu-devel] [PATCH] hw/bt/sdp: Fix resource leak detect by coverity

[Shannon Zhao]

Free data in function sdp_attr_write after use.

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 hw/bt/sdp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/bt/sdp.c b/hw/bt/sdp.c
index 218e075..8e6d5e3 100644
--- a/hw/bt/sdp.c
+++ b/hw/bt/sdp.c
@@ -735,6 +735,7 @@ static void sdp_service_record_build(struct sdp_service_record_s *record,
         record->attribute_list[record->attributes ++].len = len;
         data += len;
     }
+    g_free(data);
 
     /* Sort the attribute list by the AttributeID */
     qsort(record->attribute_list, record->attributes,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] hw/bt/sdp: Fix resource leak detect by coverity

[Stefan Weil]

Am 14.03.2015 um 04:42 schrieb Shannon Zhao:
> Free data in function sdp_attr_write after use.
>
> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>   hw/bt/sdp.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/hw/bt/sdp.c b/hw/bt/sdp.c
> index 218e075..8e6d5e3 100644
> --- a/hw/bt/sdp.c
> +++ b/hw/bt/sdp.c
> @@ -735,6 +735,7 @@ static void sdp_service_record_build(struct sdp_service_record_s *record,
>           record->attribute_list[record->attributes ++].len = len;
>           data += len;
>       }
> +    g_free(data);

No, here more work is needed. data is no longer the original data,
because two lines above it is modified.

>   
>       /* Sort the attribute list by the AttributeID */
>       qsort(record->attribute_list, record->attributes,

Re: [Qemu-devel] [PATCH] hw/bt/sdp: Fix resource leak detect by coverity

[Shannon Zhao]

On 2015/3/14 14:57, Stefan Weil wrote:
> Am 14.03.2015 um 04:42 schrieb Shannon Zhao:
>> Free data in function sdp_attr_write after use.
>>
>> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> ---
>>   hw/bt/sdp.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/bt/sdp.c b/hw/bt/sdp.c
>> index 218e075..8e6d5e3 100644
>> --- a/hw/bt/sdp.c
>> +++ b/hw/bt/sdp.c
>> @@ -735,6 +735,7 @@ static void sdp_service_record_build(struct sdp_service_record_s *record,
>>           record->attribute_list[record->attributes ++].len = len;
>>           data += len;
>>       }
>> +    g_free(data);
> 
> No, here more work is needed. data is no longer the original data,
> because two lines above it is modified.

Thanks for pointing out.

> 
>>         /* Sort the attribute list by the AttributeID */
>>       qsort(record->attribute_list, record->attributes,
> 
> 
> .
> 

[Qemu-devel] [PATCH] hw/bt/sdp: Fix resource leak detect by coverity

[Shannon Zhao]

Free data in function sdp_attr_write after use.

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
    For minimum modification, just add a variable to record the data.
---
 hw/bt/sdp.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/bt/sdp.c b/hw/bt/sdp.c
index 218e075..8be0d14 100644
--- a/hw/bt/sdp.c
+++ b/hw/bt/sdp.c
@@ -698,7 +698,7 @@ static void sdp_service_record_build(struct sdp_service_record_s *record,
                 struct sdp_def_service_s *def, int handle)
 {
     int len = 0;
-    uint8_t *data;
+    uint8_t *data, *pt;
     int *uuid;
 
     record->uuids = 0;
@@ -712,7 +712,7 @@ static void sdp_service_record_build(struct sdp_service_record_s *record,
             g_malloc0(record->attributes * sizeof(*record->attribute_list));
     record->uuid =
             g_malloc0(record->uuids * sizeof(*record->uuid));
-    data = g_malloc(len);
+    pt = data = g_malloc(len);
 
     record->attributes = 0;
     uuid = record->uuid;
@@ -735,6 +735,7 @@ static void sdp_service_record_build(struct sdp_service_record_s *record,
         record->attribute_list[record->attributes ++].len = len;
         data += len;
     }
+    g_free(pt);
 
     /* Sort the attribute list by the AttributeID */
     qsort(record->attribute_list, record->attributes,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] hw/bt/sdp: Fix resource leak detect by coverity

[Stefan Weil]

Am 14.03.2015 um 10:47 schrieb Shannon Zhao:
> Free data in function sdp_attr_write after use.
>
> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>      For minimum modification, just add a variable to record the data.
> ---
>   hw/bt/sdp.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/bt/sdp.c b/hw/bt/sdp.c
> index 218e075..8be0d14 100644
> --- a/hw/bt/sdp.c
> +++ b/hw/bt/sdp.c
> @@ -698,7 +698,7 @@ static void sdp_service_record_build(struct sdp_service_record_s *record,
>                   struct sdp_def_service_s *def, int handle)
>   {
>       int len = 0;
> -    uint8_t *data;
> +    uint8_t *data, *pt;
>       int *uuid;
>   
>       record->uuids = 0;
> @@ -712,7 +712,7 @@ static void sdp_service_record_build(struct sdp_service_record_s *record,
>               g_malloc0(record->attributes * sizeof(*record->attribute_list));
>       record->uuid =
>               g_malloc0(record->uuids * sizeof(*record->uuid));
> -    data = g_malloc(len);
> +    pt = data = g_malloc(len);
>   
>       record->attributes = 0;
>       uuid = record->uuid;
> @@ -735,6 +735,7 @@ static void sdp_service_record_build(struct sdp_service_record_s *record,
>           record->attribute_list[record->attributes ++].len = len;
>           data += len;
>       }
> +    g_free(pt);
>   
>       /* Sort the attribute list by the AttributeID */
>       qsort(record->attribute_list, record->attributes,

This fixes the memory leak, but I still don't understand what is done here.
data is allocated, then filled with values, now it is also deallocated. 
But I'm
missing the part where all those data is used.

Stefan

Re: [Qemu-devel] [PATCH] hw/bt/sdp: Fix resource leak detect by coverity

[Paolo Bonzini]

On 14/03/2015 11:07, Stefan Weil wrote:
> 
> This fixes the memory leak, but I still don't understand what is done here.
> data is allocated, then filled with values, now it is also deallocated.
> But I'm missing the part where all those data is used.

"data" escapes in record->attribute_list[record->attributes].pair.

The bug is in bt_l2cap_sdp_close_ch which does an invalid free every
time it frees the first sdp->service_list[i].attribute_list->pair (but
the qsort could have moved it elsewhere in the list).  The right fix is
to do a separate malloc for each attribute, instead of a single one.

In any case, it seems simpler to just leave this code aside.

Paolo

Re: [Qemu-devel] [PATCH] hw/bt/sdp: Fix resource leak detect by coverity

[Michael Tokarev]

15.03.2015 12:21, Paolo Bonzini wrote:
> On 14/03/2015 11:07, Stefan Weil wrote:
>>
>> This fixes the memory leak, but I still don't understand what is done here.
>> data is allocated, then filled with values, now it is also deallocated.
>> But I'm missing the part where all those data is used.
> 
> "data" escapes in record->attribute_list[record->attributes].pair.
> 
> The bug is in bt_l2cap_sdp_close_ch which does an invalid free every
> time it frees the first sdp->service_list[i].attribute_list->pair (but
> the qsort could have moved it elsewhere in the list).  The right fix is
> to do a separate malloc for each attribute, instead of a single one.

Or, alternatively, to keep this `data' pointer in sdp to use it in
bt_l2cap_sdp_close_ch().

> In any case, it seems simpler to just leave this code aside.

How many times this code is called?

We have many many places in qemu where resources are allocated once
at startup and never freed just because there's no need to.

Thanks,

/mjt

Re: [Qemu-devel] [PATCH] hw/bt/sdp: Fix resource leak detect by coverity

[Paolo Bonzini]

On 15/03/2015 11:23, Michael Tokarev wrote:
> Or, alternatively, to keep this `data' pointer in sdp to use it in
> bt_l2cap_sdp_close_ch().

Yes.

>> > In any case, it seems simpler to just leave this code aside.
> How many times this code is called?
> 
> We have many many places in qemu where resources are allocated once
> at startup and never freed just because there's no need to.

Well, in this case the bug in bt_l2cap_sdp_close_ch is much worse than a
resource leak.  But bluetooth is not the utmost priority in QEMU
development...

Paolo

Re: [Qemu-devel] [PATCH] hw/bt/sdp: Fix resource leak detect by coverity

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 15/03/2015 11:23, Michael Tokarev wrote:
>> Or, alternatively, to keep this `data' pointer in sdp to use it in
>> bt_l2cap_sdp_close_ch().
>
> Yes.
>
>>> > In any case, it seems simpler to just leave this code aside.
>> How many times this code is called?
>> 
>> We have many many places in qemu where resources are allocated once
>> at startup and never freed just because there's no need to.
>
> Well, in this case the bug in bt_l2cap_sdp_close_ch is much worse than a
> resource leak.  But bluetooth is not the utmost priority in QEMU
> development...

To put it more bluntly: it's rotting in peace.

Occasional drive-by fixes won't stop the rot, a dedicated maintener
could.

Re: [Qemu-devel] [PATCH] hw/bt/sdp: Fix resource leak detect by coverity

[Paolo Bonzini]

On 16/03/2015 08:29, Markus Armbruster wrote:
> > Well, in this case the bug in bt_l2cap_sdp_close_ch is much worse than a
> > resource leak.  But bluetooth is not the utmost priority in QEMU
> > development...
> 
> To put it more bluntly: it's rotting in peace.
> 
> Occasional drive-by fixes won't stop the rot, a dedicated maintener
> could.

I disagree.  The code is not that good, but apparently it works.
Samsung folks are using it and presented their work at KVM Forum 2014.

Paolo