ui: add standalone VNC server over D-Bus

[PATCH 01/60] ui/vnc-jobs: fix VncRectEntry leak on job cleanup

Posted by Marc-André Lureau 2 weeks, 6 days ago

When a VncJob is freed, its associated VncRectEntry list must also be
freed. Previously, vnc_job_push() and the disconnected path in
vnc_worker_thread_loop() called g_free(job) directly, leaking all
VncRectEntry allocations.

Introduce vnc_job_free() which iterates and frees the rectangle entries
before freeing the job itself, and use it in both paths.

Also add QLIST_REMOVE() in the worker loop before g_free(entry), so
that entries processed during normal operation are properly unlinked.
Without this, vnc_job_free() would iterate dangling pointers to
already-freed entries, causing use-after-free.

Fixes: bd023f953e5e ("vnc: threaded VNC server")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/vnc-jobs.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
index b296d19e089..ca625da6d05 100644
--- a/ui/vnc-jobs.c
+++ b/ui/vnc-jobs.c
@@ -107,11 +107,25 @@ int vnc_job_add_rect(VncJob *job, int x, int y, int w, int h)
     return 1;
 }
 
+static void vnc_job_free(VncJob *job)
+{
+    VncRectEntry *entry, *tmp;
+
+    if (!job) {
+        return;
+    }
+    QLIST_FOREACH_SAFE(entry, &job->rectangles, next, tmp) {
+        /* no need for QLIST_REMOVE(entry, next) */
+        g_free(entry);
+    }
+    g_free(job);
+}
+
 void vnc_job_push(VncJob *job)
 {
     vnc_lock_queue(queue);
     if (queue->exit || QLIST_EMPTY(&job->rectangles)) {
-        g_free(job);
+        vnc_job_free(job);
     } else {
         QTAILQ_INSERT_TAIL(&queue->jobs, job, next);
         qemu_cond_broadcast(&queue->cond);
@@ -296,6 +310,7 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
                 n_rectangles += n;
             }
         }
+        QLIST_REMOVE(entry, next);
         g_free(entry);
     }
     trace_vnc_job_nrects(&vs, job, n_rectangles);
@@ -324,7 +339,7 @@ disconnected:
     QTAILQ_REMOVE(&queue->jobs, job, next);
     vnc_unlock_queue(queue);
     qemu_cond_broadcast(&queue->cond);
-    g_free(job);
+    vnc_job_free(job);
     vs.magic = 0;
     return 0;
 }

-- 
2.53.0

Re: [PATCH 01/60] ui/vnc-jobs: fix VncRectEntry leak on job cleanup

Posted by Michael Tokarev 6 days, 12 hours ago

On 17.03.2026 11:50, Marc-André Lureau wrote:
> When a VncJob is freed, its associated VncRectEntry list must also be
> freed. Previously, vnc_job_push() and the disconnected path in
> vnc_worker_thread_loop() called g_free(job) directly, leaking all
> VncRectEntry allocations.
> 
> Introduce vnc_job_free() which iterates and frees the rectangle entries
> before freeing the job itself, and use it in both paths.
> 
> Also add QLIST_REMOVE() in the worker loop before g_free(entry), so
> that entries processed during normal operation are properly unlinked.
> Without this, vnc_job_free() would iterate dangling pointers to
> already-freed entries, causing use-after-free.
> 
> Fixes: bd023f953e5e ("vnc: threaded VNC server")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   ui/vnc-jobs.c | 19 +++++++++++++++++--
>   1 file changed, 17 insertions(+), 2 deletions(-)

I'm picking this up for qemu-stable.  Please let me know if I shouldn't.

Thanks,

/mjt

Re: [PATCH 01/60] ui/vnc-jobs: fix VncRectEntry leak on job cleanup

Posted by Daniel P. Berrangé 1 week, 6 days ago

On Tue, Mar 17, 2026 at 12:50:15PM +0400, Marc-André Lureau wrote:
> When a VncJob is freed, its associated VncRectEntry list must also be
> freed. Previously, vnc_job_push() and the disconnected path in
> vnc_worker_thread_loop() called g_free(job) directly, leaking all
> VncRectEntry allocations.
> 
> Introduce vnc_job_free() which iterates and frees the rectangle entries
> before freeing the job itself, and use it in both paths.
> 
> Also add QLIST_REMOVE() in the worker loop before g_free(entry), so
> that entries processed during normal operation are properly unlinked.
> Without this, vnc_job_free() would iterate dangling pointers to
> already-freed entries, causing use-after-free.
> 
> Fixes: bd023f953e5e ("vnc: threaded VNC server")

... Oppps ...  Wed Jul 7 2010 


> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  ui/vnc-jobs.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)

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


With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|