[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] virStoragePoolSourceFree(S) does not free S



On Fri, Feb 05, 2010 at 05:26:35PM +0100, Jim Meyering wrote:
> I was surprised to see that virStoragePoolSourceFree(S) does not free S.
> The other three vir*Free functions in storage_conf *do* free S.

[snip]

> One fix might be to call VIR_FREE(def) in the "if (def)..."
> block, but that seems short-sighted, since the name
> virStoragePoolSourceFree makes me think *it* should be
> doing the whole job.

It is a bad name - it should be renamed to virStoragePoolSourceClear()

> 
> However, if I make the logical change to virStoragePoolSourceFree,
> 
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 62b8394..ffe38cc 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -291,6 +291,7 @@ virStoragePoolSourceFree(virStoragePoolSourcePtr source) {
>          VIR_FREE(source->auth.chap.login);
>          VIR_FREE(source->auth.chap.passwd);
>      }
> +    VIR_FREE(source);
>  }
> 
> that causes "make check" to fail miserably due to heap corruption,
> as reported by valgrind:

> I tracked the problem down to this definition in src/conf/storage_conf.h:
> 
>     typedef struct _virStoragePoolDef virStoragePoolDef;
>     typedef virStoragePoolDef *virStoragePoolDefPtr;
>     struct _virStoragePoolDef {
>         /* General metadata */
>         char *name;
>         unsigned char uuid[VIR_UUID_BUFLEN];
>         int type; /* virStoragePoolType */
> 
>         unsigned long long allocation;
>         unsigned long long capacity;
>         unsigned long long available;
> 
>         virStoragePoolSource source;   <== this is a *STRUCT*, not a ptr
>         virStoragePoolTarget target;   <== Likewise
>     };


Yep, the 'virStoragePoolSource' object is embedded directly in other
structs, not referenced via a pointer, so you can't free this object
directly most of the time... except we later added an internal API
which lets you use virStoragePoolSource as a standalone object which
does need free'ing.

I think we need to rename the current virStoragePoolSourceFree
to virStoragePoolSourceClear(), and then add a new implmenetation
of virStoragePoolSourceFree that calls Clear() and VIR_FREE(def)
making the latter be used where applicable.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]