[libvirt] [PATCH] storage: fix memory leak with encrypted images

[Jim Fehlig]

Eric Blake wrote:
> Jim Fehlig reported a regression found by libvirt-TCK tests:
>
>   
>> ~ # perl /usr/share/libvirt-tck/tests/qemu/100-disk-encryption.t
>>     
> ...
>   
>> ok 4 - defined persistent domain config
>> # Starting inactive domain config
>> libvirt error code: 1, message: internal error: unable to execute QEMU command
>> 'cont': 'drive-ide0-0-1'
>> (/var/cache/libvirt-tck/300-disk-encryption/demo.qcow2) is encrypted
>>     
>
> Commit 2279d560 converted a boolean into a pointer with the intent of
> transferring that pointer out of a temporary object into the caller's
> data structure.  The temporary structure meant that meta->encryption
> was always NULL on entry, so we could get away with blindly allocating
> the pointer when the header said so.  But later commits then tweaked
> things to do backing chain detection in-place, rather than via a
> temporary object; this has the net result that meta->encryption can be
> non-NULL on entry.

For reference, bisected and found the 'later commit' you mentioned to be

commit 8823272d41a259c1246c05d89f40ad3614fba58c
Author: Peter Krempa <pkrempa at redhat.com>
Date:   Fri Apr 18 14:49:54 2014 +0200

    util: storage: Invert the way recursive metadata retrieval works

>   Not only did this turn the latent behavior into a
> memory leak, it is also a behavior regression: blindly allocating a
> new pointer wipes out what secrets we already knew about the chain,
> making it impossible to restart the domain.
>
> Of course, no one in their right mind should be relying on qcow2
> encryption - it is fundamentally flawed.  And sadly, the TCK tests
> don't get run often enough, and this shows that our virstoragetest
> does not exercise encrypted images at all.  Otherwise, we could
> have avoided a release containing this regression.
>
> * src/util/virstoragefile.c (virStorageFileGetMetadataInternal):
> Don't nuke an already-existing encryption.
>
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  src/util/virstoragefile.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 43b7a5a..0792dd8 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -805,7 +805,8 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta,
>
>          crypt_format = virReadBufInt32BE(buf +
>                                           fileTypeInfo[meta->format].qcowCryptOffset);
> -        if (crypt_format && VIR_ALLOC(meta->encryption) < 0)
> +        if (crypt_format && !meta->encryption &&
> +            VIR_ALLOC(meta->encryption) < 0)
>              goto cleanup;
>      }
>   

ACK.

Regards,
Jim