[PATCH 0/4] virIdentity: Don't leak params in virGetConnectGeneric and refactor
Fix a memleak and make the code a bit neater. Peter Krempa (4): util: typedparam: Introduce virTypedParamListFromParams virGetConnectGeneric: Fix memleak of 'identparams' when connecting between split daemons virIdentityGetParameters: Return 'virTypedParamList' doRemoteOpen: Refactor control flow src/driver.c | 8 +++---- src/libvirt_private.syms | 1 + src/remote/remote_daemon_dispatch.c | 36 ++++++++++++----------------- src/util/viridentity.c | 15 ++++-------- src/util/viridentity.h | 5 ++-- src/util/virtypedparam.c | 13 +++++++++++ src/util/virtypedparam.h | 4 ++++ 7 files changed, 44 insertions(+), 38 deletions(-) -- 2.35.3
The helper constructs a virTypedParamList from loose params. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virtypedparam.c | 13 +++++++++++++ src/util/virtypedparam.h | 4 ++++ 3 files changed, 18 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d95c181793..3711e33cf5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3493,6 +3493,7 @@ virTypedParamListAddString; virTypedParamListAddUInt; virTypedParamListAddULLong; virTypedParamListFree; +virTypedParamListFromParams; virTypedParamListStealParams; virTypedParamsCheck; virTypedParamsCopy; diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index aa6a871049..2d7e4ab354 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -738,6 +738,19 @@ virTypedParamListStealParams(virTypedParamList *list, return ret; } +virTypedParamList * +virTypedParamListFromParams(virTypedParameterPtr *params, + size_t nparams) +{ + virTypedParamList *l = g_new0(virTypedParamList, 1); + + l->par = g_steal_pointer(params); + l->npar = nparams; + l->par_alloc = nparams; + + return l; +} + static int G_GNUC_PRINTF(2, 0) virTypedParamSetNameVPrintf(virTypedParameterPtr par, diff --git a/src/util/virtypedparam.h b/src/util/virtypedparam.h index f4b3921c38..c4bc58ee8f 100644 --- a/src/util/virtypedparam.h +++ b/src/util/virtypedparam.h @@ -138,6 +138,10 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(virTypedParamList, virTypedParamListFree); size_t virTypedParamListStealParams(virTypedParamList *list, virTypedParameterPtr *params); +virTypedParamList * +virTypedParamListFromParams(virTypedParameterPtr *params, + size_t nparams); + int virTypedParamListAddInt(virTypedParamList *list, int value, const char *namefmt, -- 2.35.3
The 'identparams' typed parameter list obtained from virIdentityGetParameters is leaked when called from 'virGetConnectGeneric'. Use 'virTypedParamListFromParams' to absorb it into a virTypedParamList which can be autofreed. Note that the memleak is observable only when running in split-daemon mode. Closes: https://gitlab.com/libvirt/libvirt/-/issues/314 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/driver.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/driver.c b/src/driver.c index 41b4f0055b..d4136c438f 100644 --- a/src/driver.c +++ b/src/driver.c @@ -36,6 +36,7 @@ #include "viridentity.h" #include "datatypes.h" #include "configmake.h" +#include "virtypedparam.h" VIR_LOG_INIT("driver"); @@ -159,6 +160,7 @@ virGetConnectGeneric(virThreadLocal *threadPtr, const char *name) if (conn->driver->connectSetIdentity != NULL) { g_autoptr(virIdentity) ident = NULL; + g_autoptr(virTypedParamList) paramlist = NULL; virTypedParameterPtr identparams = NULL; int nidentparams = 0; @@ -169,7 +171,9 @@ virGetConnectGeneric(virThreadLocal *threadPtr, const char *name) if (virIdentityGetParameters(ident, &identparams, &nidentparams) < 0) goto error; - if (virConnectSetIdentity(conn, identparams, nidentparams, 0) < 0) + paramlist = virTypedParamListFromParams(&identparams, nidentparams); + + if (virConnectSetIdentity(conn, paramlist->par, paramlist->npar, 0) < 0) goto error; } } -- 2.35.3
Refactor the code to use virTypedParamList which simplifies cleanup. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/driver.c | 10 +++------- src/remote/remote_daemon_dispatch.c | 8 +++----- src/util/viridentity.c | 15 +++++---------- src/util/viridentity.h | 5 ++--- 4 files changed, 13 insertions(+), 25 deletions(-) diff --git a/src/driver.c b/src/driver.c index d4136c438f..cea74bdf95 100644 --- a/src/driver.c +++ b/src/driver.c @@ -160,20 +160,16 @@ virGetConnectGeneric(virThreadLocal *threadPtr, const char *name) if (conn->driver->connectSetIdentity != NULL) { g_autoptr(virIdentity) ident = NULL; - g_autoptr(virTypedParamList) paramlist = NULL; - virTypedParameterPtr identparams = NULL; - int nidentparams = 0; + g_autoptr(virTypedParamList) identparams = NULL; VIR_DEBUG("Attempting to delegate current identity"); if (!(ident = virIdentityGetCurrent())) goto error; - if (virIdentityGetParameters(ident, &identparams, &nidentparams) < 0) + if (!(identparams = virIdentityGetParameters(ident))) goto error; - paramlist = virTypedParamListFromParams(&identparams, nidentparams); - - if (virConnectSetIdentity(conn, paramlist->par, paramlist->npar, 0) < 0) + if (virConnectSetIdentity(conn, identparams->par, identparams->npar, 0) < 0) goto error; } } diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index 2463386e39..39953f46cf 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -1793,8 +1793,7 @@ remoteOpenConn(const char *uri, bool preserveIdentity, virConnectPtr *conn) { - virTypedParameterPtr params = NULL; - int nparams = 0; + g_autoptr(virTypedParamList) identparams = NULL; int ret = -1; VIR_DEBUG("Getting secondary uri=%s readonly=%d preserveIdent=%d conn=%p", @@ -1814,7 +1813,7 @@ remoteOpenConn(const char *uri, if (!(ident = virIdentityGetCurrent())) return -1; - if (virIdentityGetParameters(ident, ¶ms, &nparams) < 0) + if (!(identparams = virIdentityGetParameters(ident))) goto error; } @@ -1828,7 +1827,7 @@ remoteOpenConn(const char *uri, VIR_DEBUG("Opened driver %p", *conn); if (preserveIdentity) { - if (virConnectSetIdentity(*conn, params, nparams, 0) < 0) + if (virConnectSetIdentity(*conn, identparams->par, identparams->npar, 0) < 0) goto error; VIR_DEBUG("Forwarded current identity to secondary driver"); @@ -1836,7 +1835,6 @@ remoteOpenConn(const char *uri, ret = 0; cleanup: - virTypedParamsFree(params, nparams); return ret; error: diff --git a/src/util/viridentity.c b/src/util/viridentity.c index 70843ecf9f..e3a9cbb661 100644 --- a/src/util/viridentity.c +++ b/src/util/viridentity.c @@ -838,17 +838,12 @@ int virIdentitySetParameters(virIdentity *ident, } -int virIdentityGetParameters(virIdentity *ident, - virTypedParameterPtr *params, - int *nparams) +virTypedParamList *virIdentityGetParameters(virIdentity *ident) { - *params = NULL; - *nparams = 0; + virTypedParameter *tmp = NULL; - if (virTypedParamsCopy(params, ident->params, ident->nparams) < 0) - return -1; - - *nparams = ident->nparams; + if (virTypedParamsCopy(&tmp, ident->params, ident->nparams) < 0) + return NULL; - return 0; + return virTypedParamListFromParams(&tmp, ident->nparams); } diff --git a/src/util/viridentity.h b/src/util/viridentity.h index 6da6d0c557..5f87d7268b 100644 --- a/src/util/viridentity.h +++ b/src/util/viridentity.h @@ -23,6 +23,7 @@ #include "internal.h" #include <glib-object.h> +#include "virtypedparam.h" #define VIR_TYPE_IDENTITY vir_identity_get_type() G_DECLARE_FINAL_TYPE(virIdentity, vir_identity, VIR, IDENTITY, GObject); @@ -88,6 +89,4 @@ int virIdentitySetParameters(virIdentity *ident, virTypedParameterPtr params, int nparams); -int virIdentityGetParameters(virIdentity *ident, - virTypedParameterPtr *params, - int *nparams); +virTypedParamList *virIdentityGetParameters(virIdentity *ident); -- 2.35.3
Use a temporary variable 'newconn' to hold the newly opened connection until we are ready to pass it back instead of the original connection. This way we can avoid complicated 'error'/'cleanup' sections. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/remote/remote_daemon_dispatch.c | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index 39953f46cf..c1f85925a3 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -1794,7 +1794,7 @@ remoteOpenConn(const char *uri, virConnectPtr *conn) { g_autoptr(virTypedParamList) identparams = NULL; - int ret = -1; + g_autoptr(virConnect) newconn = NULL; VIR_DEBUG("Getting secondary uri=%s readonly=%d preserveIdent=%d conn=%p", NULLSTR(uri), readonly, preserveIdentity, conn); @@ -1814,34 +1814,30 @@ remoteOpenConn(const char *uri, return -1; if (!(identparams = virIdentityGetParameters(ident))) - goto error; + return -1; } VIR_DEBUG("Opening driver %s", uri); if (readonly) - *conn = virConnectOpenReadOnly(uri); + newconn = virConnectOpenReadOnly(uri); else - *conn = virConnectOpen(uri); - if (!*conn) - goto error; - VIR_DEBUG("Opened driver %p", *conn); + newconn = virConnectOpen(uri); + + if (!newconn) + return -1; + + VIR_DEBUG("Opened driver %p", newconn); if (preserveIdentity) { if (virConnectSetIdentity(*conn, identparams->par, identparams->npar, 0) < 0) - goto error; + return -1; VIR_DEBUG("Forwarded current identity to secondary driver"); } - ret = 0; - cleanup: - return ret; + *conn = g_steal_pointer(&newconn); - error: - if (*conn) { - g_clear_pointer(conn, virConnectClose); - } - goto cleanup; + return 0; } -- 2.35.3
On Wed, May 18, 2022 at 11:22:09AM +0200, Peter Krempa wrote:
Fix a memleak and make the code a bit neater.
Peter Krempa (4): util: typedparam: Introduce virTypedParamListFromParams virGetConnectGeneric: Fix memleak of 'identparams' when connecting between split daemons virIdentityGetParameters: Return 'virTypedParamList' doRemoteOpen: Refactor control flow
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
participants (2)
-
Pavel Hrdina -
Peter Krempa