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

[Christian Schoenebeck]

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 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;

Best regards,
Christian Schoenebeck