[libvirt] [PATCH] remoteClientCloseFunc: Don't mangle connection object refcount

[Pavel Hrdina]

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 at redhat.com>
> ---
> 
> 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