Re: [libvirt] [PATCH] bugfix: add lock in ClientClose to avoid data race

On 03/01/2018 04:43 AM, xinhua.Cao wrote: The description above is not easy to read/understand without looking at the code as well. Took a while to chase through these and understand what the issue is (or at least what it appears to be). So the problem is partially that remoteClientFreePrivateCallbacks is called without taking the priv->lock. But it may also be partially that the *EventCallback Register/Deregister API's will check conn->priv and then take the priv->lock (unlike the sasl API code). The solution you have come up with is to add a bool @clientClosed to daemonClientPrivate to be set when remoteClientCloseFunc is fired. Then in the various remoteDispatchConnect*EventRegister calls, you check if that bool is set before continuing. IOW: Don't allow registering a new one if it's determined that remoteClientCloseFunc already ran. So my first thought is - why limit to just those API's? Couldn't any one of the API's that calls virNetServerClientGetPrivateData to fetch @priv be "interrupted [in]appropriately" prior to checking "if (!priv->conn)"? A @priv that could have been deleted by something else? I'm probably over thinking though ;-) Anyway, let's assume that cannot happen. Let's assume that checking priv->conn is still "good enough". Thus there's two things that I'd like to see tested/done before adding a new boolean: 1. Rather the check "if (!priv->conn)" before the virMutexLock, move the virMutexLock(&priv->lock); to before the if condition. You'll note the "if" failure will "goto cleanup" where cleanup: will virMutexUnlock even though we don't have the lock. We don't check the error, so no big deal. This should be a separate/initial patch. 2. Add the virMutexLock before remoteClientFreePrivateCallbacks and virMutexUnlock afterwards. This would be a second patch. Can you reproduce the stack trace under those conditions? If so, then there may be a really nasty TOCTOU problem which could be quite invasive to resolve properly. If not, then it seems "priv->conn" is "good enough" as the gating condition. If you can reproduce the condition, then since remoteClientCloseFunc is essentially being in the middle of a close is either of the client fields "delayedClose" or "wantClose" already set? (easy enough to check while debugging). If so, can we use that knowledge somehow to not allow new EventCallbacks to be added? I think the "worst" that could happen with the proper locking in place for remoteClientFreePrivateCallbacks is some event callback is added and leaked because we were able to RegisterAny after remoteClientCloseFunc ran. In which case, so what, we were dying anyway... NB: Couple of other things to think about below... ^^^^^^^^^^ This needs to be a more proper name. If we find we really need to add it, I think this bool should be moved before the various EventCallback defs and renamed to "inhibitEventAdd" (or something similar) with a comment before it indicating how it'll be used. "This will be set when the remoteClientCloseFunc is called due to socket closure as a result of a client process being killed. It will ensure no other thread could be adding to the various client private *EventCallbacks" If we find we need to add it, then the message could be "connection closing". John