From: Liu Song <liu.song13@zte.com.cn>

The ff callback is never called in remoteStreamCallbackFree() because
cbdata->cb can not be NULL. This causes a leak of 'cbdata->opaque'.

The leak can be reproduced by attaching and detaching to the console of
an VM using `virsh console`.

ASAN reports the leak stack as:
Direct leak of 288 byte(s) in 1 object(s) allocated from:
    #0 0x7f6edf6ba0c7 in calloc (/lib64/libasan.so.8+0xba0c7)
    #1 0x7f6edf5175b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x615b0)
    #2 0x7f6ede6d0be3 in g_type_create_instance (/lib64/libgobject-2.0.so.0+0x3cbe3)
    #3 0x7f6ede6b82cf in g_object_new_internal (/lib64/libgobject-2.0.so.0+0x242cf)
    #4 0x7f6ede6b9877 in g_object_new_with_properties (/lib64/libgobject-2.0.so.0+0x25877)
    #5 0x7f6ede6ba620 in g_object_new (/lib64/libgobject-2.0.so.0+0x26620)
    #6 0x7f6edeb78138 in virObjectNew ../src/util/virobject.c:252
    #7 0x7f6edeb7a78b in virObjectLockableNew ../src/util/virobject.c:274
    #8 0x558251e427e1 in virConsoleNew ../tools/virsh-console.c:369
    #9 0x558251e427e1 in virshRunConsole ../tools/virsh-console.c:427

Signed-off-by: Liu Song <liu.song13@zte.com.cn>
---
Changes in v2:
- Fixed email formatting to ensure patch applies correctly

 src/remote/remote_daemon_stream.c | 2 +-
 src/remote/remote_driver.c        | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/remote/remote_daemon_stream.c b/src/remote/remote_daemon_stream.c
index 453728a66b..a5032f9a43 100644
--- a/src/remote/remote_daemon_stream.c
+++ b/src/remote/remote_daemon_stream.c
@@ -437,13 +437,13 @@ int daemonAddClientStream(virNetServerClient *client,
         return -1;
     }

+    virObjectRef(client);
     if (virStreamEventAddCallback(stream->st, 0,
                                   daemonStreamEvent, client,
                                   virObjectUnref) < 0)
         return -1;

     virObjectRef(client);
-
     if ((stream->filterID = virNetServerClientAddFilter(client,
                                                         daemonStreamFilter,
                                                         stream)) < 0) {
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 2690c05267..9ac13469e9 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -5336,7 +5336,7 @@ static void remoteStreamCallbackFree(void *opaque)
 {
     struct remoteStreamCallbackData *cbdata = opaque;

-    if (!cbdata->cb && cbdata->ff)
+    if (cbdata->ff)
         (cbdata->ff)(cbdata->opaque);

     virObjectUnref(cbdata->st);
-- 
2.27.0

On Wed, Jun 25, 2025 at 09:27:33 +0800, liu.xuemei1@zte.com.cn wrote:
> From: Liu Song <liu.song13@zte.com.cn>
> 
> The ff callback is never called in remoteStreamCallbackFree() because
> cbdata->cb can not be NULL. This causes a leak of 'cbdata->opaque'.
> 
> The leak can be reproduced by attaching and detaching to the console of
> an VM using `virsh console`.
> 
> ASAN reports the leak stack as:
> Direct leak of 288 byte(s) in 1 object(s) allocated from:
>     #0 0x7f6edf6ba0c7 in calloc (/lib64/libasan.so.8+0xba0c7)
>     #1 0x7f6edf5175b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x615b0)
>     #2 0x7f6ede6d0be3 in g_type_create_instance (/lib64/libgobject-2.0.so.0+0x3cbe3)
>     #3 0x7f6ede6b82cf in g_object_new_internal (/lib64/libgobject-2.0.so.0+0x242cf)
>     #4 0x7f6ede6b9877 in g_object_new_with_properties (/lib64/libgobject-2.0.so.0+0x25877)
>     #5 0x7f6ede6ba620 in g_object_new (/lib64/libgobject-2.0.so.0+0x26620)
>     #6 0x7f6edeb78138 in virObjectNew ../src/util/virobject.c:252
>     #7 0x7f6edeb7a78b in virObjectLockableNew ../src/util/virobject.c:274
>     #8 0x558251e427e1 in virConsoleNew ../tools/virsh-console.c:369
>     #9 0x558251e427e1 in virshRunConsole ../tools/virsh-console.c:427
> 
> Signed-off-by: Liu Song <liu.song13@zte.com.cn>
> ---
> Changes in v2:
> - Fixed email formatting to ensure patch applies correctly
> 
>  src/remote/remote_daemon_stream.c | 2 +-
>  src/remote/remote_driver.c        | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/remote/remote_daemon_stream.c b/src/remote/remote_daemon_stream.c
> index 453728a66b..a5032f9a43 100644
> --- a/src/remote/remote_daemon_stream.c
> +++ b/src/remote/remote_daemon_stream.c
> @@ -437,13 +437,13 @@ int daemonAddClientStream(virNetServerClient *client,
>          return -1;
>      }
> 
> +    virObjectRef(client);
>      if (virStreamEventAddCallback(stream->st, 0,
>                                    daemonStreamEvent, client,
>                                    virObjectUnref) < 0)
>          return -1;
> 
>      virObjectRef(client);
> -
>      if ((stream->filterID = virNetServerClientAddFilter(client,

The above looks weird. Now 'client' is referenced twice and you delete
an empty line?

> On Wed, Jun 25, 2025 at 09:27:33 +0800, liu.xuemei1(a)zte.com.cn wrote:
> 
> The above looks weird. Now 'client' is referenced twice and you delete
> an empty line?
Since the patch fixes the virStreamEventAddCallback (not freeing its 'opaque'),
'client' should be referenced before every call of it. I thought the second was
for virNetServerClientAddFilter and tried to make them more associative.

However it seems no matter the second reference exists or not, there is no leak
or UAF reported. I'll remove it in next version if I confirm this.

On Thu, Jun 26, 2025 at 03:53:35 -0000, liu.song13@zte.com.cn wrote:
> > On Wed, Jun 25, 2025 at 09:27:33 +0800, liu.xuemei1(a)zte.com.cn wrote:
> > 
> > The above looks weird. Now 'client' is referenced twice and you delete
> > an empty line?
> Since the patch fixes the virStreamEventAddCallback (not freeing its 'opaque'),
> 'client' should be referenced before every call of it. I thought the second was
> for virNetServerClientAddFilter and tried to make them more associative.
> 
> However it seems no matter the second reference exists or not, there is no leak
> or UAF reported. I'll remove it in next version if I confirm this.

Having two references this way would look confusing, so if you decide
that it is necessary please add a comment explaining why it is
necessary.

In this patch it looked extra confusing as you deleted an empty line
right after the second reference which looked like you wanted to just
move the reference.

> On Thu, Jun 26, 2025 at 03:53:35 -0000, liu.song13(a)zte.com.cn wrote:
> 
> Having two references this way would look confusing, so if you decide
> that it is necessary please add a comment explaining why it is
> necessary.
> 
> In this patch it looked extra confusing as you deleted an empty line
> right after the second reference which looked like you wanted to just
> move the reference.
I see, I didn't realize that the orginal reference is for
virStreamEventAddCallback, because it's after the call.

However the code here might be a little confusing and risky, the close events
may come early and unreference 'client' before we reference it. It might be
better to put the reference before virStreamEventAddCallback.