[v1] hw/display: don't accidentally autofree existing virgl resources

[RFC PATCH] hw/display: don't accidentally autofree existing virgl resources

Posted by Alex Bennée 1 month, 1 week ago

While sanity checking a create blob operation the use of the auto
freed res variable could lead to inadvertently freeing an existing
blob.

Avoid this by in-lining the virtio_gpu_virgl_find_resource() check as
the value is not needed anyway.

While at it add a comment to the end and use g_steal_pointer to make
it clearer the object lifetime exceeds the function bounds if we pass
all the checks.

Fixes: 7c092f17cce (virtio-gpu: Handle resource blob commands)
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: qemu-stable@nongnu.org
---
 hw/display/virtio-gpu-virgl.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index b7a2d160ddd..add85bd4e61 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -830,8 +830,7 @@ static void virgl_cmd_resource_create_blob(VirtIOGPU *g,
         return;
     }
 
-    res = virtio_gpu_virgl_find_resource(g, cblob.resource_id);
-    if (res) {
+    if (virtio_gpu_virgl_find_resource(g, cblob.resource_id)) {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n",
                       __func__, cblob.resource_id);
         cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
@@ -884,8 +883,9 @@ static void virgl_cmd_resource_create_blob(VirtIOGPU *g,
 
     res->base.dmabuf_fd = info.fd;
 
+    /* Now live, cleaned up in virtio_gpu_virgl_resource_unref */
     QTAILQ_INSERT_HEAD(&g->reslist, &res->base, next);
-    res = NULL;
+    g_steal_pointer(&res);
 }
 
 static void virgl_cmd_resource_map_blob(VirtIOGPU *g,
-- 
2.47.3

Re: [RFC PATCH] hw/display: don't accidentally autofree existing virgl resources

Posted by Dmitry Osipenko 1 month, 1 week ago

On 4/17/26 12:44, Alex Bennée wrote:
> While sanity checking a create blob operation the use of the auto
> freed res variable could lead to inadvertently freeing an existing
> blob.
> 
> Avoid this by in-lining the virtio_gpu_virgl_find_resource() check as
> the value is not needed anyway.
> 
> While at it add a comment to the end and use g_steal_pointer to make
> it clearer the object lifetime exceeds the function bounds if we pass
> all the checks.
> 
> Fixes: 7c092f17cce (virtio-gpu: Handle resource blob commands)
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: qemu-stable@nongnu.org
> ---
>  hw/display/virtio-gpu-virgl.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index b7a2d160ddd..add85bd4e61 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -830,8 +830,7 @@ static void virgl_cmd_resource_create_blob(VirtIOGPU *g,
>          return;
>      }
>  
> -    res = virtio_gpu_virgl_find_resource(g, cblob.resource_id);
> -    if (res) {
> +    if (virtio_gpu_virgl_find_resource(g, cblob.resource_id)) {
>          qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n",
>                        __func__, cblob.resource_id);
>          cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> @@ -884,8 +883,9 @@ static void virgl_cmd_resource_create_blob(VirtIOGPU *g,
>  
>      res->base.dmabuf_fd = info.fd;
>  
> +    /* Now live, cleaned up in virtio_gpu_virgl_resource_unref */
>      QTAILQ_INSERT_HEAD(&g->reslist, &res->base, next);
> -    res = NULL;
> +    g_steal_pointer(&res);
>  }
>  
>  static void virgl_cmd_resource_map_blob(VirtIOGPU *g,

Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

-- 
Best regards,
Dmitry

Re: [RFC PATCH] hw/display: don't accidentally autofree existing virgl resources

Posted by Manos Pitsidianakis 1 month, 1 week ago

On Fri, Apr 17, 2026 at 12:44 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
> While sanity checking a create blob operation the use of the auto
> freed res variable could lead to inadvertently freeing an existing
> blob.
>
> Avoid this by in-lining the virtio_gpu_virgl_find_resource() check as
> the value is not needed anyway.
>
> While at it add a comment to the end and use g_steal_pointer to make
> it clearer the object lifetime exceeds the function bounds if we pass
> all the checks.
>
> Fixes: 7c092f17cce (virtio-gpu: Handle resource blob commands)
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: qemu-stable@nongnu.org
> ---
>  hw/display/virtio-gpu-virgl.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index b7a2d160ddd..add85bd4e61 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -830,8 +830,7 @@ static void virgl_cmd_resource_create_blob(VirtIOGPU *g,
>          return;
>      }
>
> -    res = virtio_gpu_virgl_find_resource(g, cblob.resource_id);
> -    if (res) {
> +    if (virtio_gpu_virgl_find_resource(g, cblob.resource_id)) {
>          qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n",
>                        __func__, cblob.resource_id);
>          cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> @@ -884,8 +883,9 @@ static void virgl_cmd_resource_create_blob(VirtIOGPU *g,
>
>      res->base.dmabuf_fd = info.fd;
>
> +    /* Now live, cleaned up in virtio_gpu_virgl_resource_unref */
>      QTAILQ_INSERT_HEAD(&g->reslist, &res->base, next);
> -    res = NULL;
> +    g_steal_pointer(&res);
>  }
>
>  static void virgl_cmd_resource_map_blob(VirtIOGPU *g,
> --
> 2.47.3
>

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>