[libvirt PATCH] remote: add mutex when freeing private callbacks
This commit resolves illegal memory accesses observed via: remoteClientFreePrivateCallbacks() remoteClientCloseFunc() virNetServerClientCloseLocked() virNetServerProcessClients() daemonServerProcessClients() virHashForEach() virNetDaemonRun() main() Signed-off-by: Mike Pontillo <mpontillo@digitalocean.com> --- src/remote/remote_daemon_dispatch.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index 250eb51e6b..8d043d0bce 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -1711,6 +1711,8 @@ remoteClientFreePrivateCallbacks(struct daemonClientPrivate *priv) g_autoptr(virIdentity) sysident = virIdentityGetSystem(); virIdentitySetCurrent(sysident); + virMutexLock(&priv->lock); + DEREG_CB(priv->conn, priv->domainEventCallbacks, priv->ndomainEventCallbacks, virConnectDomainEventDeregisterAny, "domain"); @@ -1737,6 +1739,8 @@ remoteClientFreePrivateCallbacks(struct daemonClientPrivate *priv) } virIdentitySetCurrent(NULL); + + virMutexUnlock(&priv->lock); } #undef DEREG_CB -- 2.34.1
On Mon, Mar 06, 2023 at 06:35:44PM +0000, Mike Pontillo wrote:
This commit resolves illegal memory accesses observed via:
remoteClientFreePrivateCallbacks() remoteClientCloseFunc() virNetServerClientCloseLocked() virNetServerProcessClients() daemonServerProcessClients() virHashForEach() virNetDaemonRun() main()
Signed-off-by: Mike Pontillo <mpontillo@digitalocean.com> --- src/remote/remote_daemon_dispatch.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index 250eb51e6b..8d043d0bce 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -1711,6 +1711,8 @@ remoteClientFreePrivateCallbacks(struct daemonClientPrivate *priv) g_autoptr(virIdentity) sysident = virIdentityGetSystem(); virIdentitySetCurrent(sysident);
+ virMutexLock(&priv->lock); +
Good catch, we can make it even slicker with: VIR_LOCK_GUARD lock = virLockGuardLock(&priv->lock); and rely on automatic unlock at the return. Either way (let me know if you're OK with the proposed change): Reviewed-by: Martin Kletzander <mkletzan@redhat.com> and I'll wait a bit for your answer before pushing.
DEREG_CB(priv->conn, priv->domainEventCallbacks, priv->ndomainEventCallbacks, virConnectDomainEventDeregisterAny, "domain"); @@ -1737,6 +1739,8 @@ remoteClientFreePrivateCallbacks(struct daemonClientPrivate *priv) }
virIdentitySetCurrent(NULL); + + virMutexUnlock(&priv->lock); } #undef DEREG_CB
-- 2.34.1
On Tue, Mar 7, 2023, 00:59 Martin Kletzander <mkletzan@redhat.com> wrote:
Good catch, we can make it even slicker with:
VIR_LOCK_GUARD lock = virLockGuardLock(&priv->lock);
and rely on automatic unlock at the return. Either way (let me know if you're OK with the proposed change):
Thanks for the review; I'll look at amending the patch to do this. (This patch was originally written for a version of libvirt where this macro did not yet exist!) Thanks, Mike
On Tue, Mar 07, 2023 at 08:22:25AM -0800, Mike Pontillo wrote:
On Tue, Mar 7, 2023, 00:59 Martin Kletzander <mkletzan@redhat.com> wrote:
Good catch, we can make it even slicker with:
VIR_LOCK_GUARD lock = virLockGuardLock(&priv->lock);
and rely on automatic unlock at the return. Either way (let me know if you're OK with the proposed change):
Thanks for the review; I'll look at amending the patch to do this. (This patch was originally written for a version of libvirt where this macro did not yet exist!)
I can do that before pushing if you are OK with it, I'll keep your S-o-b.
Thanks, Mike
participants (2)
-
Martin Kletzander -
Mike Pontillo