[libvirt] [PATCH v2] test driver: don't unlock pool after freeing it

[David Mansfield]

On 09/17/2015 01:58 AM, Peter Krempa wrote:
> On Wed, Sep 16, 2015 at 17:14:28 -0400, David Mansfield wrote:
>> The attached patch (taken from my modified Fedora 22 source rpm,
>> 1.2.13.1-2.fc22, sorry),  fixes a case where, in the test driver, memory
>> is accessed after it's freed.
>>
>> Patch applies to latest git with:
>>
>> Hunk #1 succeeded at 4395 (offset -469 lines).
>>
>> The illegal access was found using valgrind.
>>
>> The function testStoragePoolUndefine() calls virStoragePoolObjRemove()
>> (which seems to free the memory for that pool structure) and then in the
>> cleanup stanza calls virStoragePoolObjUnlock() which tampers with the
>> freed structure.
>>
>> --
>> Thanks,
>> David Mansfield
>
>> From: David Mansfield <dmansfield at gmail.com>
>> Date: Tue, 15 Sep 2015 10:05:56 -0400
>> Subject: [PATCH] test driver: don't unlock pool after freeing it
>>
>> The test driver was unlocking the pool object after it had been
>> freed causing memory corruption.
>>
>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
>> index a386270..c2256dc 100644
>> --- a/src/test/test_driver.c
>> +++ b/src/test/test_driver.c
>> @@ -4864,8 +4864,10 @@ testStoragePoolUndefine(virStoragePoolPtr pool)
>>       ret = 0;
>>
>>    cleanup:
>> -    if (privpool)
>> -        virStoragePoolObjUnlock(privpool);
>> +    // privpool cannot be unlocked because memory for it has been
>> +    // freed by the virStoragePoolObjRemove call above
>> +    // if (privpool)
>> +    //     virStoragePoolObjUnlock(privpool);
>
> This cannot be just commented out since the code above is allowing only
> inactive pools to be deleted and jumps to the cleanup label otherwise.
> With this approach the lock would stay locked and the test driver would
> deadlock the next time you are going to reference the same pool.

You're right of course. Corrected patch is attached, this time directly 
against git master.

>
> The right fix here is to clear the 'privpool' pointer after we know that
> the pool was removed.
>
> As a note, line comments are not allowed in libvirt.

Second dumb mistake. I've been in c++ all week and should have know better.

-- 
Thanks,
David Mansfield


-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0009-don-t-unlock-after-free.patch
Type: text/x-patch
Size: 637 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150917/06a099fd/attachment-0001.bin>