[libvirt] [PATCH] daemon: Plug memory leak on remoteDispatchAuthList
Detected by valgrind. Leak is introuduced in commit fcdfa31. * daemon/remote.c (remoteDispatchAuthList): fix memory leak on failure path. * Actual result ==26844== 150,592 bytes in 9,602 blocks are definitely lost in loss record 735 of 738 ==26844== at 0x4A05FDE: malloc (vg_replace_malloc.c:236) ==26844== by 0x36AEAFEDE7: __vasprintf_chk (in /lib64/libc-2.12.so) ==26844== by 0x36C3A59B03: virVasprintf (stdio2.h:199) ==26844== by 0x36C3A59BA7: virAsprintf (util.c:1827) ==26844== by 0x4335F5: remoteDispatchAuthListHelper (remote.c:2054) ==26844== by 0x36C3B10394: virNetServerProgramDispatch (virnetserverprogram.c:416) ==26844== by 0x36C3B11620: virNetServerHandleJob (virnetserver.c:164) ==26844== by 0x36C3A57AAB: virThreadPoolWorker (threadpool.c:144) ==26844== by 0x36C3A573C8: virThreadHelper (threads-pthread.c:161) ==26844== by 0x36AEE077F0: start_thread (in /lib64/libpthread-2.12.so) ==26844== by 0x36AEAE570C: clone (in /lib64/libc-2.12.so) Notes, the test is in a loop, so you will see a quite big leaks. Signed-off-by: Alex Jia <ajia@redhat.com> --- daemon/remote.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 74a5f16..9018c15 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -2054,6 +2054,7 @@ remoteDispatchAuthList(virNetServerPtr server ATTRIBUTE_UNUSED, if (virAsprintf(&ident, "pid:%lld,uid:%d", (long long) callerPid, callerUid) < 0) { virReportOOMError(); + VIR_FREE(ident); goto cleanup; } VIR_INFO("Bypass polkit auth for privileged client %s", ident); -- 1.7.1
On 02/20/2012 03:05 AM, Alex Jia wrote:
Detected by valgrind. Leak is introuduced in commit fcdfa31.
* daemon/remote.c (remoteDispatchAuthList): fix memory leak on failure path.
* Actual result
+++ b/daemon/remote.c @@ -2054,6 +2054,7 @@ remoteDispatchAuthList(virNetServerPtr server ATTRIBUTE_UNUSED, if (virAsprintf(&ident, "pid:%lld,uid:%d", (long long) callerPid, callerUid) < 0) { virReportOOMError(); + VIR_FREE(ident);
How can that possibly be fixing the leak? If virAsprintf returns < 0, then ident is guaranteed to be NULL, and you have added a no-op statement. I wouldn't be surprised if there is still a lurking bug here; after all, my commit 15a280bb was an attempt to solve a valgrind memory leak, which was then in turn reworked by Jim in commits c05ec920 and fcdfa31f. But we need to diagnose and patch the real problem, if this is the case. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Hello Eric, As you said, it's impossible to fix this leak by my change, and from codes point of view, the failure path hasn't any memory leak. I suspect valgrind gave a incorrect report, and I will check it later. Thanks for your comment, Alex ----- Original Message ----- From: "Eric Blake" <eblake@redhat.com> To: "Alex Jia" <ajia@redhat.com> Cc: libvir-list@redhat.com Sent: Monday, February 20, 2012 10:24:43 PM Subject: Re: [libvirt] [PATCH] daemon: Plug memory leak on remoteDispatchAuthList On 02/20/2012 03:05 AM, Alex Jia wrote:
Detected by valgrind. Leak is introuduced in commit fcdfa31.
* daemon/remote.c (remoteDispatchAuthList): fix memory leak on failure path.
* Actual result
+++ b/daemon/remote.c @@ -2054,6 +2054,7 @@ remoteDispatchAuthList(virNetServerPtr server ATTRIBUTE_UNUSED, if (virAsprintf(&ident, "pid:%lld,uid:%d", (long long) callerPid, callerUid) < 0) { virReportOOMError(); + VIR_FREE(ident);
How can that possibly be fixing the leak? If virAsprintf returns < 0, then ident is guaranteed to be NULL, and you have added a no-op statement. I wouldn't be surprised if there is still a lurking bug here; after all, my commit 15a280bb was an attempt to solve a valgrind memory leak, which was then in turn reworked by Jim in commits c05ec920 and fcdfa31f. But we need to diagnose and patch the real problem, if this is the case. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
[side note - top-posting on technical lists makes it a bit harder to follow the conversation - I've reformatted my reply] On 02/20/2012 09:06 AM, Alex Jia wrote:
Hello Eric,
I wouldn't be surprised if there is still a lurking bug here; after all, my commit 15a280bb was an attempt to solve a valgrind memory leak, which was then in turn reworked by Jim in commits c05ec920 and fcdfa31f. But we need to diagnose and patch the real problem, if this is the case.
As you said, it's impossible to fix this leak by my change, and from codes point of view, the failure path hasn't any memory leak. I suspect valgrind gave a incorrect report, and I will check it later.
At this point, I've re-read fcdfa31f, and don't see any leak of ident. I suspect that what really happened is that you ran valgrind on 0.9.10, prior to my commit 15a280b, where there really was a memleak on ident; but then wrote your patch after upgrading to latest rather than re-checking valgrind to see if libvirt.git had fixed the bug in the meantime. Since my commit 15a280b didn't mention plugging a memory leak, nor that I had found the regression in question by using valgrind, I can see why I might have confused you. So not only did I cause problems by introducing the leak prior to 0.9.10, I compounded it in my patch to fix the leak. Sorry for the mess I caused :) -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/21/2012 02:08 AM, Eric Blake wrote:
[side note - top-posting on technical lists makes it a bit harder to follow the conversation - I've reformatted my reply]
Hello Eric,
I wouldn't be surprised if there is still a lurking bug here; after all, my commit 15a280bb was an attempt to solve a valgrind memory leak, which was then in turn reworked by Jim in commits c05ec920 and fcdfa31f. But we need to diagnose and patch the real problem, if this is the case. As you said, it's impossible to fix this leak by my change, and from codes point of view, the failure path hasn't any memory leak. I suspect valgrind gave a incorrect report, and I will check it later. At this point, I've re-read fcdfa31f, and don't see any leak of ident. I suspect that what really happened is that you ran valgrind on 0.9.10,
On 02/20/2012 09:06 AM, Alex Jia wrote: prior to my commit 15a280b, where there really was a memleak on ident; but then wrote your patch after upgrading to latest rather than re-checking valgrind to see if libvirt.git had fixed the bug in the meantime. Since my commit 15a280b didn't mention plugging a memory leak, nor that I had found the regression in question by using valgrind, I can see why I might have confused you. So not only did I cause problems by introducing the leak prior to 0.9.10, I compounded it in my patch to fix the leak. Sorry for the mess I caused :) Thanks, it's okay for me, I should see patches more careful next time :)
Regards, Alex
participants (2)
-
Alex Jia -
Eric Blake