Re: [PATCH 1/3] esx: Wrap libcurl multi handle

[Matthias Bolte <matthias.bolte@xxxxxxxxxxxxxx>]

2012/7/3 Doug Goldstein <cardoe@xxxxxxxxxx>:
> On Mon, Jul 2, 2012 at 4:44 PM, Matthias Bolte
> <matthias.bolte@xxxxxxxxxxxxxx> wrote:
>> ---
>>  src/esx/esx_vi.c |  111 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  src/esx/esx_vi.h |   18 +++++++++
>>  2 files changed, 129 insertions(+), 0 deletions(-)

>> +/* esxVI_MultiCURL_Alloc */
>> +ESX_VI__TEMPLATE__ALLOC(MultiCURL)
>> +
>> +/* esxVI_MultiCURL_Free */
>> +ESX_VI__TEMPLATE__FREE(MultiCURL,
>> +{
>> +    if (item->count > 0) {
>> +        /* Better leak than crash */
>> +        VIR_ERROR(_("Trying to free MultiCURL object that is still in use"));
>> +        return;
>> +    }
>> +
>> +    if (item->handle != NULL) {
>> +        curl_multi_cleanup(item->handle);
>> +    }
>
> Since its a double pointer maybe setting the passed in value to NULL
> to prevent a double free situations?

Actually that's what already happens here but hidden inside the
ESX_VI__TEMPLATE__FREE macro. Expanded esxVI_MultiCURL_Free looks like
this

void esxVI_MultiCURL_Free(esxVI_MultiCURL **multi)
{
    esxVI_MultiCURL *item;

    if (multi == NULL || *multi == NULL) {
        return;
    }

    item = *multi;

    if (item->count > 0) {
        /* Better leak than crash */
        VIR_ERROR(_("Trying to free MultiCURL object that is still in use"));
        return;
    }

    if (item->handle != NULL) {
        curl_multi_cleanup(item->handle);
    }

    VIR_FREE(*multi);
}

As VIR_FREE sets its argument to NULL after freeing it a double free
is not possible here.

>> +int
>> +esxVI_MultiCURL_Remove(esxVI_MultiCURL *multi, esxVI_CURL *curl)
>> +{
>> +    if (curl->handle == NULL) {
>> +        ESX_VI_ERROR(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                     _("Cannot remove uninitialized CURL handle from a "
>> +                       "multi handle"));
>> +        return -1;
>> +    }
>> +
>> +    if (curl->multi == NULL) {
>> +        ESX_VI_ERROR(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                     _("Cannot remove CURL handle from a multi handle when it "
>> +                       "wasn't added before"));
>> +        return -1;
>> +    }
>> +
>> +    if (curl->multi != multi) {
>> +        ESX_VI_ERROR(VIR_ERR_INTERNAL_ERROR, "%s", _("CURL (multi) mismatch"));
>> +        return -1;
>> +    }
>> +
>> +    virMutexLock(&curl->lock);
>> +
>> +    curl_multi_remove_handle(multi->handle, curl->handle);
>> +
>> +    curl->multi = NULL;
>> +    --multi->count;
>> +
>> +    virMutexUnlock(&curl->lock);
>
> Maybe add your free code here when count is 0? That way you wouldn't
> have to contend with a potential memory leak case when the free is
> called when its still ref'd.

count is not meant as a refcount here. It's sole purpose is to avoid
freeing a multi handle that still has easy handles attached to it.
Also calling free one count == 0 here would not allow a call sequence
such as add, remove, add, remove.

-- 
Matthias Bolte
http://photron.blogspot.com

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list