ka maybe have been freeed in virObjectUnref, application using
virKeepAliveTimer will segfault when unlock ka. We should keep
ka's refs positive before using it.

Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
---
 src/rpc/virkeepalive.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/src/rpc/virkeepalive.c b/src/rpc/virkeepalive.c
index c9faf88..4f666fd 100644
--- a/src/rpc/virkeepalive.c
+++ b/src/rpc/virkeepalive.c
@@ -160,17 +160,17 @@ virKeepAliveTimer(int timer ATTRIBUTE_UNUSED, void *opaque)
     bool dead;
     void *client;
 
+    virObjectRef(ka);
     virObjectLock(ka);
 
     client = ka->client;
     dead = virKeepAliveTimerInternal(ka, &msg);
 
+    virObjectUnlock(ka);
+
     if (!dead && !msg)
         goto cleanup;
 
-    virObjectRef(ka);
-    virObjectUnlock(ka);
-
     if (dead) {
         ka->deadCB(client);
     } else if (ka->sendCB(client, msg) < 0) {
@@ -178,11 +178,8 @@ virKeepAliveTimer(int timer ATTRIBUTE_UNUSED, void *opaque)
         virNetMessageFree(msg);
     }
 
-    virObjectLock(ka);
-    virObjectUnref(ka);
-
  cleanup:
-    virObjectUnlock(ka);
+    virObjectUnref(ka);
 }
 
 
-- 
1.8.3.1


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

On 04/18/2017 03:55 AM, Yi Wang wrote:
> ka maybe have been freeed in virObjectUnref, application using
> virKeepAliveTimer will segfault when unlock ka. We should keep
> ka's refs positive before using it.
>
> Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> ---
>  src/rpc/virkeepalive.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/src/rpc/virkeepalive.c b/src/rpc/virkeepalive.c
> index c9faf88..4f666fd 100644
> --- a/src/rpc/virkeepalive.c
> +++ b/src/rpc/virkeepalive.c
> @@ -160,17 +160,17 @@ virKeepAliveTimer(int timer ATTRIBUTE_UNUSED, void *opaque)
>      bool dead;
>      void *client;
>
> +    virObjectRef(ka);
>      virObjectLock(ka);
>
>      client = ka->client;
>      dead = virKeepAliveTimerInternal(ka, &msg);
>
> +    virObjectUnlock(ka);
> +
>      if (!dead && !msg)
>          goto cleanup;
>
> -    virObjectRef(ka);
> -    virObjectUnlock(ka);
> -
>      if (dead) {
>          ka->deadCB(client);
>      } else if (ka->sendCB(client, msg) < 0) {
> @@ -178,11 +178,8 @@ virKeepAliveTimer(int timer ATTRIBUTE_UNUSED, void *opaque)
>          virNetMessageFree(msg);
>      }
>
> -    virObjectLock(ka);
> -    virObjectUnref(ka);
> -
>   cleanup:
> -    virObjectUnlock(ka);
> +    virObjectUnref(ka);
>  }
>
>
>

Something doesn't feel right here. Firstly, there should always be at 
least one reference for this callback to use obtained in 
virKeepAliveStart(). So we should be able to do virObjectLock(ka) safely 
here. If we are not, something else is messing up the ref counting.

Secondly, it shouldn't matter if we do ref() followed by lock() or the 
other way around. The first seems racy; well decreasing chance to hit a 
race but nor resolving it.

Do you have a reproducer or something? backtrace perhaps? We can 
investigate further.

Michal

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

On 04/18/2017 03:55 AM, Yi Wang wrote:
> ka maybe have been freeed in virObjectUnref, application using
> virKeepAliveTimer will segfault when unlock ka. We should keep
> ka's refs positive before using it.
>
> Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> ---
>  src/rpc/virkeepalive.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)

So after discussion in the other thread [1], I've squashed in the 
backtrace you posted there, ACKed and pushed.

Congratulations on your first libvirt contribution!

Michal

1: https://www.redhat.com/archives/libvir-list/2017-April/msg01024.html

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