Re: [PATCH] virtio-dmabuf: Ensure UUID persistence for hash table insertion

[Dorinda Bassey <dbassey@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3570 bytes --]

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 <sgarzare@redhat.com>
wrote:

> On Thu, Nov 7, 2024 at 2:43 PM Stefano Garzarella <sgarzare@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 <dbassey@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
> > >
>
>

[-- Attachment #2: Type: text/html, Size: 5255 bytes --]