[PATCH v2] remote/stream-event: Fix a memory leak in remoteStreamCallbackFree()
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.
participants (3)
-
liu.song13＠zte.com.cn -
liu.xuemei1＠zte.com.cn -
Peter Krempa