Hi, Since now we allocate memory for the key, we should provide the > `key_destroy_func` when calling g_hash_table_new_full(), otherwise > this new memory will not be de-allocated. >
Ack IIUC it can be null, only if `uuid` was null since glib memory > API usually terminates the application if memory allocation fails, > see https://docs.gtk.org/glib/memory.html Ok, but the documentation doesn't explicitly state what `g_memdup2` returns on failure, I checked it manually and it terminates the application on failure. So indeed this check is not necessary. I think it may be worth clarifying that it can also return false if > the resource failed > to be added. Hence, I think this is not needed since `g_memdup2` will terminate the application if memory allocation fails and no return value is provided then. Thanks, Dorinda. On Thu, Nov 7, 2024 at 2:47 PM Stefano Garzarella <sgarz...@redhat.com> wrote: > On Thu, Nov 7, 2024 at 2:43 PM Stefano Garzarella <sgarz...@redhat.com> > wrote: > > > > On Thu, Nov 07, 2024 at 01:52:01PM +0100, Dorinda Bassey wrote: > > >In `virtio_add_resource` function, the UUID used as a key for > > >`g_hash_table_insert` was temporary, which could lead to > > >invalid lookups when accessed later. This patch ensures that > > >the UUID remains valid by duplicating it into a newly allocated > > >memory space (persistent_uuid). The value is then inserted into > > >the hash table with this persistent UUID key to ensure that the > > >key stored in the hash table remains valid as long as the hash > > >table entry exists. > > > > It's a fix right, so maybe better to add a Fixes tag: > > > > Fixes: faefdba847 ("hw/display: introduce virtio-dmabuf") > > > > > > > >Signed-off-by: Dorinda Bassey <dbas...@redhat.com> > > >--- > > > hw/display/virtio-dmabuf.c | 7 ++++++- > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > >diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c > > >index 3dba4577ca7..4353970bc87 100644 > > >--- a/hw/display/virtio-dmabuf.c > > >+++ b/hw/display/virtio-dmabuf.c > > >@@ -39,7 +39,12 @@ static bool virtio_add_resource(QemuUUID *uuid, > VirtioSharedObject *value) > > > g_free); > > > } > > > if (g_hash_table_lookup(resource_uuids, uuid) == NULL) { > > >- g_hash_table_insert(resource_uuids, uuid, value); > > >+ QemuUUID *persistent_uuid = g_memdup2(uuid, sizeof(QemuUUID)); > > > > Since now we allocate memory for the key, we should provide the > > `key_destroy_func` when calling g_hash_table_new_full(), otherwise > > this new memory will not be de-allocated. > > > > >+ if (persistent_uuid == NULL) { > > > > IIUC it can be null, only if `uuid` was null since glib memory > > API usually terminates the application if memory allocation fails, > > see https://docs.gtk.org/glib/memory.html > > > > So maybe we can just do: > > g_hash_table_insert(resource_uuids, g_memdup2(uuid, > sizeof(QemuUUID)), > > value); > > Or even better: > g_hash_table_insert(resource_uuids, g_memdup2(uuid, > sizeof(*uuid)), > value); > > Thanks, > Stefano > > > > > >+ result = false; > > >+ } else { > > >+ g_hash_table_insert(resource_uuids, persistent_uuid, > value); > > >+ } > > > } else { > > > result = false; > > > } > > >-- > > >2.47.0 > > > > >
