Bug 903213

Summary: Various valgrind errors in libvirt.git
Product: [Fedora] Fedora Reporter: Richard W.M. Jones <rjones>
Component: libvirtAssignee: Libvirt Maintainers <libvirt-maint>
Status: CLOSED UPSTREAM QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: rawhideCC: berrange, clalancette, crobinso, itamar, jforbes, jyang, laine, libvirt-maint, veillard, virt-maint
Target Milestone: ---   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2013-01-24 15:56:25 EST Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Description Richard W.M. Jones 2013-01-23 09:02:26 EST
Description of problem:

I'm testing libvirtd using libvirt from git
(commit 682c79c4f5496ef1d21cab52cad0ba609bb0b9cf) and there
are various valgrind errors.

==30020== Conditional jump or move depends on uninitialised value(s)
==30020==    at 0x5125DBD: linuxNodeInfoCPUPopulate (nodeinfo.c:513)
==30020==    by 0x51261A0: nodeGetInfo (nodeinfo.c:884)
==30020==    by 0x149B9B10: qemuCapsInit (qemu_capabilities.c:846)
==30020==    by 0x14A11B25: qemuCreateCapabilities (qemu_driver.c:424)
==30020==    by 0x14A12426: qemuStartup (qemu_driver.c:874)
==30020==    by 0x512A7AF: virStateInitialize (libvirt.c:822)
==30020==    by 0x40DE04: daemonRunStateInit (libvirtd.c:877)
==30020==    by 0x50ADCE5: virThreadHelper (virthreadpthread.c:161)
==30020==    by 0x328CA07D14: start_thread (pthread_create.c:308)
==30020==    by 0x328C6F246C: clone (clone.S:114)
(happened twice)

        if (socks > nodeinfo->sockets)    <--- here
            nodeinfo->sockets = socks;

This appears to be caused because nodeinfo->sockets is not
initialized earlier in the linuxNodeInfoCPUPopulate function
(unlike nodeinfo->cores, nodeinfo->nodes which are both initialized
to zero).


==30020== Invalid read of size 1
==30020==    at 0x4A091A2: strlen (mc_replace_strmem.c:403)
==30020==    by 0x5087EC7: virHashStrCode (virhash.c:78)
==30020==    by 0x508839B: virHashRemoveEntry (virhash.c:100)
==30020==    by 0x149E09BF: qemuDriverCloseCallbackRun (qemu_conf.c:537)
==30020==    by 0x50885C8: virHashForEach (virhash.c:514)
==30020==    by 0x149E294D: qemuDriverCloseCallbackRunAll (qemu_conf.c:549)
==30020==    by 0x14A27CB7: qemuClose (qemu_driver.c:1229)
==30020==    by 0x512163E: virConnectDispose (datatypes.c:145)
==30020==    by 0x50A016A: virObjectUnref (virobject.c:264)
==30020==    by 0x512AD3C: virConnectClose (libvirt.c:1492)
==30020==    by 0x42D83C: remoteClientFreeFunc (remote.c:682)
==30020==    by 0x51966D1: virNetServerClientDispose (virnetserverclient.c:744)
==30020==  Address 0x1794f250 is 0 bytes inside a block of size 37 free'd
==30020==    at 0x4A077A6: free (vg_replace_malloc.c:446)
==30020==    by 0x506FF28: virFree (viralloc.c:419)
==30020==    by 0x5087B72: virHashStrFree (virhash.c:93)
==30020==    by 0x508841B: virHashRemoveEntry (virhash.c:470)
==30020==    by 0x149E2720: qemuDriverCloseCallbackUnset (qemu_conf.c:477)
==30020==    by 0x149E904A: qemuProcessStop (qemu_process.c:4211)
==30020==    by 0x149E988B: qemuProcessAutoDestroy (qemu_process.c:4584)
==30020==    by 0x149E099F: qemuDriverCloseCallbackRun (qemu_conf.c:533)
==30020==    by 0x50885C8: virHashForEach (virhash.c:514)
==30020==    by 0x149E294D: qemuDriverCloseCallbackRunAll (qemu_conf.c:549)
==30020==    by 0x14A27CB7: qemuClose (qemu_driver.c:1229)
==30020==    by 0x512163E: virConnectDispose (datatypes.c:145)
(happened twice)

The code is:

    dom = closeDef->cb(data->driver, dom, data->conn);    # line 533
    if (dom)
        virObjectUnlock(dom);

    virHashRemoveEntry(data->driver->closeCallbacks, uuidstr);  # line 537

I'm not totally clear about what's happening, but it appears
that uuidstr must be freed as a side-effect of the call to
closeDef->cb, and then you're using the freed string in the
call to virHashRemoveEntry.  Possibly swapping the two clauses
around should fix this.


==30020== Invalid read of size 4
==30020==    at 0x5088B0F: virHashCodeGen (virhashcode.c:82)
==30020==    by 0x508839B: virHashRemoveEntry (virhash.c:100)
==30020==    by 0x149E09BF: qemuDriverCloseCallbackRun (qemu_conf.c:537)
==30020==    by 0x50885C8: virHashForEach (virhash.c:514)
==30020==    by 0x149E294D: qemuDriverCloseCallbackRunAll (qemu_conf.c:549)
==30020==    by 0x14A27CB7: qemuClose (qemu_driver.c:1229)
==30020==    by 0x512163E: virConnectDispose (datatypes.c:145)
==30020==    by 0x50A016A: virObjectUnref (virobject.c:264)
==30020==    by 0x512AD3C: virConnectClose (libvirt.c:1492)
==30020==    by 0x42D83C: remoteClientFreeFunc (remote.c:682)
==30020==    by 0x51966D1: virNetServerClientDispose (virnetserverclient.c:744)
==30020==    by 0x50A016A: virObjectUnref (virobject.c:264)
==30020==  Address 0x1794f250 is 0 bytes inside a block of size 37 free'd
==30020==    at 0x4A077A6: free (vg_replace_malloc.c:446)
==30020==    by 0x506FF28: virFree (viralloc.c:419)
==30020==    by 0x5087B72: virHashStrFree (virhash.c:93)
==30020==    by 0x508841B: virHashRemoveEntry (virhash.c:470)
==30020==    by 0x149E2720: qemuDriverCloseCallbackUnset (qemu_conf.c:477)
==30020==    by 0x149E904A: qemuProcessStop (qemu_process.c:4211)
==30020==    by 0x149E988B: qemuProcessAutoDestroy (qemu_process.c:4584)
==30020==    by 0x149E099F: qemuDriverCloseCallbackRun (qemu_conf.c:533)
==30020==    by 0x50885C8: virHashForEach (virhash.c:514)
==30020==    by 0x149E294D: qemuDriverCloseCallbackRunAll (qemu_conf.c:549)
==30020==    by 0x14A27CB7: qemuClose (qemu_driver.c:1229)
==30020==    by 0x512163E: virConnectDispose (datatypes.c:145)
(seen once)

This seems to be a variation of the previous error, but along
a slightly different code path.
Comment 1 Daniel Berrange 2013-01-23 11:14:26 EST
(In reply to comment #0)
> Description of problem:
> 
> I'm testing libvirtd using libvirt from git
> (commit 682c79c4f5496ef1d21cab52cad0ba609bb0b9cf) and there
> are various valgrind errors.
> 
> ==30020== Conditional jump or move depends on uninitialised value(s)
> ==30020==    at 0x5125DBD: linuxNodeInfoCPUPopulate (nodeinfo.c:513)
> ==30020==    by 0x51261A0: nodeGetInfo (nodeinfo.c:884)
> ==30020==    by 0x149B9B10: qemuCapsInit (qemu_capabilities.c:846)
> ==30020==    by 0x14A11B25: qemuCreateCapabilities (qemu_driver.c:424)
> ==30020==    by 0x14A12426: qemuStartup (qemu_driver.c:874)
> ==30020==    by 0x512A7AF: virStateInitialize (libvirt.c:822)
> ==30020==    by 0x40DE04: daemonRunStateInit (libvirtd.c:877)
> ==30020==    by 0x50ADCE5: virThreadHelper (virthreadpthread.c:161)
> ==30020==    by 0x328CA07D14: start_thread (pthread_create.c:308)
> ==30020==    by 0x328C6F246C: clone (clone.S:114)
> (happened twice)
> 
>         if (socks > nodeinfo->sockets)    <--- here
>             nodeinfo->sockets = socks;
> 
> This appears to be caused because nodeinfo->sockets is not
> initialized earlier in the linuxNodeInfoCPUPopulate function
> (unlike nodeinfo->cores, nodeinfo->nodes which are both initialized
> to zero).

Fixed upstream in

commit 5750c2e740cb8cdb2f5a2cf86bb5c256c21340e8
Author: Daniel P. Berrange <berrange@redhat.com>
Date:   Wed Jan 23 15:43:21 2013 +0000

    Ensure nodeinfo struct is initialized to zero
    
    When linuxNodeInfoCPUPopulate() method triggered use of an
    uninitialize value, since it did not initialize the 'sockets'
    field in the virNodeInfoPtr struct:
Comment 2 Daniel Berrange 2013-01-23 12:29:49 EST
(In reply to comment #0)
> 
> ==30020== Invalid read of size 1
> ==30020==    at 0x4A091A2: strlen (mc_replace_strmem.c:403)
> ==30020==    by 0x5087EC7: virHashStrCode (virhash.c:78)
> ==30020==    by 0x508839B: virHashRemoveEntry (virhash.c:100)
> ==30020==    by 0x149E09BF: qemuDriverCloseCallbackRun (qemu_conf.c:537)
> ==30020==    by 0x50885C8: virHashForEach (virhash.c:514)
> ==30020==    by 0x149E294D: qemuDriverCloseCallbackRunAll (qemu_conf.c:549)
> ==30020==    by 0x14A27CB7: qemuClose (qemu_driver.c:1229)
> ==30020==    by 0x512163E: virConnectDispose (datatypes.c:145)
> ==30020==    by 0x50A016A: virObjectUnref (virobject.c:264)
> ==30020==    by 0x512AD3C: virConnectClose (libvirt.c:1492)
> ==30020==    by 0x42D83C: remoteClientFreeFunc (remote.c:682)
> ==30020==    by 0x51966D1: virNetServerClientDispose
> (virnetserverclient.c:744)
> ==30020==  Address 0x1794f250 is 0 bytes inside a block of size 37 free'd
> ==30020==    at 0x4A077A6: free (vg_replace_malloc.c:446)
> ==30020==    by 0x506FF28: virFree (viralloc.c:419)
> ==30020==    by 0x5087B72: virHashStrFree (virhash.c:93)
> ==30020==    by 0x508841B: virHashRemoveEntry (virhash.c:470)
> ==30020==    by 0x149E2720: qemuDriverCloseCallbackUnset (qemu_conf.c:477)
> ==30020==    by 0x149E904A: qemuProcessStop (qemu_process.c:4211)
> ==30020==    by 0x149E988B: qemuProcessAutoDestroy (qemu_process.c:4584)
> ==30020==    by 0x149E099F: qemuDriverCloseCallbackRun (qemu_conf.c:533)
> ==30020==    by 0x50885C8: virHashForEach (virhash.c:514)
> ==30020==    by 0x149E294D: qemuDriverCloseCallbackRunAll (qemu_conf.c:549)
> ==30020==    by 0x14A27CB7: qemuClose (qemu_driver.c:1229)
> ==30020==    by 0x512163E: virConnectDispose (datatypes.c:145)
> (happened twice)
> 
> The code is:
> 
>     dom = closeDef->cb(data->driver, dom, data->conn);    # line 533
>     if (dom)
>         virObjectUnlock(dom);
> 
>     virHashRemoveEntry(data->driver->closeCallbacks, uuidstr);  # line 537
> 
> I'm not totally clear about what's happening, but it appears
> that uuidstr must be freed as a side-effect of the call to
> closeDef->cb, and then you're using the freed string in the
> call to virHashRemoveEntry.  Possibly swapping the two clauses
> around should fix this.

The 'uuidstr' parameter in qemuDriverCloseCallbackRun is a pointer to the hash key. closeDef->cb terminates the guest, causing the close callback to be removed. This means the hash key is freed, and thus 'uuidstr' now points to free memory.