[libvirt] [PATCH 14/20] snapshot: qemu: Add support for external inactive snapshots

[Eric Blake]

On 10/23/2012 09:12 AM, Peter Krempa wrote:
> This patch adds support for external disk snapshots of inactive domains.
> The snapshot is created by calling
>  qemu-img create -o backing_file=/path/to/disk /path/snapshot_file

This doesn't match the actual code, which also adds '-f format'
arguments.  Also, see below about an incomplete argument.

> on each of the disks selected for snapshotting.


> +/* The domain is expected to be locked and inactive. */
> +static int
> +qemuDomainSnapshotCreateInactiveExternal(struct qemud_driver *driver,
> +                                         virDomainObjPtr vm,
> +                                         virDomainSnapshotObjPtr snap,
> +                                         bool reuse)
> +{
> +    const char *qemuimgarg[] = { NULL, "create", "-f", NULL, "-o", NULL, NULL, NULL };

This seems awkward.  I guess I see why you're preparing the list up
front (because we are creating different commands in a loop, and because
of copy-and-paste from earlier code), but I wonder if it would be
simpler to create a virCommand from scratch on each iteration of the
loop instead of reusing qemuimgarg.


> +
> +        if (snapdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
> +            VIR_FREE(backingFileArg);
> +
> +            /* remove old disk image */
> +            if (reuse && virFileExists(snapdisk->file) &&
> +                unlink(snapdisk->file) < 0) {

Hmm, I wonder if that's the right thing to do?  If the file already
exists, then it seems like we should just skip the qemu-img call
altogether, and use the file as-is (trusting that the user created it
correctly, the same as we would do for an active disk snapshot), rather
than unlink()ing and then recreating the file.  Also, unlink() is unsafe
if the existing file is a block device instead of a regular file.


> +
> +            if (virAsprintf(&backingFileArg, "backing_file=%s", defdisk->src) < 0) {
> +                virReportOOMError();
> +                goto cleanup;

This is a reintroduction of a backing file probing CVE if we know the
file format of defdisk->src.  You need to pass -o
backing_file=%s,backing_fmt=%s with the backing format, if it is known,
otherwise, we have to probe a file type where we previously knew the
file type.

I didn't look closely at the rest of the patch, but want to make sure we
avoid a security hole.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 617 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20121031/8598044d/attachment-0001.sig>