Commit 393c13b9 authored by Paolo Bonzini's avatar Paolo Bonzini
Browse files

bt: stop the sdp memory allocation craziness 



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: default avatarPaolo Bonzini <pbonzini@redhat.com>
parent f5aa69bd

hw/bt/sdp.c

+13 −4
Original line number Diff line number Diff line
@@ -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),