On 01/25/2017 05:37 PM, John Ferlan wrote:

Originally/discovered proposed by "Wang King <king.wang@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@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