[libvirt] [PATCH] fix memory leak in virCopyLastError

[Daniel Veillard]

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 at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/