On 05/03/2011 11:18 PM, Laine Stump wrote:

On 05/03/2011 01:58 PM, Eric Blake wrote:

...

Detected by clang. NULL deref added in commit 343a27a (Mar 11), but leak of voldef present since commit 2cd9b2d (Apr 09).

* src/storage/storage_driver.c (storageVolumeCreateXML): Don't leak voldef or dereference null volobj. --- src/storage/storage_driver.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 1ea5d12..19c7d63 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1319,8 +1319,10 @@ storageVolumeCreateXML(virStoragePoolPtr obj, pool->volumes.objs[pool->volumes.count++] = voldef; volobj = virGetStorageVol(obj->conn, pool->def->name, voldef->name, voldef->key); + if (!volobj) + goto cleanup;

At this point, voldef has been added into pool->volumes.objs[], but voldef hasn't been NULLed out yet, so there is an extra pointer to it. If you goto cleanup, you will end up calling virStorageVolDefFree(voldef), so that object will get freed, but pool->volumes.objs still has a pointer to it.

You either need to NULL out voldef, or remove it from pool->volumes.objs[] (ie, just decrement the count). I'm too tired to figure out which is more appropriate :-)

Good catch. I think decrementing the count is correct. v2 coming up. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On 05/04/2011 12:52 PM, Eric Blake wrote:

Detected by clang. NULL deref added in commit 343a27a (Mar 11), but leak of voldef present since commit 2cd9b2d (Apr 09).

* src/storage/storage_driver.c (storageVolumeCreateXML): Don't leak voldef or dereference null volobj. ---

v2: don't leave pool->volumes.objs pointing to stale memory on failure

src/storage/storage_driver.c | 8 ++++++-- 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 1ea5d12..5118ffb 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1,7 +1,7 @@ /* * storage_driver.c: core driver for storage APIs * - * Copyright (C) 2006-2010 Red Hat, Inc. + * Copyright (C) 2006-2011 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -1319,8 +1319,12 @@ storageVolumeCreateXML(virStoragePoolPtr obj, pool->volumes.objs[pool->volumes.count++] = voldef; volobj = virGetStorageVol(obj->conn, pool->def->name, voldef->name, voldef->key); + if (!volobj) { + pool->volumes.count--; + goto cleanup; + }

- if (volobj&& backend->buildVol) { + if (backend->buildVol) { int buildret; virStorageVolDefPtr buildvoldef = NULL;

ACK.