[libvirt] [PATCH] storage: avoid null deref and leak on failure

[Laine Stump]

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 :-)

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