4:05 a.m.
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(a)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
8:24 a.m.
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
10:06 a.m.
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(a)redhat.com>
To: "Alex Jia" <ajia(a)redhat.com>
Cc: libvir-list(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
12:08 p.m.
[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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:54 p.m.
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]
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 :)
Thanks, it's okay for
me, I should see patches more careful next time :)
Regards,
Alex
4659
days inactive
4660
days old
4 comments
2 participants
participants (2)
-
Alex Jia -
Eric Blake