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@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@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