On Sat, 29 Jan 2022 13:33:59 +0100
Christian Schoenebeck <qemu_...@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 <gr...@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
>
>
