Hi Akihiko
On Tue, Jun 3, 2025 at 1:19 PM Akihiko Odaki
<od...@rsg.ci.i.u-tokyo.ac.jp> wrote:
>
> vnc_worker_thread_loop() copies z_stream stored in its local VncState to
> the persistent VncState, and the copied one is freed with deflateEnd()
> later. However, deflateEnd() refuses to operate with a copied z_stream
> and returns Z_STREAM_ERROR, leaking the allocated memory.
>
> Avoid copying the zlib state to fix the memory leak.
>
> Fixes: bd023f953e5e ("vnc: threaded VNC server")
> Signed-off-by: Akihiko Odaki <od...@rsg.ci.i.u-tokyo.ac.jp>
lgtm,
Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com>
I wonder if VncZlib could be created and owned exclusively by the
worker thread though.
It looks like we could drop this too:
--- a/ui/vnc-jobs.c
+++ b/ui/vnc-jobs.c
@@ -201,7 +201,6 @@ static void vnc_async_encoding_end(VncState *orig,
VncState *local)
{
buffer_free(&local->output);
orig->tight = local->tight;
- orig->zlib = local->zlib;
> ---
> ui/vnc.h | 2 +-
> ui/vnc-enc-zlib.c | 30 +++++++++++++++---------------
> ui/vnc.c | 13 ++++++++++---
> 3 files changed, 26 insertions(+), 19 deletions(-)
>
> diff --git a/ui/vnc.h b/ui/vnc.h
> index
> 02613aa63a170901734a04aee84abff7b58d8736..82b883bb69fa5ce331945dcffe25588d5fd73f50
> 100644
> --- a/ui/vnc.h
> +++ b/ui/vnc.h
> @@ -340,7 +340,7 @@ struct VncState
> * update vnc_async_encoding_start()
> */
> VncTight *tight;
> - VncZlib zlib;
> + VncZlib *zlib;
> VncHextile hextile;
> VncZrle *zrle;
> VncZywrle zywrle;
> diff --git a/ui/vnc-enc-zlib.c b/ui/vnc-enc-zlib.c
> index
> 900ae5b30f6bd2ddbcd797d212c010c48d451094..52e9193eab572a79733b11c89bde81daf01679e7
> 100644
> --- a/ui/vnc-enc-zlib.c
> +++ b/ui/vnc-enc-zlib.c
> @@ -48,21 +48,21 @@ void vnc_zlib_zfree(void *x, void *addr)
>
> static void vnc_zlib_start(VncState *vs)
> {
> - buffer_reset(&vs->zlib.zlib);
> + buffer_reset(&vs->zlib->zlib);
>
> // make the output buffer be the zlib buffer, so we can compress it later
> - vs->zlib.tmp = vs->output;
> - vs->output = vs->zlib.zlib;
> + vs->zlib->tmp = vs->output;
> + vs->output = vs->zlib->zlib;
> }
>
> static int vnc_zlib_stop(VncState *vs)
> {
> - z_streamp zstream = &vs->zlib.stream;
> + z_streamp zstream = &vs->zlib->stream;
> int previous_out;
>
> // switch back to normal output/zlib buffers
> - vs->zlib.zlib = vs->output;
> - vs->output = vs->zlib.tmp;
> + vs->zlib->zlib = vs->output;
> + vs->output = vs->zlib->tmp;
>
> // compress the zlib buffer
>
> @@ -85,24 +85,24 @@ static int vnc_zlib_stop(VncState *vs)
> return -1;
> }
>
> - vs->zlib.level = vs->tight->compression;
> + vs->zlib->level = vs->tight->compression;
> zstream->opaque = vs;
> }
>
> - if (vs->tight->compression != vs->zlib.level) {
> + if (vs->tight->compression != vs->zlib->level) {
> if (deflateParams(zstream, vs->tight->compression,
> Z_DEFAULT_STRATEGY) != Z_OK) {
> return -1;
> }
> - vs->zlib.level = vs->tight->compression;
> + vs->zlib->level = vs->tight->compression;
> }
>
> // reserve memory in output buffer
> - buffer_reserve(&vs->output, vs->zlib.zlib.offset + 64);
> + buffer_reserve(&vs->output, vs->zlib->zlib.offset + 64);
>
> // set pointers
> - zstream->next_in = vs->zlib.zlib.buffer;
> - zstream->avail_in = vs->zlib.zlib.offset;
> + zstream->next_in = vs->zlib->zlib.buffer;
> + zstream->avail_in = vs->zlib->zlib.offset;
> zstream->next_out = vs->output.buffer + vs->output.offset;
> zstream->avail_out = vs->output.capacity - vs->output.offset;
> previous_out = zstream->avail_out;
> @@ -147,8 +147,8 @@ int vnc_zlib_send_framebuffer_update(VncState *vs, int x,
> int y, int w, int h)
>
> void vnc_zlib_clear(VncState *vs)
> {
> - if (vs->zlib.stream.opaque) {
> - deflateEnd(&vs->zlib.stream);
> + if (vs->zlib->stream.opaque) {
> + deflateEnd(&vs->zlib->stream);
> }
> - buffer_free(&vs->zlib.zlib);
> + buffer_free(&vs->zlib->zlib);
> }
> diff --git a/ui/vnc.c b/ui/vnc.c
> index
> d095cd7da31e2fe0d7241894b7ed5d2cbb21f72c..59009ff61b350487153960d0236eb438f93e665b
> 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -56,6 +56,11 @@
> #include "io/dns-resolver.h"
> #include "monitor/monitor.h"
>
> +typedef struct VncConnection {
> + VncState vs;
> + VncZlib zlib;
> +} VncConnection;
> +
> #define VNC_REFRESH_INTERVAL_BASE GUI_REFRESH_INTERVAL_DEFAULT
> #define VNC_REFRESH_INTERVAL_INC 50
> #define VNC_REFRESH_INTERVAL_MAX GUI_REFRESH_INTERVAL_IDLE
> @@ -1362,7 +1367,7 @@ void vnc_disconnect_finish(VncState *vs)
> vs->magic = 0;
> g_free(vs->zrle);
> g_free(vs->tight);
> - g_free(vs);
> + g_free(container_of(vs, VncConnection, vs));
> }
>
> size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error *err)
> @@ -3241,11 +3246,13 @@ static void vnc_refresh(DisplayChangeListener *dcl)
> static void vnc_connect(VncDisplay *vd, QIOChannelSocket *sioc,
> bool skipauth, bool websocket)
> {
> - VncState *vs = g_new0(VncState, 1);
> + VncConnection *vc = g_new0(VncConnection, 1);
> + VncState *vs = &vc->vs;
> bool first_client = QTAILQ_EMPTY(&vd->clients);
> int i;
>
> trace_vnc_client_connect(vs, sioc);
> + vs->zlib = &vc->zlib;
> vs->zrle = g_new0(VncZrle, 1);
> vs->tight = g_new0(VncTight, 1);
> vs->magic = VNC_MAGIC;
> @@ -3268,7 +3275,7 @@ static void vnc_connect(VncDisplay *vd,
> QIOChannelSocket *sioc,
> #ifdef CONFIG_PNG
> buffer_init(&vs->tight->png, "vnc-tight-png/%p", sioc);
> #endif
> - buffer_init(&vs->zlib.zlib, "vnc-zlib/%p", sioc);
> + buffer_init(&vc->zlib.zlib, "vnc-zlib/%p", sioc);
> buffer_init(&vs->zrle->zrle, "vnc-zrle/%p", sioc);
> buffer_init(&vs->zrle->fb, "vnc-zrle-fb/%p", sioc);
> buffer_init(&vs->zrle->zlib, "vnc-zrle-zlib/%p", sioc);
>
> --
> 2.49.0
>
>
--
Marc-André Lureau
