[libvirt] [PATCH] fix memory leak in virCopyLastError
memset before virResetError will cause memory leak. virResetError and virCopyError, which calls virResetError, will do memset properly, so we don't have to worry about it here. --- src/util/virterror.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/util/virterror.c b/src/util/virterror.c index 7caa69e..46afd37 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -276,8 +276,6 @@ int virCopyLastError(virErrorPtr to) { virErrorPtr err = virLastErrorObject(); - /* We can't guarantee caller has initialized it to zero */ - memset(to, 0, sizeof(*to)); if (err) virCopyError(err, to); else -- 1.7.10.2
On Fri, Sep 14, 2012 at 02:24:15PM +0800, Hu Tao wrote:
memset before virResetError will cause memory leak.
virResetError and virCopyError, which calls virResetError, will do memset properly, so we don't have to worry about it here.
Disagree, it's a public API, we can't justify behaviour just on how it is used internally. NACK, at least the explanation need to be fixed What is the scenario for the leak ? Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Fri, Sep 14, 2012 at 03:10:13PM +0800, Daniel Veillard wrote:
On Fri, Sep 14, 2012 at 02:24:15PM +0800, Hu Tao wrote:
memset before virResetError will cause memory leak.
virResetError and virCopyError, which calls virResetError, will do memset properly, so we don't have to worry about it here.
Disagree, it's a public API, we can't justify behaviour just on how it is used internally.
NACK, at least the explanation need to be fixed
What is the scenario for the leak ?
The leaked memory was allocated at qemu_monitor.c:636. One of the leak reported by valgrind is: ==12636== 40 bytes in 1 blocks are definitely lost in loss record 302 of 620 ==12636== at 0x4A05E46: malloc (vg_replace_malloc.c:195) ==12636== by 0x306B27FC01: strdup (in /lib64/libc-2.13.so) ==12636== by 0x4EA5669: virCopyError (virterror.c:182) ==12636== by 0x4EA573C: virCopyLastError (virterror.c:282) ==12636== by 0x110CFEA9: qemuMonitorIO (qemu_monitor.c:636) ==12636== by 0x4E83950: virEventPollRunOnce (event_poll.c:485) ==12636== by 0x4E82004: virEventRunDefaultImpl (event.c:247) ==12636== by 0x4F822BC: virNetServerRun (virnetserver.c:751) ==12636== by 0x40C433: main (libvirtd.c:1338) The scenario is: If we deep-copy a virError, by virCopyLastError, into another virError object which is previously deep-copied into, then we have no chance to free previously allocated memory, because the memset in virCopyLastError loses any pointers to them. -- Thanks, Hu Tao
On Fri, Sep 14, 2012 at 03:34:29PM +0800, Hu Tao wrote:
On Fri, Sep 14, 2012 at 03:10:13PM +0800, Daniel Veillard wrote:
On Fri, Sep 14, 2012 at 02:24:15PM +0800, Hu Tao wrote:
memset before virResetError will cause memory leak.
virResetError and virCopyError, which calls virResetError, will do memset properly, so we don't have to worry about it here.
Disagree, it's a public API, we can't justify behaviour just on how it is used internally.
NACK, at least the explanation need to be fixed
What is the scenario for the leak ?
The leaked memory was allocated at qemu_monitor.c:636. One of the leak reported by valgrind is:
==12636== 40 bytes in 1 blocks are definitely lost in loss record 302 of 620 ==12636== at 0x4A05E46: malloc (vg_replace_malloc.c:195) ==12636== by 0x306B27FC01: strdup (in /lib64/libc-2.13.so) ==12636== by 0x4EA5669: virCopyError (virterror.c:182) ==12636== by 0x4EA573C: virCopyLastError (virterror.c:282) ==12636== by 0x110CFEA9: qemuMonitorIO (qemu_monitor.c:636) ==12636== by 0x4E83950: virEventPollRunOnce (event_poll.c:485) ==12636== by 0x4E82004: virEventRunDefaultImpl (event.c:247) ==12636== by 0x4F822BC: virNetServerRun (virnetserver.c:751) ==12636== by 0x40C433: main (libvirtd.c:1338)
The scenario is: If we deep-copy a virError, by virCopyLastError, into another virError object which is previously deep-copied into, then we have no chance to free previously allocated memory, because the memset in virCopyLastError loses any pointers to them.
So we have a problem here, we use virCopyLastError() t copy errors internally in location where an error might have been allocated, and that's also a public entry point where we can't trust the user provided data. We can't change the API behaviour so virCopyLastError() has to keep the memset, but we should do an private function virCopyLastErrorinternal() which would deallocate existing data in @to before calling virCopyError() Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Fri, Sep 14, 2012 at 03:58:31PM +0800, Daniel Veillard wrote:
On Fri, Sep 14, 2012 at 03:34:29PM +0800, Hu Tao wrote:
On Fri, Sep 14, 2012 at 03:10:13PM +0800, Daniel Veillard wrote:
On Fri, Sep 14, 2012 at 02:24:15PM +0800, Hu Tao wrote:
memset before virResetError will cause memory leak.
virResetError and virCopyError, which calls virResetError, will do memset properly, so we don't have to worry about it here.
Disagree, it's a public API, we can't justify behaviour just on how it is used internally.
NACK, at least the explanation need to be fixed
What is the scenario for the leak ?
The leaked memory was allocated at qemu_monitor.c:636. One of the leak reported by valgrind is:
==12636== 40 bytes in 1 blocks are definitely lost in loss record 302 of 620 ==12636== at 0x4A05E46: malloc (vg_replace_malloc.c:195) ==12636== by 0x306B27FC01: strdup (in /lib64/libc-2.13.so) ==12636== by 0x4EA5669: virCopyError (virterror.c:182) ==12636== by 0x4EA573C: virCopyLastError (virterror.c:282) ==12636== by 0x110CFEA9: qemuMonitorIO (qemu_monitor.c:636) ==12636== by 0x4E83950: virEventPollRunOnce (event_poll.c:485) ==12636== by 0x4E82004: virEventRunDefaultImpl (event.c:247) ==12636== by 0x4F822BC: virNetServerRun (virnetserver.c:751) ==12636== by 0x40C433: main (libvirtd.c:1338)
The scenario is: If we deep-copy a virError, by virCopyLastError, into another virError object which is previously deep-copied into, then we have no chance to free previously allocated memory, because the memset in virCopyLastError loses any pointers to them.
So we have a problem here, we use virCopyLastError() t copy errors internally in location where an error might have been allocated, and that's also a public entry point where we can't trust the user provided data. We can't change the API behaviour so virCopyLastError() has to keep the memset, but we should do an private function virCopyLastErrorinternal() which would deallocate existing data in @to before calling virCopyError()
No, I don't believe we need that. The code in question is already checking whether the error is allocated or not: if (mon->lastError.code != VIR_ERR_OK) { /* Already have an error, so clear any new error */ virResetLastError(); } else { virErrorPtr err = virGetLastError(); if (!err) virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Error while processing monitor IO")); virCopyLastError(&mon->lastError); virResetLastError(); } The only way we could leak memory as described is if mon->lastError had a code of VIR_ERR_OK, and had non-NULL description, which ought to be impossible. 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 Fri, Sep 14, 2012 at 02:24:15PM +0800, Hu Tao wrote:
memset before virResetError will cause memory leak.
virResetError and virCopyError, which calls virResetError, will do memset properly, so we don't have to worry about it here.
--- src/util/virterror.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/src/util/virterror.c b/src/util/virterror.c index 7caa69e..46afd37 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -276,8 +276,6 @@ int virCopyLastError(virErrorPtr to) { virErrorPtr err = virLastErrorObject(); - /* We can't guarantee caller has initialized it to zero */ - memset(to, 0, sizeof(*to)); if (err) virCopyError(err, to); else
Concur with DV's NACK on this. It is the *callers* responsibility to ensure that 'to' does not have any allocations, by calling virResetError(&to) themselvs if needed. 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 :|
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Hu Tao