When the given uuid is already present in the hash table,
virtio_add_resource() does not add the passed VirtioSharedObject. In
this case, free it in the callers to avoid leaking memory. This fixed
the following `make check` error, when built with --enable-sanitizers:

  4/166 qemu:unit / test-virtio-dmabuf   ERROR 1.51s   exit status 1

  ==7716==ERROR: LeakSanitizer: detected memory leaks
  Direct leak of 320 byte(s) in 20 object(s) allocated from:
      #0 0x7f6fc16e3808 in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:144
      #1 0x7f6fc1503e98 in g_malloc (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x57e98)
      #2 0x564d63cafb6b in test_add_invalid_resource ../tests/unit/test-virtio-dmabuf.c:100
      #3 0x7f6fc152659d  (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7a59d)
  SUMMARY: AddressSanitizer: 320 byte(s) leaked in 20 allocation(s).

The changes at virtio_add_resource() itself are not strictly necessary
for the memleak fix, but they make it more obvious that, on an error
return, the passed object is not added to the hash.

Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
---
 hw/display/virtio-dmabuf.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
index 4a8e430f3d..3dba4577ca 100644
--- a/hw/display/virtio-dmabuf.c
+++ b/hw/display/virtio-dmabuf.c
@@ -29,7 +29,7 @@ static int uuid_equal_func(const void *lhv, const void *rhv)
 
 static bool virtio_add_resource(QemuUUID *uuid, VirtioSharedObject *value)
 {
-    bool result = false;
+    bool result = true;
 
     g_mutex_lock(&lock);
     if (resource_uuids == NULL) {
@@ -39,7 +39,9 @@ static bool virtio_add_resource(QemuUUID *uuid, VirtioSharedObject *value)
                                                g_free);
     }
     if (g_hash_table_lookup(resource_uuids, uuid) == NULL) {
-        result = g_hash_table_insert(resource_uuids, uuid, value);
+        g_hash_table_insert(resource_uuids, uuid, value);
+    } else {
+        result = false;
     }
     g_mutex_unlock(&lock);
 
@@ -57,6 +59,9 @@ bool virtio_add_dmabuf(QemuUUID *uuid, int udmabuf_fd)
     vso->type = TYPE_DMABUF;
     vso->value = GINT_TO_POINTER(udmabuf_fd);
     result = virtio_add_resource(uuid, vso);
+    if (!result) {
+        g_free(vso);
+    }
 
     return result;
 }
@@ -72,6 +77,9 @@ bool virtio_add_vhost_device(QemuUUID *uuid, struct vhost_dev *dev)
     vso->type = TYPE_VHOST_DEV;
     vso->value = dev;
     result = virtio_add_resource(uuid, vso);
+    if (!result) {
+        g_free(vso);
+    }
 
     return result;
 }
-- 
2.37.2

On Fri, Oct 6, 2023 at 10:41 PM Matheus Tavares Bernardino <
quic_mathbern@quicinc.com> wrote:

> When the given uuid is already present in the hash table,
> virtio_add_resource() does not add the passed VirtioSharedObject. In
> this case, free it in the callers to avoid leaking memory. This fixed
> the following `make check` error, when built with --enable-sanitizers:
>
>   4/166 qemu:unit / test-virtio-dmabuf   ERROR 1.51s   exit status 1
>
>   ==7716==ERROR: LeakSanitizer: detected memory leaks
>   Direct leak of 320 byte(s) in 20 object(s) allocated from:
>       #0 0x7f6fc16e3808 in __interceptor_malloc
> ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:144
>       #1 0x7f6fc1503e98 in g_malloc
> (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x57e98)
>       #2 0x564d63cafb6b in test_add_invalid_resource
> ../tests/unit/test-virtio-dmabuf.c:100
>       #3 0x7f6fc152659d  (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7a59d)
>   SUMMARY: AddressSanitizer: 320 byte(s) leaked in 20 allocation(s).
>
> The changes at virtio_add_resource() itself are not strictly necessary
> for the memleak fix, but they make it more obvious that, on an error
> return, the passed object is not added to the hash.
>
> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> ---
>  hw/display/virtio-dmabuf.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
> index 4a8e430f3d..3dba4577ca 100644
> --- a/hw/display/virtio-dmabuf.c
> +++ b/hw/display/virtio-dmabuf.c
> @@ -29,7 +29,7 @@ static int uuid_equal_func(const void *lhv, const void
> *rhv)
>
>  static bool virtio_add_resource(QemuUUID *uuid, VirtioSharedObject *value)
>  {
> -    bool result = false;
> +    bool result = true;
>
>      g_mutex_lock(&lock);
>      if (resource_uuids == NULL) {
> @@ -39,7 +39,9 @@ static bool virtio_add_resource(QemuUUID *uuid,
> VirtioSharedObject *value)
>                                                 g_free);
>      }
>      if (g_hash_table_lookup(resource_uuids, uuid) == NULL) {
> -        result = g_hash_table_insert(resource_uuids, uuid, value);
> +        g_hash_table_insert(resource_uuids, uuid, value);
> +    } else {
> +        result = false;
>      }
>      g_mutex_unlock(&lock);
>
> @@ -57,6 +59,9 @@ bool virtio_add_dmabuf(QemuUUID *uuid, int udmabuf_fd)
>      vso->type = TYPE_DMABUF;
>      vso->value = GINT_TO_POINTER(udmabuf_fd);
>      result = virtio_add_resource(uuid, vso);
> +    if (!result) {
> +        g_free(vso);
> +    }
>
>      return result;
>  }
> @@ -72,6 +77,9 @@ bool virtio_add_vhost_device(QemuUUID *uuid, struct
> vhost_dev *dev)
>      vso->type = TYPE_VHOST_DEV;
>      vso->value = dev;
>      result = virtio_add_resource(uuid, vso);
> +    if (!result) {
> +        g_free(vso);
> +    }
>
>      return result;
>  }
> --
> 2.37.2
>
>
Thanks!

Reviewed-by: Albert Esteve <aesteve@redhat.com>