| Message ID | 1576025722-41720-1-git-send-email-pannengyuan@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | vhost-user-test: fix a memory leak | expand |
On 11/12/2019 01:55, pannengyuan@huawei.com wrote: > From: Pan Nengyuan <pannengyuan@huawei.com> > > Spotted by ASAN. > > Reported-by: Euler Robot <euler.robot@huawei.com> > Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> > --- > tests/vhost-user-test.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c > index 91ea373..54be931 100644 > --- a/tests/vhost-user-test.c > +++ b/tests/vhost-user-test.c > @@ -717,6 +717,8 @@ static void test_migrate(void *obj, void *arg, QGuestAllocator *alloc) > guint64 size; > > if (!wait_for_fds(s)) { > + g_free(uri); > + test_server_free(dest); > return; > } > Don't we need also a g_string_free(dest_cmdline)? I think it is also missing at the end of the function. Thanks, Laurent
Hi! On 11/12/2019 01.55, pannengyuan@huawei.com wrote: [...] > diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c > index 91ea373..54be931 100644 > --- a/tests/vhost-user-test.c > +++ b/tests/vhost-user-test.c > @@ -717,6 +717,8 @@ static void test_migrate(void *obj, void *arg, QGuestAllocator *alloc) > guint64 size; > > if (!wait_for_fds(s)) { > + g_free(uri); > + test_server_free(dest); > return; > } Well spotted. But I'd prefer to rather move the allocation of these resources after the if-statement instead of doing the allocation at the declaration of the variables already. Or maybe use a "goto out" and jump to the end of the function instead? ... whatever you prefer, but duplicating the "free" functions sounds like a cumbersome solution to me. Thanks, Thomas
Hi On Wed, Dec 11, 2019 at 11:57 AM Thomas Huth <thuth@redhat.com> wrote: > > Hi! > > On 11/12/2019 01.55, pannengyuan@huawei.com wrote: > [...] > > diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c > > index 91ea373..54be931 100644 > > --- a/tests/vhost-user-test.c > > +++ b/tests/vhost-user-test.c > > @@ -717,6 +717,8 @@ static void test_migrate(void *obj, void *arg, QGuestAllocator *alloc) > > guint64 size; > > > > if (!wait_for_fds(s)) { > > + g_free(uri); > > + test_server_free(dest); > > return; > > } > > Well spotted. But I'd prefer to rather move the allocation of these > resources after the if-statement instead of doing the allocation at the > declaration of the variables already. Or maybe use a "goto out" and jump > to the end of the function instead? ... whatever you prefer, but > duplicating the "free" functions sounds like a cumbersome solution to me. g_auto (preferably) should work as well.
On 2019/12/11 15:48, Laurent Vivier wrote: > On 11/12/2019 01:55, pannengyuan@huawei.com wrote: >> From: Pan Nengyuan <pannengyuan@huawei.com> >> >> Spotted by ASAN. >> >> Reported-by: Euler Robot <euler.robot@huawei.com> >> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> >> --- >> tests/vhost-user-test.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c >> index 91ea373..54be931 100644 >> --- a/tests/vhost-user-test.c >> +++ b/tests/vhost-user-test.c >> @@ -717,6 +717,8 @@ static void test_migrate(void *obj, void *arg, QGuestAllocator *alloc) >> guint64 size; >> >> if (!wait_for_fds(s)) { >> + g_free(uri); >> + test_server_free(dest); >> return; >> } >> > > Don't we need also a g_string_free(dest_cmdline)? > > I think it is also missing at the end of the function. > Yes, you are right. But I'm surprised that it hasn't been detected by asan. I will continue to try it and send a new version. > Thanks, > Laurent > >
On 2019/12/11 15:57, Thomas Huth wrote: > Hi! > > On 11/12/2019 01.55, pannengyuan@huawei.com wrote: > [...] >> diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c >> index 91ea373..54be931 100644 >> --- a/tests/vhost-user-test.c >> +++ b/tests/vhost-user-test.c >> @@ -717,6 +717,8 @@ static void test_migrate(void *obj, void *arg, QGuestAllocator *alloc) >> guint64 size; >> >> if (!wait_for_fds(s)) { >> + g_free(uri); >> + test_server_free(dest); >> return; >> } > > Well spotted. But I'd prefer to rather move the allocation of these > resources after the if-statement instead of doing the allocation at the > declaration of the variables already. Or maybe use a "goto out" and jump > to the end of the function instead? ... whatever you prefer, but > duplicating the "free" functions sounds like a cumbersome solution to me. > Yes, I think use a "goto out" is more better. I will change it. Thanks. > Thanks, > Thomas > >
On 2019/12/11 16:18, Marc-André Lureau wrote: > Hi > > On Wed, Dec 11, 2019 at 11:57 AM Thomas Huth <thuth@redhat.com> wrote: >> >> Hi! >> >> On 11/12/2019 01.55, pannengyuan@huawei.com wrote: >> [...] >>> diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c >>> index 91ea373..54be931 100644 >>> --- a/tests/vhost-user-test.c >>> +++ b/tests/vhost-user-test.c >>> @@ -717,6 +717,8 @@ static void test_migrate(void *obj, void *arg, QGuestAllocator *alloc) >>> guint64 size; >>> >>> if (!wait_for_fds(s)) { >>> + g_free(uri); >>> + test_server_free(dest); >>> return; >>> } >> >> Well spotted. But I'd prefer to rather move the allocation of these >> resources after the if-statement instead of doing the allocation at the >> declaration of the variables already. Or maybe use a "goto out" and jump >> to the end of the function instead? ... whatever you prefer, but >> duplicating the "free" functions sounds like a cumbersome solution to me. > > g_auto (preferably) should work as well. Yes, it should work as well. But try to keep it as it is, I will use a "goto" instead. Thanks. > >
diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c index 91ea373..54be931 100644 --- a/tests/vhost-user-test.c +++ b/tests/vhost-user-test.c @@ -717,6 +717,8 @@ static void test_migrate(void *obj, void *arg, QGuestAllocator *alloc) guint64 size; if (!wait_for_fds(s)) { + g_free(uri); + test_server_free(dest); return; }