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@zte.com.cn>
Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
---
 src/rpc/virnetsocket.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

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);
-
+  
     if (ff)
         ff(eopaque);
 
@@ -2207,6 +2205,7 @@ void virNetSocketUpdateIOCallback(virNetSocketPtr sock,
 
 void virNetSocketRemoveIOCallback(virNetSocketPtr sock)
 {
+    virObjectRef(sock);
     virObjectLock(sock);
 
     if (sock->watch < 0) {
@@ -2220,6 +2219,7 @@ void virNetSocketRemoveIOCallback(virNetSocketPtr sock)
     sock->watch = -1;
 
     virObjectUnlock(sock);
+    virObjectRef(sock);
 }
 
 void virNetSocketClose(virNetSocketPtr sock)
-- 
1.8.3.1


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@zte.com.cn>
> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> ---
>  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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list