[libvirt] [PATCH] util: Fix domain object leaks on closecallbacks

[Michal Privoznik]

On 01/25/2017 05:37 PM, John Ferlan wrote:
> Originally/discovered proposed by "Wang King <king.wang at huawei.com>"
> 
> When the virCloseCallbacksSet is first called, it increments the refcnt
> on the domain object to ensure it doesn't get deleted before the callback
> is called. The refcnt would be decremented in virCloseCallbacksUnset once
> the entry is removed from the closeCallbacks has table.
> 
> When (mostly) normal shutdown occurs, the qemuProcessStop will end up
> calling qemuProcessAutoDestroyRemove and will remove the callback from
> the list and hash table normally and decrement the refcnt.
> 
> However, when qemuConnectClose calls virCloseCallbacksRun, it will scan
> the (locked) closeCallbacks list for matching domain and callback function.
> If an entry is found, it will be removed from the closeCallbacks list and
> placed into a lookaside list to be processed when the closeCallbacks lock
> is dropped. The callback function (e.g. qemuProcessAutoDestroy) is called
> and will run qemuProcessStop. That code will fail to find the callback
> in the list when qemuProcessAutoDestroyRemove is called and thus not decrement
> the domain refcnt. Instead since the entry isn't found the code will just
> return (mostly) harmlessly.
> 
> This patch will resolve the issue by taking another ref during the
> search UUID process during virCloseCallackRun, decrementing the refcnt
> taken by virCloseCallbacksSet, calling the callback routine and returning
> overwriting the vm (since it could return NULL). Finally, it will call the
> virDomainObjEndAPI to lower the refcnt and remove the lock taken during
> the search UUID processing. This may cause the vm to be destroyed.
> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  src/util/virclosecallbacks.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/src/util/virclosecallbacks.c b/src/util/virclosecallbacks.c
> index 2f430cf..4db50e8 100644
> --- a/src/util/virclosecallbacks.c
> +++ b/src/util/virclosecallbacks.c
> @@ -346,17 +346,24 @@ virCloseCallbacksRun(virCloseCallbacksPtr closeCallbacks,
>      for (i = 0; i < list->nentries; i++) {
>          virDomainObjPtr vm;
>  
> -        if (!(vm = virDomainObjListFindByUUID(domains,
> -                                              list->entries[i].uuid))) {
> +        /* Grab a ref and lock to the vm */
> +        if (!(vm = virDomainObjListFindByUUIDRef(domains,
> +                                                 list->entries[i].uuid))) {
>              char uuidstr[VIR_UUID_STRING_BUFLEN];
>              virUUIDFormat(list->entries[i].uuid, uuidstr);
>              VIR_DEBUG("No domain object with UUID %s", uuidstr);
>              continue;
>          }
>  
> -        vm = list->entries[i].callback(vm, conn, opaque);
> -        if (vm)
> -            virObjectUnlock(vm);
> +        /* Remove the ref taken out during virCloseCallbacksSet since
> +         * we're about to call the callback function and we have another
> +         * ref anyway (so it cannot be deleted).
> +         *
> +         * Call the callback function, ignoring the return since it might be
> +         * NULL. Once we're done with the object, then end the API usage. */
> +        virObjectUnref(vm);
> +        ignore_value(list->entries[i].callback(vm, conn, opaque));
> +        virDomainObjEndAPI(&vm);
>      }
>      VIR_FREE(list->entries);
>      VIR_FREE(list);
> 

Looks very hacky, but I am unable to come up with a better fix.

ACK

Michal