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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list