[libvirt] [PATCH] qemu: Fix crash when changing media of cdrom or floppy disk

[Daniel P. Berrange]

On Thu, Feb 07, 2013 at 09:53:17PM +0800, Osier Yang wrote:
> The disk def could be free'ed by qemuDomainChangeEjectableMedia
> for cdrom or floppy disk. This moves the setup/checking before
> the attaching takes place.
> ---
>  src/qemu/qemu_driver.c  |   29 ++++++++++++++---------------
>  src/qemu/qemu_process.c |   16 ++++++++++++----
>  src/qemu/qemu_process.h |    3 ++-
>  3 files changed, 28 insertions(+), 20 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 906501b..3509880 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -5829,8 +5829,20 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
>  
>      if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK &&
>          disk->shared &&
> -        (qemuCheckSharedDisk(driver->sharedDisks, disk) < 0))
> -        goto end;
> +        disk->src) {
> +        if (qemuCheckSharedDisk(driver->sharedDisks, disk, false) < 0)
> +            goto end;
> +
> +        if (qemuAddSharedDisk(driver->sharedDisks, disk->src) < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Failed to add disk '%s' to shared disk table"),
> +                           disk->src);
> +            goto end;
> +        }
> +    }
> +
> +    if (qemuSetUnprivSGIO(disk) < 0)
> +         goto end;

Getting better, but this is still flawed. qemuSetUnprivSGIO expects
the caller to have validated type==BLOCK - so this will still crash
if given a disk type==NETWORK.

IMHO these APIs are badly designed - you should put the check for
type=BLOCK and shared == true, into the methods themselves and not
rely on the callers to do it right, since clearly they are not.


> @@ -5883,19 +5895,6 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
>                       NULLSTR(disk->src));
>      }
>  
> -    if (ret == 0) {
> -        if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK &&
> -            disk->shared &&
> -            disk->src) {
> -            if (qemuAddSharedDisk(driver->sharedDisks, disk->src) < 0)
> -                VIR_WARN("Failed to add disk '%s' to shared disk table",
> -                         disk->src);
> -        }
> -
> -        if (qemuSetUnprivSGIO(disk) < 0)
> -            VIR_WARN("Failed to set unpriv_sgio of disk '%s'", disk->src);
> -    }
> -

If the existing device that was associated with the CD was 
a shared one with SGIO set, then this is still leaking that
usage.  You'd need to remove the old device from the list
after changing media.

Or just unconditionally forbid setting SGIO==true for any
media=cdrom devices.

> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 98ed552..ee5a10c 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3442,7 +3442,14 @@ qemuSetUnprivSGIO(virDomainDiskDefPtr disk)
>      return 0;
>  }
>  
> -/* Check if a shared disk's setting conflicts with the conf
> +/* qemuCheckSharedDisk:
> + * @sharedDisks: Shared disk hash table
> + * @disk: Disk def
> + * @after_adding: Whether this function is involked after
> + *                adding the disk to the hash table by
> + *                qemuAddSharedDisk.
> + *
> + * Check if a shared disk's setting conflicts with the conf
>   * used by other domain(s). Currently only checks the sgio
>   * setting. Note that this should only be called for disk with
>   * block source.
> @@ -3451,7 +3458,8 @@ qemuSetUnprivSGIO(virDomainDiskDefPtr disk)
>   */
>  int
>  qemuCheckSharedDisk(virHashTablePtr sharedDisks,
> -                    virDomainDiskDefPtr disk)
> +                    virDomainDiskDefPtr disk,
> +                    bool after_adding)

This is also a symptom of bad design IMHO. The functionality for
qemuCheckSharedDisk and qemuAddSharedDisk disk should be merged
into one single method, instead of relying on the callers to
invoke both in the correct order.

>  {
>      int val;
>      size_t *ref = NULL;
> @@ -3470,7 +3478,7 @@ qemuCheckSharedDisk(virHashTablePtr sharedDisks,
>      if (!(ref = virHashLookup(sharedDisks, key)))
>          goto cleanup;
>  
> -    if (ref == (void *)0x1)
> +    if (after_adding && ref == (void *)0x1)
>          goto cleanup;
>  
>      if (virGetDeviceUnprivSGIO(disk->src, NULL, &val) < 0) {
> @@ -3846,7 +3854,7 @@ int qemuProcessStart(virConnectPtr conn,
>              if (qemuAddSharedDisk(driver->sharedDisks, disk->src) < 0)
>                  goto cleanup;
>  
> -            if (qemuCheckSharedDisk(driver->sharedDisks, disk) < 0)
> +            if (qemuCheckSharedDisk(driver->sharedDisks, disk, true) < 0)
>                  goto cleanup;
>          }
>  
> diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
> index 313fa39..ff294e7 100644
> --- a/src/qemu/qemu_process.h
> +++ b/src/qemu/qemu_process.h
> @@ -101,6 +101,7 @@ virBitmapPtr qemuPrepareCpumap(virQEMUDriverPtr driver,
>  int qemuSetUnprivSGIO(virDomainDiskDefPtr disk);
>  
>  int qemuCheckSharedDisk(virHashTablePtr sharedDisks,
> -                        virDomainDiskDefPtr disk);
> +                        virDomainDiskDefPtr disk,
> +                        bool after_adding);
>  
>  #endif /* __QEMU_PROCESS_H__ */


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|