| Message ID | c61c13f9a0c67dec473bdbfc8789c29ef26c900b.1696624734.git.quic_mathbern@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/display: fix memleak from virtio_add_resource | expand |
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>
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; }
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(-)