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
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
> ---
> 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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
