Re: [libvirt] [PATCH] fix memory leak in virCopyLastError

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.