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

[John Ferlan]

Going through some older on list patches with no activity...  There were
two which appeared to be the same thing, so I chose the later one.
Pardon the delay in processing this...

On 11/23/2016 11:29 PM, Wang King wrote:
> From: wangjing <king.wang at huawei.com>
> 
> The virCloseCallbacksSet method increase object reference for
> VM, and decrease object reference in virCloseCallbacksUnset.
> But VM UUID will be deleted from closecallbacks list in
> virCloseCallbacksRun when connection disconnected, and then
> object reference cannot be decreased by virCloseCallbacksUnset
> in callback functions.
> 
> Signed-off-by: Wang King <king.wang at huawei.com>
> ---
>  src/util/virclosecallbacks.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
> 

It seems there may be multiple things being fixed - if so then they
should each be separate patches.

Of the following paths (consumers of close callbacks), which was causing
the refcnt issue?  Do you have a backtrace?  How reproducible?

1. bhyveProcessAutoDestroy will destroy the transient vm, return NULL
2. lxcProcessAutoDestroy will destroy the transient vm, return NULL
3. qemuMigrationCleanup will not destroy the vm
4. qemuProcessAutoDestroy will call qemuDomainRemoveInactive which looks
to destroy the transient vm... if it does then last virDomainObjEndAPI
will cause the return to be NULL


> diff --git a/src/util/virclosecallbacks.c b/src/util/virclosecallbacks.c
> index 891a92b..26d5075 100644
> --- a/src/util/virclosecallbacks.c
> +++ b/src/util/virclosecallbacks.c
> @@ -300,7 +300,9 @@ virCloseCallbacksGetForConn(virCloseCallbacksPtr closeCallbacks,
>      data.list = list;
>      data.oom = false;
>  
> +    virObjectLock(closeCallbacks);
>      virHashForEach(closeCallbacks->list, virCloseCallbacksGetOne, &data);
> +    virObjectUnlock(closeCallbacks);

So this seems to be "a" bugfix for [1]

>  
>      if (data.oom) {
>          VIR_FREE(list->entries);
> @@ -329,22 +331,15 @@ virCloseCallbacksRun(virCloseCallbacksPtr closeCallbacks,
>       * them all from the hash. At that point we can release
>       * the lock and run the callbacks safely. */
>  
> -    virObjectLock(closeCallbacks);
>      list = virCloseCallbacksGetForConn(closeCallbacks, conn);
>      if (!list)
>          return;

[1]
Considering the former code, this return would return with the
closeCallbacks locked - so that's "one" bug...

>  
>      for (i = 0; i < list->nentries; i++) {
> -        char uuidstr[VIR_UUID_STRING_BUFLEN];
> -        virUUIDFormat(list->entries[i].uuid, uuidstr);
> -        virHashRemoveEntry(closeCallbacks->list, uuidstr);
> -    }

I'm trying to figure out why this hunk of code moved below. Is that a
separate issue?  I would think the process in the comment is what is
desired...  With a lock, grab all the callback entries into a local
structure by UUID and callback function, then clear out the callbacks
from the hash, then call the callback functions.

My concern is this change could have some other timing issue as this
path is called during/from a <driver>ConnectClose() function (bhyve,
qemu, lxc)

> -    virObjectUnlock(closeCallbacks);
> -
> -    for (i = 0; i < list->nentries; i++) {
>          virDomainObjPtr vm;
> +        virDomainObjPtr dom;
>  
> -        if (!(vm = virDomainObjListFindByUUID(domains,
> +        if (!(vm = virDomainObjListFindByUUIDRef(domains,
>                                                list->entries[i].uuid))) {

The second line needs to move 3 spaces to line up under domains... I
think this is the "second" bug fix. In this case, the bug fix is that we
don't want the callback function to remove our reference.


>              char uuidstr[VIR_UUID_STRING_BUFLEN];
>              virUUIDFormat(list->entries[i].uuid, uuidstr);
> @@ -352,10 +347,20 @@ virCloseCallbacksRun(virCloseCallbacksPtr closeCallbacks,
>              continue;
>          }
>  
> -        vm = list->entries[i].callback(vm, conn, opaque);
> -        if (vm)
> -            virObjectUnlock(vm);
> +        dom = list->entries[i].callback(vm, conn, opaque);
> +        if (dom)
> +            virObjectUnlock(dom);
> +        virObjectUnref(vm);

and the corollary for the Ref which probably causes 'vm' to be disposed.
This is more than likely what the issue is - calling something which
causes vm to ref down to 0.

John

>      }
> +
> +    virObjectLock(closeCallbacks);
> +    for (i = 0; i < list->nentries; i++) {
> +        char uuidstr[VIR_UUID_STRING_BUFLEN];
> +        virUUIDFormat(list->entries[i].uuid, uuidstr);
> +        virHashRemoveEntry(closeCallbacks->list, uuidstr);
> +    }
> +    virObjectUnlock(closeCallbacks);
> +
>      VIR_FREE(list->entries);
>      VIR_FREE(list);
>  }
>