[libvirt] [PATCH] rpc : fix a access for null pointer

[Michal Privoznik]

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 at zte.com.cn>
> Signed-off-by: Peng Hao <peng.hao2 at 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