From 5e9bdccd92b271abdd3dedcd5ab9ee586da2fb94 Mon Sep 17 00:00:00 2001 From: Michal Privoznik Date: Mon, 25 Nov 2019 15:13:17 +0100 Subject: [PATCH] remote_daemon_stream: Hold an extra reference to stream in daemonStreamFilter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In v5.9.0-273-g8ecab214de I've tried to fix a lock ordering problem, but introduced a crasher. Problem is that because the client lock is unlocked (in order to honour lock ordering) the stream we are currently checking in daemonStreamFilter() might be freed and thus stream->priv might not even exist when the control get to virMutexLock() call. To resolve this, grab an extra reference to the stream and handle its cleanup should the refcounter reach zero after the deref. If that's the case and we are the only ones holding a reference to the stream, we MUST return a positive value to make virNetServerClientDispatchRead() break its loop where it iterates over filters. The problem is, if we did not do so, then "filter = filter->next" line will read from a memory that was just freed (freeing a stream also unregisters its filter). Signed-off-by: Michal Privoznik Reviewed-by: Ján Tomko --- src/remote/remote_daemon_stream.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/remote/remote_daemon_stream.c b/src/remote/remote_daemon_stream.c index 82cadb67ac..73e4d7befb 100644 --- a/src/remote/remote_daemon_stream.c +++ b/src/remote/remote_daemon_stream.c @@ -293,10 +293,25 @@ daemonStreamFilter(virNetServerClientPtr client, daemonClientStream *stream = opaque; int ret = 0; + /* We must honour lock ordering here. Client private data lock must + * be acquired before client lock. Bu we are already called with + * client locked. To avoid stream disappearing while we unlock + * everything, let's increase its refcounter. This has some + * implications though. */ + stream->refs++; virObjectUnlock(client); virMutexLock(&stream->priv->lock); virObjectLock(client); + if (stream->refs == 1) { + /* So we are the only ones holding the reference to the stream. + * Return 1 to signal to the caller that we've processed the + * message. And to "process" means free. */ + virNetMessageFree(msg); + ret = 1; + goto cleanup; + } + if (msg->header.type != VIR_NET_STREAM && msg->header.type != VIR_NET_STREAM_HOLE) goto cleanup; @@ -318,6 +333,10 @@ daemonStreamFilter(virNetServerClientPtr client, cleanup: virMutexUnlock(&stream->priv->lock); + /* Don't pass client here, because client is locked here and this + * function might try to lock it again which would result in a + * deadlock. */ + daemonFreeClientStream(NULL, stream); return ret; }