[libvirt] [PATCH 0/4] admin: Fix a leaking conn reference and other improvements
This series fixes a memory leak in a dangling conn object reference caused by assigning NULL to conn->closeCallback object by the RPC catch-disconnect routine followed by a comparison of equality of conn->closeCallback->callback and the client-registered callback reference. Before fixing the issue itself, there are some new helpers introduced following the same idea as the helpers introduced by series https://www.redhat.com/archives/libvir-list/2016-February/msg00849.html Erik Skultety (4): vsh: Drop conditional error reporting in vshErrorHandler admin: Remove unnecessary @conn object locking datatypes: Introduce some admin-related close callback handling helpers admin: Use the newly introduced close callback handling helpers po/POTFILES.in | 1 + src/admin/admin_remote.c | 6 +--- src/datatypes.c | 67 +++++++++++++++++++++++++++++++++++++++--- src/datatypes.h | 8 +++++ src/libvirt-admin.c | 24 +-------------- src/libvirt_admin_private.syms | 3 ++ tools/vsh.c | 2 -- 7 files changed, 77 insertions(+), 34 deletions(-) -- 2.5.5
First, since commit 834c5720 the error reporting within the vshErrorHandler doesn't work because there was a lot of renaming going on (dull mechanical renaming without much thinking about it, yep - shame on me) and so the original env variable VIRSH_DEBUG got renamed to VSH_DEBUG which we don't support nor document anywhere. Second, by specifying this env variable, the last libvirt error gets reported twice despite the fact we say the error reporting should be deferred until the command finishes, and last but not least the vintage code's logic is a bit 'odd', since the error would get reported iff the env variable is set, even if the value should be equal to our DEFAULT value in which case it doesn't make sense that we behave differently when an env variable is set to some value and when there's no env variable at all but we use the same value automatically as default. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- tools/vsh.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index 3ba09dd..07ceb7b 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -246,8 +246,6 @@ vshErrorHandler(void *opaque ATTRIBUTE_UNUSED, virErrorPtr error) { virFreeError(last_error); last_error = virSaveLastError(); - if (virGetEnvAllowSUID("VSH_DEBUG") != NULL) - virDefaultErrorFunc(error); } /* Store a libvirt error that is from a helper API that doesn't raise errors -- 2.5.5
The only place we change the @conn object is actually virAdmConnectOpen routine, thus at the moment we don't really need to lock it, given the fact that what we're trying to do here is to change the closeCallback object which is a lockable object itself, so that should be enough to avoid races. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/libvirt-admin.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c index 88eef54..1b5fd44 100644 --- a/src/libvirt-admin.c +++ b/src/libvirt-admin.c @@ -465,7 +465,6 @@ int virAdmConnectRegisterCloseCallback(virAdmConnectPtr conn, virObjectRef(conn); - virObjectLock(conn); virObjectLock(conn->closeCallback); virCheckNonNullArgGoto(cb, error); @@ -482,13 +481,11 @@ int virAdmConnectRegisterCloseCallback(virAdmConnectPtr conn, conn->closeCallback->freeCallback = freecb; virObjectUnlock(conn->closeCallback); - virObjectUnlock(conn); return 0; error: virObjectUnlock(conn->closeCallback); - virObjectUnlock(conn); virDispatchError(NULL); virObjectUnref(conn); return -1; @@ -517,7 +514,6 @@ int virAdmConnectUnregisterCloseCallback(virAdmConnectPtr conn, virCheckAdmConnectReturn(conn, -1); - virObjectLock(conn); virObjectLock(conn->closeCallback); virCheckNonNullArgGoto(cb, error); @@ -534,14 +530,12 @@ int virAdmConnectUnregisterCloseCallback(virAdmConnectPtr conn, conn->closeCallback->freeCallback = NULL; virObjectUnlock(conn->closeCallback); - virObjectUnlock(conn); virObjectUnref(conn); return 0; error: virObjectUnlock(conn->closeCallback); - virObjectUnlock(conn); virDispatchError(NULL); return -1; } -- 2.5.5
On 11.11.2016 16:32, Erik Skultety wrote:
The only place we change the @conn object is actually virAdmConnectOpen routine, thus at the moment we don't really need to lock it, given the fact that what we're trying to do here is to change the closeCallback object which is a lockable object itself, so that should be enough to avoid races.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/libvirt-admin.c | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c index 88eef54..1b5fd44 100644 --- a/src/libvirt-admin.c +++ b/src/libvirt-admin.c @@ -465,7 +465,6 @@ int virAdmConnectRegisterCloseCallback(virAdmConnectPtr conn,
virObjectRef(conn);
- virObjectLock(conn); virObjectLock(conn->closeCallback);
virCheckNonNullArgGoto(cb, error); @@ -482,13 +481,11 @@ int virAdmConnectRegisterCloseCallback(virAdmConnectPtr conn, conn->closeCallback->freeCallback = freecb;
virObjectUnlock(conn->closeCallback); - virObjectUnlock(conn);
Well, sometimes we are locking objects just for the sake of lock ordering. But looking into the code I cannot find any other place where they would rely on some specific lock ordering. Michal
Well, there were three different spots where closeCallback->freeCallback was called, not looking the same --> potential for bugs - and there indeed is a bug with refcounting of the @conn object. So this patch partially follows the path set by commit 24dbb69f by introducing some close callback helpers both to replace all the spots where we call clean the close callback data with a dedicated function and to be able to fix the refcounting bug causing a memleak. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- po/POTFILES.in | 1 + src/datatypes.c | 62 ++++++++++++++++++++++++++++++++++++++++++ src/datatypes.h | 8 ++++++ src/libvirt_admin_private.syms | 3 ++ 4 files changed, 74 insertions(+) diff --git a/po/POTFILES.in b/po/POTFILES.in index 1469240..5a37083 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -47,6 +47,7 @@ src/cpu/cpu_arm.c src/cpu/cpu_map.c src/cpu/cpu_ppc64.c src/cpu/cpu_x86.c +src/datatypes.c src/driver.c src/esx/esx_driver.c src/esx/esx_network_driver.c diff --git a/src/datatypes.c b/src/datatypes.c index ff0c46f..24e4f77 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -935,6 +935,68 @@ virAdmConnectCloseCallbackDataDispose(void *obj) virObjectUnlock(cb_data); } +void +virAdmConnectCloseCallbackDataReset(virAdmConnectCloseCallbackDataPtr cbdata) +{ + if (cbdata->freeCallback) + cbdata->freeCallback(cbdata->opaque); + + virObjectUnref(cbdata->conn); + cbdata->conn = NULL; + cbdata->freeCallback = NULL; + cbdata->callback = NULL; + cbdata->opaque = NULL; +} + +int +virAdmConnectCloseCallbackDataUnregister(virAdmConnectCloseCallbackDataPtr cbdata, + virAdmConnectCloseFunc cb) +{ + int ret = -1; + + virObjectLock(cbdata); + if (cbdata->callback != cb) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("A different callback was requested")); + goto cleanup; + } + + virAdmConnectCloseCallbackDataReset(cbdata); + ret = 0; + cleanup: + virObjectUnlock(cbdata); + return ret; +} + +int +virAdmConnectCloseCallbackDataRegister(virAdmConnectCloseCallbackDataPtr cbdata, + virAdmConnectPtr conn, + virAdmConnectCloseFunc cb, + void *opaque, + virFreeCallback freecb) +{ + int ret = -1; + + virObjectLock(cbdata); + + if (cbdata->callback) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("A close callback is already registered")); + goto cleanup; + } + + virObjectRef(conn); + cbdata->conn = conn; + cbdata->callback = cb; + cbdata->opaque = opaque; + cbdata->freeCallback = freecb; + + ret = 0; + cleanup: + virObjectUnlock(conn->closeCallback); + return ret; +} + virAdmServerPtr virAdmGetServer(virAdmConnectPtr conn, const char *name) { diff --git a/src/datatypes.h b/src/datatypes.h index 2b6adb4..9a5fbbc 100644 --- a/src/datatypes.h +++ b/src/datatypes.h @@ -722,5 +722,13 @@ void virConnectCloseCallbackDataCall(virConnectCloseCallbackDataPtr close, int reason); virConnectCloseFunc virConnectCloseCallbackDataGetCallback(virConnectCloseCallbackDataPtr close); +void virAdmConnectCloseCallbackDataReset(virAdmConnectCloseCallbackDataPtr cbdata); +int virAdmConnectCloseCallbackDataRegister(virAdmConnectCloseCallbackDataPtr cbdata, + virAdmConnectPtr conn, + virAdmConnectCloseFunc cb, + void *opaque, + virFreeCallback freecb); +int virAdmConnectCloseCallbackDataUnregister(virAdmConnectCloseCallbackDataPtr cbdata, + virAdmConnectCloseFunc cb); #endif /* __VIR_DATATYPES_H__ */ diff --git a/src/libvirt_admin_private.syms b/src/libvirt_admin_private.syms index 8c173ab..191e3f2 100644 --- a/src/libvirt_admin_private.syms +++ b/src/libvirt_admin_private.syms @@ -29,6 +29,9 @@ xdr_admin_server_set_threadpool_parameters_args; # datatypes.h virAdmClientClass; virAdmConnectClass; +virAdmConnectCloseCallbackDataRegister; +virAdmConnectCloseCallbackDataReset; +virAdmConnectCloseCallbackDataUnregister; virAdmGetServer; virAdmServerClass; -- 2.5.5
Use the newly introduced close callback helpers to make the code look just a bit cleaner and more importantly, to fix the following memleak regarding a dangling virAdmConnect object reference caused by assigning NULL to the close callback data once the catch-disconnect routine used the callback followed by a comparison of NULL to the originally defined close callback (which at that moment had already been NULL'd by remoteAdminClientCloseFunc) in virAdmConnectCloseCallbackUnregister. 717 (88 direct, 629 indirect) bytes in 1 blocks are definitely lost record 110 of 141 at 0x4C2A988: calloc (vg_replace_malloc.c:711) by 0x530696F: virAllocVar (viralloc.c:560) by 0x53689E6: virObjectNew (virobject.c:193) by 0x5368B5E: virObjectLockableNew (virobject.c:219) by 0x4E3E7EE: virAdmConnectNew (datatypes.c:900) by 0x4E398BB: virAdmConnectOpen (libvirt-admin.c:220) by 0x10D3E3: vshAdmConnect (virt-admin.c:161) by 0x10D624: vshAdmReconnect (virt-admin.c:215) by 0x10DB0A: cmdConnect (virt-admin.c:353) by 0x11288F: vshCommandRun (vsh.c:1313) by 0x10FDB6: main (virt-admin.c:1439) Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1357358 Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/admin/admin_remote.c | 6 +----- src/datatypes.c | 5 +---- src/libvirt-admin.c | 18 +----------------- 3 files changed, 3 insertions(+), 26 deletions(-) diff --git a/src/admin/admin_remote.c b/src/admin/admin_remote.c index 10a3b18..f37ff5e 100644 --- a/src/admin/admin_remote.c +++ b/src/admin/admin_remote.c @@ -144,11 +144,7 @@ remoteAdminClientCloseFunc(virNetClientPtr client ATTRIBUTE_UNUSED, VIR_DEBUG("Triggering connection close callback %p reason=%d, opaque=%p", cbdata->callback, reason, cbdata->opaque); cbdata->callback(cbdata->conn, reason, cbdata->opaque); - - if (cbdata->freeCallback) - cbdata->freeCallback(cbdata->opaque); - cbdata->callback = NULL; - cbdata->freeCallback = NULL; + virAdmConnectCloseCallbackDataReset(cbdata); } virObjectUnlock(cbdata); } diff --git a/src/datatypes.c b/src/datatypes.c index 24e4f77..c8a46b0 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -928,10 +928,7 @@ virAdmConnectCloseCallbackDataDispose(void *obj) virAdmConnectCloseCallbackDataPtr cb_data = obj; virObjectLock(cb_data); - - if (cb_data->freeCallback) - cb_data->freeCallback(cb_data->opaque); - + virAdmConnectCloseCallbackDataReset(cb_data); virObjectUnlock(cb_data); } diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c index 1b5fd44..8877e49 100644 --- a/src/libvirt-admin.c +++ b/src/libvirt-admin.c @@ -513,29 +513,13 @@ int virAdmConnectUnregisterCloseCallback(virAdmConnectPtr conn, virResetLastError(); virCheckAdmConnectReturn(conn, -1); - - virObjectLock(conn->closeCallback); - virCheckNonNullArgGoto(cb, error); - if (conn->closeCallback->callback != cb) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("A different callback was requested")); + if (virAdmConnectCloseCallbackDataUnregister(conn->closeCallback, cb) < 0) goto error; - } - - conn->closeCallback->callback = NULL; - if (conn->closeCallback->freeCallback) - conn->closeCallback->freeCallback(conn->closeCallback->opaque); - conn->closeCallback->freeCallback = NULL; - - virObjectUnlock(conn->closeCallback); - virObjectUnref(conn); return 0; - error: - virObjectUnlock(conn->closeCallback); virDispatchError(NULL); return -1; } -- 2.5.5
On 11.11.2016 16:32, Erik Skultety wrote:
This series fixes a memory leak in a dangling conn object reference caused by assigning NULL to conn->closeCallback object by the RPC catch-disconnect routine followed by a comparison of equality of conn->closeCallback->callback and the client-registered callback reference. Before fixing the issue itself, there are some new helpers introduced following the same idea as the helpers introduced by series https://www.redhat.com/archives/libvir-list/2016-February/msg00849.html
Erik Skultety (4): vsh: Drop conditional error reporting in vshErrorHandler admin: Remove unnecessary @conn object locking datatypes: Introduce some admin-related close callback handling helpers admin: Use the newly introduced close callback handling helpers
po/POTFILES.in | 1 + src/admin/admin_remote.c | 6 +--- src/datatypes.c | 67 +++++++++++++++++++++++++++++++++++++++--- src/datatypes.h | 8 +++++ src/libvirt-admin.c | 24 +-------------- src/libvirt_admin_private.syms | 3 ++ tools/vsh.c | 2 -- 7 files changed, 77 insertions(+), 34 deletions(-)
ACK series Michal
----- Original Message -----
From: "Michal Privoznik" <mprivozn@redhat.com> To: "Erik Skultety" <eskultet@redhat.com>, libvir-list@redhat.com Sent: Friday, November 11, 2016 4:51:38 PM Subject: Re: [libvirt] [PATCH 0/4] admin: Fix a leaking conn reference and other improvements
On 11.11.2016 16:32, Erik Skultety wrote:
This series fixes a memory leak in a dangling conn object reference caused by assigning NULL to conn->closeCallback object by the RPC catch-disconnect routine followed by a comparison of equality of conn->closeCallback->callback and the client-registered callback reference. Before fixing the issue itself, there are some new helpers introduced following the same idea as the helpers introduced by series https://www.redhat.com/archives/libvir-list/2016-February/msg00849.html
Erik Skultety (4): vsh: Drop conditional error reporting in vshErrorHandler admin: Remove unnecessary @conn object locking datatypes: Introduce some admin-related close callback handling helpers admin: Use the newly introduced close callback handling helpers
po/POTFILES.in | 1 + src/admin/admin_remote.c | 6 +--- src/datatypes.c | 67 +++++++++++++++++++++++++++++++++++++++--- src/datatypes.h | 8 +++++ src/libvirt-admin.c | 24 +-------------- src/libvirt_admin_private.syms | 3 ++ tools/vsh.c | 2 -- 7 files changed, 77 insertions(+), 34 deletions(-)
ACK series
Michal
Thanks, I pushed the series. Erik
participants (2)
-
Erik Skultety -
Michal Privoznik