[libvirt] [PATCH 0/3] Fix virsh race and coredump
I hope that this is an acceptable solution. The race is caused by referencing a disposed connection object in a callback. In the first patch we make sure that the object reference count is reflecting the callback registration. The second patch prevents the invocation of a NULL callback. The last one tries to ensure that we don't leak connection references. Viktor Mihajlovski (3): libvirt: Increase connection reference count for callbacks remote: Don't call NULL closeFreeCallback virsh: Unregister the connection close notifier upon termination src/libvirt.c | 5 +++++ src/remote/remote_driver.c | 3 ++- tools/virsh.c | 23 +++++++++++++++++------ 3 files changed, 24 insertions(+), 7 deletions(-) -- 1.7.9.5
By adjusting the reference count of the connection object we prevent races between callback function and virConnectClose. Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- src/libvirt.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/libvirt.c b/src/libvirt.c index 02d5dd9..f7df26a 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -20184,6 +20184,8 @@ int virConnectRegisterCloseCallback(virConnectPtr conn, return -1; } + virObjectRef(conn); + virMutexLock(&conn->lock); virCheckNonNullArgGoto(cb, error); @@ -20204,6 +20206,7 @@ int virConnectRegisterCloseCallback(virConnectPtr conn, error: virMutexUnlock(&conn->lock); + virObjectUnref(conn); virDispatchError(NULL); return -1; } @@ -20253,6 +20256,8 @@ int virConnectUnregisterCloseCallback(virConnectPtr conn, virMutexUnlock(&conn->lock); + virObjectUnref(conn); + return 0; error: -- 1.7.9.5
Check function pointer before calling. Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- src/remote/remote_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 3721af9..885120e 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -358,7 +358,8 @@ static void remoteClientCloseFunc(virNetClientPtr client ATTRIBUTE_UNUSED, closeCallback(conn, reason, closeOpaque); virMutexLock(&conn->lock); conn->closeDispatch = false; - if (conn->closeUnregisterCount != closeUnregisterCount) + if (conn->closeUnregisterCount != closeUnregisterCount && + closeFreeCallback) closeFreeCallback(closeOpaque); } virMutexUnlock(&conn->lock); -- 1.7.9.5
Before closing the connection we unregister the close callback to prevent a reference leak. We make sure that we only unregister if we have previously registered a callback (not the case for virsh connect!). Further, the messages on virConnectClose != 0 are a bit more specific now. Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- tools/virsh.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index d822e09..4be8fa0 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -311,6 +311,8 @@ vshCatchDisconnect(virConnectPtr conn ATTRIBUTE_UNUSED, disconnected++; } +static int callback_registered = 0; + /* * vshReconnect: * @@ -336,9 +338,13 @@ vshReconnect(vshControl *ctl) else vshError(ctl, "%s", _("failed to connect to the hypervisor")); } else { - if (virConnectRegisterCloseCallback(ctl->conn, vshCatchDisconnect, - NULL, NULL) < 0) - vshError(ctl, "%s", _("Unable to register disconnect callback")); + if (!callback_registered) { + if (virConnectRegisterCloseCallback(ctl->conn, vshCatchDisconnect, + NULL, NULL) == 0) + callback_registered = 1; + else + vshError(ctl, "%s", _("Unable to register disconnect callback")); + } if (connected) vshError(ctl, "%s", _("Reconnected to the hypervisor")); } @@ -2672,9 +2678,14 @@ vshDeinit(vshControl *ctl) VIR_FREE(ctl->name); if (ctl->conn) { int ret; - if ((ret = virConnectClose(ctl->conn)) != 0) { - vshError(ctl, _("Failed to disconnect from the hypervisor, %d leaked reference(s)"), ret); - } + if (callback_registered && + virConnectUnregisterCloseCallback(ctl->conn, vshCatchDisconnect) < 0) + vshError(ctl, "%s", _("Failed to unregister disconnect callback")); + ret = virConnectClose(ctl->conn); + if (ret < 0) + vshError(ctl, "%s", _("Failed to disconnect from the hypervisor")); + else if (ret > 0) + vshError(ctl, "%s", _("Leaked reference(s) after disconnect from the hypervisor")); } virResetLastError(); -- 1.7.9.5
participants (1)
-
Viktor Mihajlovski