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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
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 :|
