[libvirt] [PATCHv2 0/4] Fix race and crash in connection close callback.
Originaly reported by Viktor, this changes a few bits I didn't like on the original series. Peter Krempa (2): virsh: Move cmdConnect from virsh-host.c to virsh.c virsh: Register and unregister the close callback also in cmdConnect Viktor Mihajlovski (2): libvirt: Increase connection reference count for callbacks virsh: Unregister the connection close notifier upon termination src/libvirt.c | 5 +++ tools/virsh-host.c | 67 ---------------------------------- tools/virsh.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 103 insertions(+), 72 deletions(-) -- 1.8.1.5
From: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> 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 4b9ea75..e9aff8a 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -20186,6 +20186,8 @@ int virConnectRegisterCloseCallback(virConnectPtr conn, return -1; } + virObjectRef(conn); + virMutexLock(&conn->lock); virCheckNonNullArgGoto(cb, error); @@ -20206,6 +20208,7 @@ int virConnectRegisterCloseCallback(virConnectPtr conn, error: virMutexUnlock(&conn->lock); + virObjectUnref(conn); virDispatchError(NULL); return -1; } @@ -20255,6 +20258,8 @@ int virConnectUnregisterCloseCallback(virConnectPtr conn, virMutexUnlock(&conn->lock); + virObjectUnref(conn); + return 0; error: -- 1.8.1.5
From: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> Before closing the connection we unregister the close callback to prevent a reference leak. 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, 18 insertions(+), 5 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 7ff12ec..6588470 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -321,9 +321,18 @@ vshReconnect(vshControl *ctl) { bool connected = false; - if (ctl->conn != NULL) { + if (ctl->conn) { + int ret; + connected = true; - virConnectClose(ctl->conn); + + virConnectUnregisterCloseCallback(ctl->conn, vshCatchDisconnect); + ret = virConnectClose(ctl->conn); + if (ret < 0) + vshError(ctl, "%s", _("Failed to disconnect from the hypervisor")); + else if (ret > 0) + vshError(ctl, _("Leaked %d reference(s) after disconnect " + "from the hypervisor"), ret); } ctl->conn = virConnectOpenAuth(ctl->name, @@ -2660,9 +2669,13 @@ 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); - } + virConnectUnregisterCloseCallback(ctl->conn, vshCatchDisconnect); + ret = virConnectClose(ctl->conn); + if (ret < 0) + vshError(ctl, "%s", _("Failed to disconnect from the hypervisor")); + else if (ret > 0) + vshError(ctl, _("Leaked %d reference(s) after disconnect " + "from the hypervisor"), ret); } virResetLastError(); -- 1.8.1.5
The function is used to establish connection so it should be in the main virsh file. This movement also enables further improvements done in next patches. --- tools/virsh-host.c | 67 ---------------------------------------------------- tools/virsh.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 67 deletions(-) diff --git a/tools/virsh-host.c b/tools/virsh-host.c index 2d49296..a7e31da 100644 --- a/tools/virsh-host.c +++ b/tools/virsh-host.c @@ -68,67 +68,6 @@ cmdCapabilities(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) } /* - * "connect" command - */ -static const vshCmdInfo info_connect[] = { - {.name = "help", - .data = N_("(re)connect to hypervisor") - }, - {.name = "desc", - .data = N_("Connect to local hypervisor. This is built-in " - "command after shell start up.") - }, - {.name = NULL} -}; - -static const vshCmdOptDef opts_connect[] = { - {.name = "name", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_EMPTY_OK, - .help = N_("hypervisor connection URI") - }, - {.name = "readonly", - .type = VSH_OT_BOOL, - .help = N_("read-only connection") - }, - {.name = NULL} -}; - -static bool -cmdConnect(vshControl *ctl, const vshCmd *cmd) -{ - bool ro = vshCommandOptBool(cmd, "readonly"); - const char *name = NULL; - - if (ctl->conn) { - int ret; - if ((ret = virConnectClose(ctl->conn)) != 0) { - vshError(ctl, _("Failed to disconnect from the hypervisor, %d leaked reference(s)"), ret); - return false; - } - ctl->conn = NULL; - } - - VIR_FREE(ctl->name); - if (vshCommandOptStringReq(ctl, cmd, "name", &name) < 0) - return false; - - ctl->name = vshStrdup(ctl, name); - - ctl->useGetInfo = false; - ctl->useSnapshotOld = false; - ctl->readonly = ro; - - ctl->conn = virConnectOpenAuth(ctl->name, virConnectAuthPtrDefault, - ctl->readonly ? VIR_CONNECT_RO : 0); - - if (!ctl->conn) - vshError(ctl, "%s", _("Failed to connect to the hypervisor")); - - return !!ctl->conn; -} - -/* * "freecell" command */ static const vshCmdInfo info_freecell[] = { @@ -912,12 +851,6 @@ const vshCmdDef hostAndHypervisorCmds[] = { .info = info_capabilities, .flags = 0 }, - {.name = "connect", - .handler = cmdConnect, - .opts = opts_connect, - .info = info_connect, - .flags = VSH_CMD_FLAG_NOCONNECT - }, {.name = "freecell", .handler = cmdFreecell, .opts = opts_freecell, diff --git a/tools/virsh.c b/tools/virsh.c index 6588470..0f9bb28 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -355,6 +355,69 @@ vshReconnect(vshControl *ctl) ctl->useSnapshotOld = false; } + +/* + * "connect" command + */ +static const vshCmdInfo info_connect[] = { + {.name = "help", + .data = N_("(re)connect to hypervisor") + }, + {.name = "desc", + .data = N_("Connect to local hypervisor. This is built-in " + "command after shell start up.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_connect[] = { + {.name = "name", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_EMPTY_OK, + .help = N_("hypervisor connection URI") + }, + {.name = "readonly", + .type = VSH_OT_BOOL, + .help = N_("read-only connection") + }, + {.name = NULL} +}; + +static bool +cmdConnect(vshControl *ctl, const vshCmd *cmd) +{ + bool ro = vshCommandOptBool(cmd, "readonly"); + const char *name = NULL; + + if (ctl->conn) { + int ret; + if ((ret = virConnectClose(ctl->conn)) != 0) { + vshError(ctl, _("Failed to disconnect from the hypervisor, %d leaked reference(s)"), ret); + return false; + } + ctl->conn = NULL; + } + + VIR_FREE(ctl->name); + if (vshCommandOptStringReq(ctl, cmd, "name", &name) < 0) + return false; + + ctl->name = vshStrdup(ctl, name); + + ctl->useGetInfo = false; + ctl->useSnapshotOld = false; + ctl->readonly = ro; + + ctl->conn = virConnectOpenAuth(ctl->name, virConnectAuthPtrDefault, + ctl->readonly ? VIR_CONNECT_RO : 0); + + if (!ctl->conn) + vshError(ctl, "%s", _("Failed to connect to the hypervisor")); + + return !!ctl->conn; +} + + #ifndef WIN32 static void vshPrintRaw(vshControl *ctl, ...) @@ -3000,6 +3063,12 @@ vshParseArgv(vshControl *ctl, int argc, char **argv) } static const vshCmdDef virshCmds[] = { + {.name = "connect", + .handler = cmdConnect, + .opts = opts_connect, + .info = info_connect, + .flags = VSH_CMD_FLAG_NOCONNECT + }, {.name = "cd", .handler = cmdCd, .opts = opts_cd, -- 1.8.1.5
This patch improves the error message after disconnecting from the hypervisor and adds the close callback operations required not to leak the callback reference. --- tools/virsh.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 0f9bb28..9fb517f 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -391,10 +391,15 @@ cmdConnect(vshControl *ctl, const vshCmd *cmd) if (ctl->conn) { int ret; - if ((ret = virConnectClose(ctl->conn)) != 0) { - vshError(ctl, _("Failed to disconnect from the hypervisor, %d leaked reference(s)"), ret); - return false; - } + + virConnectUnregisterCloseCallback(ctl->conn, vshCatchDisconnect); + ret = virConnectClose(ctl->conn); + if (ret < 0) + vshError(ctl, "%s", _("Failed to disconnect from the hypervisor")); + else if (ret > 0) + vshError(ctl, _("Leaked %d reference(s) after disconnect " + "from the hypervisor"), ret); + ctl->conn = NULL; } @@ -411,10 +416,16 @@ cmdConnect(vshControl *ctl, const vshCmd *cmd) ctl->conn = virConnectOpenAuth(ctl->name, virConnectAuthPtrDefault, ctl->readonly ? VIR_CONNECT_RO : 0); - if (!ctl->conn) + if (!ctl->conn) { vshError(ctl, "%s", _("Failed to connect to the hypervisor")); + return false; + } - return !!ctl->conn; + if (virConnectRegisterCloseCallback(ctl->conn, vshCatchDisconnect, + NULL, NULL) < 0) + vshError(ctl, "%s", _("Unable to register disconnect callback")); + + return true; } -- 1.8.1.5
On 03/27/2013 03:05 PM, Peter Krempa wrote:
This patch improves the error message after disconnecting from the hypervisor and adds the close callback operations required not to leak the callback reference. --- tools/virsh.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)
Hi Peter, I see a bunch of errors running make check with this patch applied (at least in conjunction with my patches). Mostly: error: Failed to disconnect from the hypervisor, 1 leaked reference(s) -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 03/27/13 16:00, Viktor Mihajlovski wrote:
On 03/27/2013 03:05 PM, Peter Krempa wrote:
This patch improves the error message after disconnecting from the hypervisor and adds the close callback operations required not to leak the callback reference. --- tools/virsh.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)
Hi Peter, I see a bunch of errors running make check with this patch applied (at least in conjunction with my patches). Mostly: error: Failed to disconnect from the hypervisor, 1 leaked reference(s)
Hmm, let me check. I might have missed some stuff without noticing :/ Peter
On 03/27/13 16:05, Peter Krempa wrote:
On 03/27/13 16:00, Viktor Mihajlovski wrote:
On 03/27/2013 03:05 PM, Peter Krempa wrote:
This patch improves the error message after disconnecting from the hypervisor and adds the close callback operations required not to leak the callback reference. --- tools/virsh.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)
Hi Peter, I see a bunch of errors running make check with this patch applied (at least in conjunction with my patches). Mostly: error: Failed to disconnect from the hypervisor, 1 leaked reference(s)
Hmm, let me check. I might have missed some stuff without noticing :/
Peter
I've re-run the test suite and I'm not getting any errors. I have the current upstream (including your 2/3 patch from v1) and these four on top. What tests are failing for you? Peter
On 03/27/2013 04:13 PM, Peter Krempa wrote:
I've re-run the test suite and I'm not getting any errors. I have the current upstream (including your 2/3 patch from v1) and these four on top.
What tests are failing for you?
Peter
After taking out 1/3 I don't get the errors, which is logical, as the refcount isn't incremented in the register callback. It seems that there are quite a lot situations where virsh exits without unregistering the callback. Please apply 1/3 (which is actually the essential patch of the series) to see it happen. -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 03/27/13 16:26, Viktor Mihajlovski wrote:
On 03/27/2013 04:13 PM, Peter Krempa wrote:
I've re-run the test suite and I'm not getting any errors. I have the current upstream (including your 2/3 patch from v1) and these four on top.
What tests are failing for you?
Peter
After taking out 1/3 I don't get the errors, which is logical, as the refcount isn't incremented in the register callback. It seems that there are quite a lot situations where virsh exits without unregistering the callback. Please apply 1/3 (which is actually the essential patch of the series) to see it happen.
My tree actually contains 1/3 from your v1 series and I also reposted it in my v2 and I don't seem to be getting errors when I have the 4 (5 with the one pushed upstream) patches applied. Peter
On 03/27/2013 04:32 PM, Peter Krempa wrote:
My tree actually contains 1/3 from your v1 series and I also reposted it in my v2 and I don't seem to be getting errors when I have the 4 (5 with the one pushed upstream) patches applied.
Peter
Never mind and sorry. It seems that I was tricked by our mail gateway which delivered the 1/4 and 2/4 messages without the [libvirt] subject prefix. So I ended up seeing only 3/4 and 4/4 in my libvir-list folder. Argh... With everything applied itlooks and works great. From my side a +1. -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 03/27/13 15:05, Peter Krempa wrote:
Originaly reported by Viktor, this changes a few bits I didn't like on the original series.
Peter Krempa (2): virsh: Move cmdConnect from virsh-host.c to virsh.c virsh: Register and unregister the close callback also in cmdConnect
Viktor Mihajlovski (2): libvirt: Increase connection reference count for callbacks virsh: Unregister the connection close notifier upon termination
src/libvirt.c | 5 +++ tools/virsh-host.c | 67 ---------------------------------- tools/virsh.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 103 insertions(+), 72 deletions(-)
This series is now obsolete. It did not fix all the possible paths where the memory corruption could happen. The new version is at: http://www.redhat.com/archives/libvir-list/2013-March/msg01682.html Peter
participants (2)
-
Peter Krempa -
Viktor Mihajlovski