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.lur...@gmail.com> Sent: Wednesday, July 07, 2021 4:05 AM To: Philippe Mathieu-Daudé <phi...@redhat.com>; Kasireddy, Vivek <vivek.kasire...@intel.com> Cc: QEMU <qemu-devel@nongnu.org>; Gerd Hoffmann <kra...@redhat.com>; Michael S. Tsirkin <m...@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é <phi...@redhat.com<mailto:phi...@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é <phi...@redhat.com<mailto:phi...@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.kasire...@intel.com<mailto:vivek.kasire...@intel.com>> Thanks, Vivek cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC; + g_free(res); return; } -- 2.26.3 -- Marc-André Lureau