9:32 a.m.
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
9:32 a.m.
New subject: [libvirt] [PATCH 1/4] vsh: Drop conditional error reporting in vshErrorHandler
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(a)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
9:32 a.m.
New subject: [libvirt] [PATCH 2/4] admin: Remove unnecessary @conn object locking
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(a)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
9:51 a.m.
New subject: [libvirt] [PATCH 2/4] admin: Remove unnecessary @conn object locking
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(a)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
9:32 a.m.
New subject: [libvirt] [PATCH 3/4] datatypes: Introduce some admin-related close callback handling helpers
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(a)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
9:32 a.m.
New subject: [libvirt] [PATCH 4/4] admin: Use the newly introduced close callback handling helpers
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(a)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
9:51 a.m.
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
3:27 a.m.
----- Original Message -----
From: "Michal Privoznik" <mprivozn(a)redhat.com>
To: "Erik Skultety" <eskultet(a)redhat.com>, libvir-list(a)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
2930
days inactive
2933
days old
7 comments
2 participants
participants (2)
-
Erik Skultety -
Michal Privoznik