Hi
On Mon, May 31, 2021 at 2:20 PM Philippe Mathieu-Daudé <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>
> ---
> 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;
> - }
> -
> 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) {
> cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
> + g_free(res);
> return;
> }
>
> --
> 2.26.3
>
>
>
--
Marc-André Lureau
