In ideal world, where clients close connection gracefully their
SASL session is freed in virNetServerClientDispose() as it's
stored in client->sasl. Unfortunately, if client connection is
closed prematurely (e.g. the moment virsh asks for credentials),
the _virNetServerClient member is never set and corresponding
SASL session is never freed. The handler is still stored in
client private data, so free it in remoteClientCloseFunc().

  20,862 (288 direct, 20,574 indirect) bytes in 3 blocks are definitely lost in loss record 1,763 of 1,772
     at 0x50390C4: g_type_create_instance (in /usr/lib64/libgobject-2.0.so.0.7800.6)
     by 0x501BDAF: g_object_new_internal.part.0 (in /usr/lib64/libgobject-2.0.so.0.7800.6)
     by 0x501D43D: g_object_new_with_properties (in /usr/lib64/libgobject-2.0.so.0.7800.6)
     by 0x501E318: g_object_new (in /usr/lib64/libgobject-2.0.so.0.7800.6)
     by 0x49BAA63: virObjectNew (virobject.c:252)
     by 0x49BABC6: virObjectLockableNew (virobject.c:274)
     by 0x4B0526C: virNetSASLSessionNewServer (virnetsaslcontext.c:230)
     by 0x18EEFC: remoteDispatchAuthSaslInit (remote_daemon_dispatch.c:3696)
     by 0x15E128: remoteDispatchAuthSaslInitHelper (remote_daemon_dispatch_stubs.h:74)
     by 0x4B0FA5E: virNetServerProgramDispatchCall (virnetserverprogram.c:423)
     by 0x4B0F591: virNetServerProgramDispatch (virnetserverprogram.c:299)
     by 0x4B18AE3: virNetServerProcessMsg (virnetserver.c:135)

Signed-off-by: Michal Privoznik <mprivozn@redhat.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 cfc1067e40..c88c9f5b15 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -1776,6 +1776,10 @@ static void remoteClientCloseFunc(virNetServerClient *client)
     daemonRemoveAllClientStreams(priv->streams);
 
     remoteClientFreePrivateCallbacks(priv);
+
+#if WITH_SASL
+    virObjectUnref(priv->sasl);
+#endif
 }
 
 
-- 
2.44.2

On Fri, Jun 14, 2024 at 03:07:01PM +0200, Michal Privoznik wrote:
> In ideal world, where clients close connection gracefully their
> SASL session is freed in virNetServerClientDispose() as it's
> stored in client->sasl. Unfortunately, if client connection is
> closed prematurely (e.g. the moment virsh asks for credentials),
> the _virNetServerClient member is never set and corresponding
> SASL session is never freed. The handler is still stored in
> client private data, so free it in remoteClientCloseFunc().
> 
>   20,862 (288 direct, 20,574 indirect) bytes in 3 blocks are definitely lost in loss record 1,763 of 1,772
>      at 0x50390C4: g_type_create_instance (in /usr/lib64/libgobject-2.0.so.0.7800.6)
>      by 0x501BDAF: g_object_new_internal.part.0 (in /usr/lib64/libgobject-2.0.so.0.7800.6)
>      by 0x501D43D: g_object_new_with_properties (in /usr/lib64/libgobject-2.0.so.0.7800.6)
>      by 0x501E318: g_object_new (in /usr/lib64/libgobject-2.0.so.0.7800.6)
>      by 0x49BAA63: virObjectNew (virobject.c:252)
>      by 0x49BABC6: virObjectLockableNew (virobject.c:274)
>      by 0x4B0526C: virNetSASLSessionNewServer (virnetsaslcontext.c:230)
>      by 0x18EEFC: remoteDispatchAuthSaslInit (remote_daemon_dispatch.c:3696)
>      by 0x15E128: remoteDispatchAuthSaslInitHelper (remote_daemon_dispatch_stubs.h:74)
>      by 0x4B0FA5E: virNetServerProgramDispatchCall (virnetserverprogram.c:423)
>      by 0x4B0F591: virNetServerProgramDispatch (virnetserverprogram.c:299)
>      by 0x4B18AE3: virNetServerProcessMsg (virnetserver.c:135)
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.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 cfc1067e40..c88c9f5b15 100644
> --- a/src/remote/remote_daemon_dispatch.c
> +++ b/src/remote/remote_daemon_dispatch.c
> @@ -1776,6 +1776,10 @@ static void remoteClientCloseFunc(virNetServerClient *client)
>      daemonRemoveAllClientStreams(priv->streams);
>  
>      remoteClientFreePrivateCallbacks(priv);
> +
> +#if WITH_SASL
> +    virObjectUnref(priv->sasl);
> +#endif
>  }

Since the virNetServerClient may live on for a bit after CloseFunc is
called, I'd be happier with

    g_clear_pointer(&priv->sasl, virObjectUnref);

to prevent an accidental use after free later. With that changed

  Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|