[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]