On 07/15/2017 05:00 PM, Peng Hao wrote:
> virNetSocketRemoveIOCallback get sock's ObjectLock and will call
> virNetSocketEventFree. virNetSocketEventFree may be free sock
> object and virNetSocketRemoveIOCallback will access a null pointer
> in release sock's ObjectLock.
>
> Signed-off-by: Liu Yun <liu.yunh@xxxxxxxxxx>
> Signed-off-by: Peng Hao <peng.hao2@xxxxxxxxxx>
> ---
> src/rpc/virnetsocket.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
I don't think this can work.
> diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
> index d228c8a..8b550e8 100644
> --- a/src/rpc/virnetsocket.c
> +++ b/src/rpc/virnetsocket.c
> @@ -2140,14 +2140,12 @@ static void virNetSocketEventFree(void *opaque)
> virFreeCallback ff;
> void *eopaque;
>
> - virObjectLock(sock);
> ff = sock->ff;
> eopaque = sock->opaque;
> sock->func = NULL;
> sock->ff = NULL;
> sock->opaque = NULL;
> - virObjectUnlock(sock);
I think we need the lock here. This function is called from the event
loop thread. So even if virNetSocketUpdateIOCallback() locks the @socket
this code can see it unlocked. Or locked. But the crucial part is it's
modifying the object and thus should have lock held.
> -
> +
> if (ff)
> ff(eopaque);
>
> @@ -2207,6 +2205,7 @@ void virNetSocketUpdateIOCallback(virNetSocketPtr sock,
>
> void virNetSocketRemoveIOCallback(virNetSocketPtr sock)
> {
> + virObjectRef(sock);
> virObjectLock(sock);
I think this is what actually fixes your problem. However, I also think
it introduces uneven ratio of ref:unref calls.
>
> if (sock->watch < 0) {
> @@ -2220,6 +2219,7 @@ void virNetSocketRemoveIOCallback(virNetSocketPtr sock)
> sock->watch = -1;
>
> virObjectUnlock(sock);
> + virObjectRef(sock);
It definitely does so because you ref twice. Anyway, do you perhaps have
a backtrace to share?
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
