[libvirt] [PATCH] qemu: fix memory leak in qemuAgentGetFSInfo
in virDomainFSInfoFree(), don't free the virDomainFSInfo data. ==10670== 80 bytes in 2 blocks are definitely lost in loss record 576 of 793 ==10670== at 0x4A06BC3: calloc (vg_replace_malloc.c:618) ==10670== by 0x509DEBD: virAlloc (viralloc.c:144) ==10670== by 0x19FBD558: qemuAgentGetFSInfo (qemu_agent.c:1837) ==10670== by 0x1A03CF91: qemuDomainGetFSInfo (qemu_driver.c:19238) Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- src/libvirt-domain.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 04545fd..7f8a7ce 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -11337,4 +11337,6 @@ virDomainFSInfoFree(virDomainFSInfoPtr info) for (i = 0; i < info->ndevAlias; i++) VIR_FREE(info->devAlias[i]); VIR_FREE(info->devAlias); + + VIR_FREE(info); } -- 1.9.3
On Tue, Mar 10, 2015 at 01:56:11PM +0800, Chen Fan wrote:
in virDomainFSInfoFree(), don't free the virDomainFSInfo data.
==10670== 80 bytes in 2 blocks are definitely lost in loss record 576 of 793 ==10670== at 0x4A06BC3: calloc (vg_replace_malloc.c:618) ==10670== by 0x509DEBD: virAlloc (viralloc.c:144) ==10670== by 0x19FBD558: qemuAgentGetFSInfo (qemu_agent.c:1837) ==10670== by 0x1A03CF91: qemuDomainGetFSInfo (qemu_driver.c:19238)
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- src/libvirt-domain.c | 2 ++ 1 file changed, 2 insertions(+)
This does fix the memory leak and makes the function behave like it's documented in virDomainGetFSInfo and virDomainFSInfoFree: http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainGetFSInfo http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainFSInfoFree But it changes the public API - if there are applications that already work around this function by freeing the info, this change would introduce a double free. I would NACK this if the documentation for both APIs didn't say that's how this function should behave. I'd like to hear a second opinion. Jan
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 04545fd..7f8a7ce 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -11337,4 +11337,6 @@ virDomainFSInfoFree(virDomainFSInfoPtr info) for (i = 0; i < info->ndevAlias; i++) VIR_FREE(info->devAlias[i]); VIR_FREE(info->devAlias); + + VIR_FREE(info); } -- 1.9.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 03/10/2015 05:32 PM, Ján Tomko wrote:
On Tue, Mar 10, 2015 at 01:56:11PM +0800, Chen Fan wrote:
in virDomainFSInfoFree(), don't free the virDomainFSInfo data.
==10670== 80 bytes in 2 blocks are definitely lost in loss record 576 of 793 ==10670== at 0x4A06BC3: calloc (vg_replace_malloc.c:618) ==10670== by 0x509DEBD: virAlloc (viralloc.c:144) ==10670== by 0x19FBD558: qemuAgentGetFSInfo (qemu_agent.c:1837) ==10670== by 0x1A03CF91: qemuDomainGetFSInfo (qemu_driver.c:19238)
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- src/libvirt-domain.c | 2 ++ 1 file changed, 2 insertions(+) This does fix the memory leak and makes the function behave like it's documented in virDomainGetFSInfo and virDomainFSInfoFree: http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainGetFSInfo http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainFSInfoFree
But it changes the public API - if there are applications that already work around this function by freeing the info, this change would introduce a double free.
I would NACK this if the documentation for both APIs didn't say that's how this function should behave.
I'd like to hear a second opinion. I don't think this documentation make any confusable. for using the function virDomainGetFSInfo(), user also need to call virDomainFSInfoFree() on each array element, and call free() info. example:
virDomainFSInfoPtr *info; ndata = virDomainGetFSInfo(dom, &info, 0); for (i = 0; i < ndata; i++) virDomainFSInfoFree(info[i]); VIR_FREE(info); Thanks, Chen
Jan
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 04545fd..7f8a7ce 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -11337,4 +11337,6 @@ virDomainFSInfoFree(virDomainFSInfoPtr info) for (i = 0; i < info->ndevAlias; i++) VIR_FREE(info->devAlias[i]); VIR_FREE(info->devAlias); + + VIR_FREE(info); } -- 1.9.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Mar 10, 2015 at 10:32:04AM +0100, Ján Tomko wrote:
On Tue, Mar 10, 2015 at 01:56:11PM +0800, Chen Fan wrote:
in virDomainFSInfoFree(), don't free the virDomainFSInfo data.
==10670== 80 bytes in 2 blocks are definitely lost in loss record 576 of 793 ==10670== at 0x4A06BC3: calloc (vg_replace_malloc.c:618) ==10670== by 0x509DEBD: virAlloc (viralloc.c:144) ==10670== by 0x19FBD558: qemuAgentGetFSInfo (qemu_agent.c:1837) ==10670== by 0x1A03CF91: qemuDomainGetFSInfo (qemu_driver.c:19238)
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- src/libvirt-domain.c | 2 ++ 1 file changed, 2 insertions(+)
This does fix the memory leak and makes the function behave like it's documented in virDomainGetFSInfo and virDomainFSInfoFree: http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainGetFSInfo http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainFSInfoFree
But it changes the public API - if there are applications that already work around this function by freeing the info, this change would introduce a double free.
Well, this is a good point that we might break somebody's application, but the virDomainFSInfoFree is the only one *Free API that doesn't call free/unref on passed argument. In the documentation the @info is an array and we specifically says that calling virDomainFSInfoFree on each element and then just free the @info is sufficient, but it isn't and there will be a memory leak. It changes behavior of public API to correctly free the allocated memory as documentation says. Let's hear another opinion, but I'm for fixing it. Pavel
I would NACK this if the documentation for both APIs didn't say that's how this function should behave.
I'd like to hear a second opinion.
Jan
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 04545fd..7f8a7ce 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -11337,4 +11337,6 @@ virDomainFSInfoFree(virDomainFSInfoPtr info) for (i = 0; i < info->ndevAlias; i++) VIR_FREE(info->devAlias[i]); VIR_FREE(info->devAlias); + + VIR_FREE(info); } -- 1.9.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Mar 10, 2015 at 11:38:52AM +0100, Pavel Hrdina wrote:
On Tue, Mar 10, 2015 at 10:32:04AM +0100, Ján Tomko wrote:
On Tue, Mar 10, 2015 at 01:56:11PM +0800, Chen Fan wrote:
in virDomainFSInfoFree(), don't free the virDomainFSInfo data.
==10670== 80 bytes in 2 blocks are definitely lost in loss record 576 of 793 ==10670== at 0x4A06BC3: calloc (vg_replace_malloc.c:618) ==10670== by 0x509DEBD: virAlloc (viralloc.c:144) ==10670== by 0x19FBD558: qemuAgentGetFSInfo (qemu_agent.c:1837) ==10670== by 0x1A03CF91: qemuDomainGetFSInfo (qemu_driver.c:19238)
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- src/libvirt-domain.c | 2 ++ 1 file changed, 2 insertions(+)
This does fix the memory leak and makes the function behave like it's documented in virDomainGetFSInfo and virDomainFSInfoFree: http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainGetFSInfo http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainFSInfoFree
But it changes the public API - if there are applications that already work around this function by freeing the info, this change would introduce a double free.
Well, this is a good point that we might break somebody's application, but the virDomainFSInfoFree is the only one *Free API that doesn't call free/unref on passed argument. In the documentation the @info is an array and we specifically says that calling virDomainFSInfoFree on each element and then just free the @info is sufficient, but it isn't and there will be a memory leak. It changes behavior of public API to correctly free the allocated memory as documentation says. Let's hear another opinion, but I'm for fixing it.
I agree, in the normal way of usage of this API, this will always be wrong. virsh is leaking this, python bindings are leaking this, perl bindings are leaking this. Only applications written in C have the opportunity to do a workaround, and most of our apps are not written in C. I think we have to fix this really. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Mar 10, 2015 at 10:46:45AM +0000, Daniel P. Berrange wrote:
On Tue, Mar 10, 2015 at 11:38:52AM +0100, Pavel Hrdina wrote:
On Tue, Mar 10, 2015 at 10:32:04AM +0100, Ján Tomko wrote:
On Tue, Mar 10, 2015 at 01:56:11PM +0800, Chen Fan wrote:
in virDomainFSInfoFree(), don't free the virDomainFSInfo data.
==10670== 80 bytes in 2 blocks are definitely lost in loss record 576 of 793 ==10670== at 0x4A06BC3: calloc (vg_replace_malloc.c:618) ==10670== by 0x509DEBD: virAlloc (viralloc.c:144) ==10670== by 0x19FBD558: qemuAgentGetFSInfo (qemu_agent.c:1837) ==10670== by 0x1A03CF91: qemuDomainGetFSInfo (qemu_driver.c:19238)
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- src/libvirt-domain.c | 2 ++ 1 file changed, 2 insertions(+)
This does fix the memory leak and makes the function behave like it's documented in virDomainGetFSInfo and virDomainFSInfoFree: http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainGetFSInfo http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainFSInfoFree
But it changes the public API - if there are applications that already work around this function by freeing the info, this change would introduce a double free.
Well, this is a good point that we might break somebody's application, but the virDomainFSInfoFree is the only one *Free API that doesn't call free/unref on passed argument. In the documentation the @info is an array and we specifically says that calling virDomainFSInfoFree on each element and then just free the @info is sufficient, but it isn't and there will be a memory leak. It changes behavior of public API to correctly free the allocated memory as documentation says. Let's hear another opinion, but I'm for fixing it.
I agree, in the normal way of usage of this API, this will always be wrong. virsh is leaking this, python bindings are leaking this, perl bindings are leaking this. Only applications written in C have the opportunity to do a workaround, and most of our apps are not written in C. I think we have to fix this really.
Now pushed. Jan
participants (4)
-
Chen Fan -
Daniel P. Berrange -
Ján Tomko -
Pavel Hrdina