Bug 903213 - Various valgrind errors in libvirt.git
Various valgrind errors in libvirt.git
Status: CLOSED UPSTREAM
Product: Fedora
Classification: Fedora
Component: libvirt (Show other bugs)
rawhide
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Libvirt Maintainers
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-23 09:02 EST by Richard W.M. Jones
Modified: 2013-01-24 15:56 EST (History)
10 users (show)

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


Attachments (Terms of Use)

  None (edit)
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.

Note You need to log in before you can comment on or make changes to this bug.