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