On Thu, Sep 03, 2015 at 12:31:46PM +0200, Michal Privoznik wrote: > Well, in 8ad126e6 we tried to fix a memory corruption problem. > However, the fix was not as good as it could be. I mean, the > commit has one line more than it should. I've noticed this output > just recently: > > # ./run valgrind --leak-check=full --show-reachable=yes ./tools/virsh domblklist gentoo > ==17019== Memcheck, a memory error detector > ==17019== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al. > ==17019== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info > ==17019== Command: /home/zippy/work/libvirt/libvirt.git/tools/.libs/virsh domblklist gentoo > ==17019== > Target Source > ------------------------------------------------ > fda /var/lib/libvirt/images/fd.img > vda /var/lib/libvirt/images/gentoo.qcow2 > hdc /home/zippy/tmp/install-amd64-minimal-20150402.iso > > ==17019== Thread 2: > ==17019== Invalid read of size 4 > ==17019== at 0x4EFF5B4: virObjectUnref (virobject.c:258) > ==17019== by 0x5038CFF: remoteClientCloseFunc (remote_driver.c:552) > ==17019== by 0x5069D57: virNetClientCloseLocked (virnetclient.c:685) > ==17019== by 0x506C848: virNetClientIncomingEvent (virnetclient.c:1852) > ==17019== by 0x5082136: virNetSocketEventHandle (virnetsocket.c:1913) > ==17019== by 0x4ECD64E: virEventPollDispatchHandles (vireventpoll.c:509) > ==17019== by 0x4ECDE02: virEventPollRunOnce (vireventpoll.c:658) > ==17019== by 0x4ECBF00: virEventRunDefaultImpl (virevent.c:308) > ==17019== by 0x130386: vshEventLoop (vsh.c:1864) > ==17019== by 0x4F1EB07: virThreadHelper (virthread.c:206) > ==17019== by 0xA8462D3: start_thread (in /lib64/libpthread-2.20.so) > ==17019== by 0xAB441FC: clone (in /lib64/libc-2.20.so) > ==17019== Address 0x139023f4 is 4 bytes inside a block of size 240 free'd > ==17019== at 0x4C2B1F0: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) > ==17019== by 0x4EA8949: virFree (viralloc.c:582) > ==17019== by 0x4EFF6D0: virObjectUnref (virobject.c:273) > ==17019== by 0x4FE74D6: virConnectClose (libvirt.c:1390) > ==17019== by 0x13342A: virshDeinit (virsh.c:406) > ==17019== by 0x134A37: main (virsh.c:950) > > The problem is, when registering remoteClientCloseFunc(), it's > conn->closeCallback which is ref'd. But in the function itself > it's conn->closeCallback->conn what is unref'd. This is causing > imbalance in reference counting. Moreover, there's no need for > the remote driver to increase/decrease conn refcount since it's > not used anywhere. It's just merely passed to client registered > callback. And for that purpose it's correctly ref'd in > virConnectRegisterCloseCallback() and then unref'd in > virConnectUnregisterCloseCallback(). > > Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx> > --- > > Not only this is nice because it fixes a bug by removing some lines, it maybe > needs to be backported all the way down to v1.0.5 main branches. > > src/remote/remote_driver.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c > index ec26ebe..aca00c0 100644 > --- a/src/remote/remote_driver.c > +++ b/src/remote/remote_driver.c > @@ -546,10 +546,6 @@ remoteClientCloseFunc(virNetClientPtr client ATTRIBUTE_UNUSED, > cbdata->freeCallback = NULL; > } > virObjectUnlock(cbdata); > - > - /* free the connection reference that comes along with the callback > - * registration */ > - virObjectUnref(cbdata->conn); > } > > /* helper macro to ease extraction of arguments from the URI */ > -- > 2.4.6 ACK Pavel -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list