[libvirt] [PATCH] test driver: don't unlock pool after freeing it
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
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@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. 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. Should I post the correct patch in your name or do you want to submit it yourself? Peter
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@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
On Thu, Sep 17, 2015 at 09:12:31 -0400, David Mansfield wrote:
On 09/17/2015 01:58 AM, Peter Krempa wrote:
On Wed, Sep 16, 2015 at 17:14:28 -0400, David Mansfield wrote:
...
From 08927d6ff222603e1be2a032c5fed68a5df8c68f Mon Sep 17 00:00:00 2001 From: David Mansfield <dmansfield@gmail.com> Date: Thu, 17 Sep 2015 08:59:24 -0400 Subject: [PATCH v2] test driver: don't unlock pool after freeing it
--- src/test/test_driver.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 213a9a1..90ab09f 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4321,6 +4321,7 @@ testStoragePoolUndefine(virStoragePoolPtr pool) }
virStoragePoolObjRemove(&privconn->pools, privpool); + privpool = NULL; ret = 0;
cleanup: -- 2.4.3
ACK and pushed now. I've added a valgrind trace to the commit message. Thanks for taking time and fixing this. Peter (Btw, we usually prefer patch postings via "git send-email" but for single patches that isn't a big issue and the patch applied just fine).
participants (2)
-
David Mansfield -
Peter Krempa