RE: [RFC PATCH] hw/display/virtio-gpu: Fix memory leak (CID 1453811)

["Kasireddy, Vivek" <vivek.kasireddy@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3844 bytes --]

Hi Philippe,
I am really sorry for the late review; I totally missed it until now.
Please find my comments inline.

From: Marc-André Lureau <marcandre.lureau@gmail.com>
Sent: Wednesday, July 07, 2021 4:05 AM
To: Philippe Mathieu-Daudé <philmd@redhat.com>; Kasireddy, Vivek <vivek.kasireddy@intel.com>
Cc: QEMU <qemu-devel@nongnu.org>; Gerd Hoffmann <kraxel@redhat.com>; Michael S. Tsirkin <mst@redhat.com>
Subject: Re: [RFC PATCH] hw/display/virtio-gpu: Fix memory leak (CID 1453811)

Hi

On Mon, May 31, 2021 at 2:20 PM Philippe Mathieu-Daudé <philmd@redhat.com<mailto:philmd@redhat.com>> wrote:
To avoid leaking memory on the error path, reorder the
code as:
- check the parameters first
- check resource already existing
- finally allocate memory

Reported-by: Coverity (CID 1453811: RESOURCE_LEAK)
Fixes: e0933d91b1c ("virtio-gpu: Add virtio_gpu_resource_create_blob")
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com<mailto:philmd@redhat.com>>
---
RFC because the s->iov check is dubious.

Yes, that looks wrong. Before the patch, the condition is always false / dead code.

Furthermore, the init_udmabuf seems to really make sense when iov != NULL and remapping takes place.

Vivek, please review

thanks

---
 hw/display/virtio-gpu.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 4d549377cbc..8d047007bbb 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -340,8 +340,15 @@ static void virtio_gpu_resource_create_blob(VirtIOGPU *g,
         return;
     }

-    res = virtio_gpu_find_resource(g, cblob.resource_id);
-    if (res) {
+    if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_GUEST &&
+        cblob.blob_flags != VIRTIO_GPU_BLOB_FLAG_USE_SHAREABLE) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid memory type\n",
+                      __func__);
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
+        return;
+    }
+
+    if (virtio_gpu_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;
@@ -352,25 +359,12 @@ static void virtio_gpu_resource_create_blob(VirtIOGPU *g,
     res->resource_id = cblob.resource_id;
     res->blob_size = cblob.size;

-    if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_GUEST &&
-        cblob.blob_flags != VIRTIO_GPU_BLOB_FLAG_USE_SHAREABLE) {
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid memory type\n",
-                      __func__);
-        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
-        g_free(res);
-        return;
-    }
-
-    if (res->iov) {
-        cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
-        return;
-    }
[Kasireddy, Vivek] Yeah, removing this makes sense. Basically,
resource_create_blob = resource_create + attach_backing and I was trying to do what
attach_backing was doing but this conditional does not make sense for create_blob as
we just created the resource few lines above.

-
     ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries, sizeof(cblob),
                                         cmd, &res->addrs, &res->iov,
                                         &res->iov_cnt);
-    if (ret != 0) {
+    if (ret != 0 || res->iov) {
[Kasireddy, Vivek] Would be redundant to check for res->iov as iov == NULL case will be covered by ret != 0.
With this change,

Reviewed-by: Vivek Kasireddy <vivek.kasireddy@intel.com<mailto:vivek.kasireddy@intel.com>>

Thanks,
Vivek

         cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
+        g_free(res);
         return;
     }

--
2.26.3



--
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 10809 bytes --]