Re: [PATCH] tests/9pfs: Use g_autofree and g_autoptr where possible

[Greg Kurz <groug@kaod.org>]

On Sat, 29 Jan 2022 13:33:59 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Freitag, 28. Januar 2022 12:49:58 CET Christian Schoenebeck wrote:
> > On Mittwoch, 26. Januar 2022 18:11:36 CET Greg Kurz wrote:
> > > The template pointer in virtio_9p_create_local_test_dir() is leaked.
> > > Add the g_autofree annotation to fix that. While here, convert the
> > > rest of the virtio 9p test code to using g_autofree or g_autoptr
> > > where possible, since this is the preferred approach to avoid potential
> > > leaks in the future.
> > > 
> > > Based-on:
> > > <f6602123c6f7d0d593466231b04fba087817abbd.1642879848.git.qemu_oss@crudebyt
> > > e
> > > .com> Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > > 
> > >  tests/qtest/libqos/virtio-9p.c | 15 +++++----------
> > >  1 file changed, 5 insertions(+), 10 deletions(-)
> > 
> > I fear there is something wrong with this patch:
> > 
> > # Start of local tests
> > # starting QEMU: exec x86_64-softmmu/qemu-system-x86_64 -qtest
> > unix:/tmp/qtest-4234.sock -qtest-log /dev/null -chardev
> > socket,path=/tmp/qtest-4234.qmp,id=char0 -mon chardev=char0,mode=control
> > -display none -M pc  -fsdev
> > local,id=fsdev0,path='',security_model=mapped-xattr -device
> > virtio-9p-pci,fsdev=fsdev0,addr=04.0,mount_tag=qtest -accel qtest
> > qemu-system-x86_64: -device
> > virtio-9p-pci,fsdev=fsdev0,addr=04.0,mount_tag=qtest: cannot initialize
> > fsdev 'fsdev0': failed to open '': No such file or directory Broken pipe
> > Aborted
> 
> Reason ...
> 
> > > diff --git a/tests/qtest/libqos/virtio-9p.c
> > > b/tests/qtest/libqos/virtio-9p.c index ef96ef006adc..0a0d0d16709b 100644
> > > --- a/tests/qtest/libqos/virtio-9p.c
> > > +++ b/tests/qtest/libqos/virtio-9p.c
> > > @@ -40,14 +40,13 @@ static char *concat_path(const char* a, const char* b)
> > > 
> > >  void virtio_9p_create_local_test_dir(void)
> > >  {
> > >  
> > >      struct stat st;
> > > 
> > > -    char *pwd = g_get_current_dir();
> > > -    char *template = concat_path(pwd, "qtest-9p-local-XXXXXX");
> > > +    g_autofree char *pwd = g_get_current_dir();
> > > +    g_autofree char *template = concat_path(pwd,
> > > "qtest-9p-local-XXXXXX");
> > > 
> > >      local_test_path = mkdtemp(template);
> 
> ... mkdtemp() does not allocate a new buffer, it just modifies the character 
> array passed, i.e. the address returned by mkdtemp() equals the address of 
> variable 'template', and when virtio_9p_create_local_test_dir() scope is left, 
> the global variable 'local_test_path' would then point to freed memory.
> 

I hate global variables ;-) and the 'Returned result must be freed' comment
in 'concat_path()' is slightly misleading in this respect.

> I would drop g_autofree from template:
> 
>     char *template = concat_path(pwd, "qtest-9p-local-XXXXXX");
> 
> And if it helps to silence a leak warning (haven't tested), to prepend 
> g_autofree to the global variable instead:
> 
> static g_autofree char *local_test_path;
> 

The way to go is either drop the g_autofree annotation as you're
suggesting, but this would make the comment in 'concat_path()'
even more awkward, or go forward with the glib way and use
g_steal_pointer() which maps exactly to what the code is doing.

> Best regards,
> Christian Schoenebeck
> 
>