[libvirt] [PATCH] Memory Leak Fix: Check def->info->alias before assigning

[Daniel P. Berrange]

On Wed, Nov 27, 2013 at 09:04:17PM +0530, Nehal J Wani wrote:
> > The 'alias' attribute should *not* be parsed from the XML provided by
> > the user. It should only be parsed in the live state XML. In the latter
> > case no codepath should take us to qemuAssignDeviceAliases. So this is
> > certainly not the right fix. It sounds like the test case is flawed to
> > me.
> So, I gave it another round to test the remaining error thrown by
> valgrind. Here is my finding:
> 
> One of the errors received while running valgrind is (not fixed by the
> patch given before):
> 
> ==3915== 9 bytes in 1 blocks are definitely lost in loss record 31 of 129
> ==3915==    at 0x4A0887C: malloc (vg_replace_malloc.c:270)
>     ==3915==    by 0x34268AF275: xmlStrndup (in /usr/lib64/libxml2.so.2.9.1)
>     ==3915==    by 0x4CCD393: virDomainDeviceInfoParseXML.isra.31
> (domain_conf.c:3439)
>     ==3915==    by 0x4CD6A49: virDomainChrDefParseXML (domain_conf.c:7258)
>     ==3915==    by 0x4CEC0C9: virDomainDeviceDefParse (domain_conf.c:9616)
>     ==3915==    by 0x41D14F: testQemuHotplug (qemuhotplugtest.c:247)
>     ==3915==    by 0x41E421: virtTestRun (testutils.c:139)
>     ==3915==    by 0x41C6E3: mymain (qemuhotplugtest.c:428)
>     ==3915==    by 0x41EAB2: virtTestMain (testutils.c:600)
>     ==3915==    by 0x341F421A04: (below main) (libc-start.c:225)
> 
> The corresponding fix is:
> 
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index ec3b958..b7d5390 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -5384,7 +5384,7 @@ qemuMonitorJSONAttachCharDev(qemuMonitorPtr mon,
>              goto cleanup;
>          }
> 
> -        if (VIR_STRDUP(chr->data.file.path, path) < 0)
> +        if (!chr->data.file.path && VIR_STRDUP(chr->data.file.path, path) < 0)
>              goto cleanup;
>      }
> 
> 
> But I can't confirm whether the testcase is wrong in this case too.

The stack trace indicates that the path came from the XML document
parser. This code snippet you show here though only runs when

    if (chr->type == VIR_DOMAIN_CHR_TYPE_PTY) {


For the PTY type chardevs, it is not valid to specify or parse a
pty path from the XML. This is an output-only attribute, not under
user control. So I think it looks like the parsing code is wrong,
not this monitor code.


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