Re: [libvirt] [PATCH] fix memory leak in virCopyLastError
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 :|