[libvirt] [PATCH] qemu: fix crash with shared disks

[John Ferlan]

On 09/17/2014 06:45 AM, Ján Tomko wrote:
> Commit f36a94f introduced a double free on all success paths
> in qemuSharedDeviceEntryInsert.
> 
> Only call qemuSharedDeviceEntryFree on the error path and
> set entry to NULL before jumping there if the entry already
> is in the hash table.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1142722
> ---
>  src/qemu/qemu_conf.c | 26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index ac10b64..adc6caf 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -1011,38 +1011,36 @@ qemuSharedDeviceEntryInsert(virQEMUDriverPtr driver,
>                              const char *name)
>  {
>      qemuSharedDeviceEntry *entry = NULL;
> -    int ret = -1;
>  
>      if ((entry = virHashLookup(driver->sharedDevices, key))) {
>          /* Nothing to do if the shared scsi host device is already
>           * recorded in the table.
>           */
> -        if (qemuSharedDeviceEntryDomainExists(entry, name, NULL)) {
> -            ret = 0;
> -            goto cleanup;
> +        if (!qemuSharedDeviceEntryDomainExists(entry, name, NULL)) {
> +            if (VIR_EXPAND_N(entry->domains, entry->ref, 1) < 0 ||
> +                VIR_STRDUP(entry->domains[entry->ref - 1], name) < 0) {
> +                /* entry is owned by the hash table here */
> +                entry = NULL;

[1] Assigning to NULL causes an issue

> +                goto error;
> +            }
>          }
> -
> -        if (VIR_EXPAND_N(entry->domains, entry->ref, 1) < 0 ||
> -            VIR_STRDUP(entry->domains[entry->ref - 1], name) < 0)
> -            goto cleanup;
>      } else {
>          if (VIR_ALLOC(entry) < 0 ||
>              VIR_ALLOC_N(entry->domains, 1) < 0 ||
>              VIR_STRDUP(entry->domains[0], name) < 0)
> -            goto cleanup;
> +            goto error;
>  
>          entry->ref = 1;
>  
>          if (virHashAddEntry(driver->sharedDevices, key, entry))
> -            goto cleanup;
> +            goto error;
>      }
>  
> -    ret = 0;
> +    return 0;
>  
> - cleanup:
> + error:
>      qemuSharedDeviceEntryFree(entry, NULL);
[1]
Because this is prototyped as:

void qemuSharedDeviceEntryFree(void *payload, const void *name)
    ATTRIBUTE_NONNULL(1);

Coverity gives us a warning when entry = NULL...

It's solveable by either allowing NULL for the function or only calling
if (entry)

ACK as long as we handle in some manner.

John

> -
> -    return ret;
> +    return -1;
>  }
>  
>  
>