diff mbox

[PULL,09/17] bt: stop the sdp memory allocation craziness

Message ID 1501604245-33460-10-git-send-email-pbonzini@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini Aug. 1, 2017, 4:17 p.m. UTC 
Clang static analyzer reports a memory leak.  Actually, the allocated
memory escapes here:

        record->attribute_list[record->attributes].pair = data;

but clang is correct that the memory might leak if len is zero.  We
know it isn't; assert that it is the case.

The craziness doesn't end there.  The memory is freed by
bt_l2cap_sdp_close_ch:

       g_free(sdp->service_list[i].attribute_list->pair);

which actually should have been written like this:

       g_free(sdp->service_list[i].attribute_list[0].pair);

The attribute_list is sorted with qsort; but indeed the first
entry of attribute_list should point to "data" even after the qsort,
because the first record has id SDP_ATTR_RECORD_HANDLE, whose
numeric value is zero.

But hang on.  The qsort function is

    static int sdp_attributeid_compare(
                const struct sdp_service_attribute_s *a,
                const struct sdp_service_attribute_s *b)
    {
        return (int) b->attribute_id - a->attribute_id;
    }

but no one ever writes attribute_id.  So it only works if qsort is
stable, and who knows what else is broken, but we can fix it by
setting attribute_id in the while loop.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/bt/sdp.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)
diff mbox

Patch

diff --git a/hw/bt/sdp.c b/hw/bt/sdp.c
index f67b3b8..3cb60b9 100644
--- a/hw/bt/sdp.c
+++ b/hw/bt/sdp.c
@@ -580,7 +580,7 @@  static void bt_l2cap_sdp_close_ch(void *opaque)
     int i;
 
     for (i = 0; i < sdp->services; i ++) {
-        g_free(sdp->service_list[i].attribute_list->pair);
+        g_free(sdp->service_list[i].attribute_list[0].pair);
         g_free(sdp->service_list[i].attribute_list);
         g_free(sdp->service_list[i].uuid);
     }
@@ -720,6 +720,8 @@  static void sdp_service_record_build(struct sdp_service_record_s *record,
         len += sdp_attr_max_size(&def->attributes[record->attributes ++].data,
                         &record->uuids);
     }
+
+    assert(len > 0);
     record->uuids = pow2ceil(record->uuids);
     record->attribute_list =
             g_malloc0(record->attributes * sizeof(*record->attribute_list));
@@ -730,12 +732,14 @@  static void sdp_service_record_build(struct sdp_service_record_s *record,
     record->attributes = 0;
     uuid = record->uuid;
     while (def->attributes[record->attributes].data.type) {
+        int attribute_id = def->attributes[record->attributes].id;
         record->attribute_list[record->attributes].pair = data;
+        record->attribute_list[record->attributes].attribute_id = attribute_id;
 
         len = 0;
         data[len ++] = SDP_DTYPE_UINT | SDP_DSIZE_2;
-        data[len ++] = def->attributes[record->attributes].id >> 8;
-        data[len ++] = def->attributes[record->attributes].id & 0xff;
+        data[len ++] = attribute_id >> 8;
+        data[len ++] = attribute_id & 0xff;
         len += sdp_attr_write(data + len,
                         &def->attributes[record->attributes].data, &uuid);
 
@@ -749,10 +753,15 @@  static void sdp_service_record_build(struct sdp_service_record_s *record,
         data += len;
     }
 
-    /* Sort the attribute list by the AttributeID */
+    /* Sort the attribute list by the AttributeID.  The first must be
+     * SDP_ATTR_RECORD_HANDLE so that bt_l2cap_sdp_close_ch can free
+     * the buffer.
+     */
     qsort(record->attribute_list, record->attributes,
                     sizeof(*record->attribute_list),
                     (void *) sdp_attributeid_compare);
+    assert(record->attribute_list[0].pair == data);
+
     /* Sort the searchable UUIDs list for bisection */
     qsort(record->uuid, record->uuids,
                     sizeof(*record->uuid),