External/libvirt
1
0
Fork 0
mirror of https://gitlab.com/libvirt/libvirt.git synced 2024-07-06 09:55:46 +00:00
Code Issues Packages Projects Releases Wiki Activity

remote_daemon_stream: Hold an extra reference to stream in daemonStreamFilter

Browse Source 
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 <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This commit is contained in:
Michal Privoznik 2019-11-25 15:13:17 +01:00
parent 21986f5047
commit 5e9bdccd92
1 changed files with 19 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
src/remote/remote_daemon_stream.c
View File

@ -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;
}