From e7000febd3badc8d85206fce9b70796591efeb27 Mon Sep 17 00:00:00 2001 From: Robert Adam Date: Mon, 1 Apr 2024 14:50:54 +0200 Subject: [PATCH 1/3] FIX(server): Stale user pointers in whisper cache Since the whisper cache entry was computed while holding the read-lock, it was possible for a user in that cache to get deleted while the voice thread drops the read lock in order to upgrade it to a write lock. In such a case, the cache entry would contain an invalid (stale) user pointer, which can lead to crashes when using the cache later on. By moving the cache entry computation into the block in which we already hold a read lock, we turn computing the cache and adding it to the cache store into an atomic operation. As soon as the cache entry is in the store, deleting a user is no longer an issue as that will implicitly clear all whisper caches (thereby preventing stale cache entries). --- src/murmur/Server.cpp | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/murmur/Server.cpp b/src/murmur/Server.cpp index 32b574f56fc..e90279e2dd5 100644 --- a/src/murmur/Server.cpp +++ b/src/murmur/Server.cpp @@ -1230,6 +1230,14 @@ void Server::processMsg(ServerUser *u, Mumble::Protocol::AudioData audioData, Au } else { ZoneScopedN(TracyConstants::AUDIO_WHISPER_CACHE_CREATE); + const unsigned int uiSession = u->uiSession; + qrwlVoiceThread.unlock(); + qrwlVoiceThread.lockForWrite(); + + if (!qhUsers.contains(uiSession)) { + return; + } + const WhisperTarget &wt = u->qmTargets.value(static_cast< int >(audioData.targetOrContext)); if (!wt.qlChannels.isEmpty()) { QMutexLocker qml(&qmCache); @@ -1302,13 +1310,9 @@ void Server::processMsg(ServerUser *u, Mumble::Protocol::AudioData audioData, Au } } - unsigned int uiSession = u->uiSession; - qrwlVoiceThread.unlock(); - qrwlVoiceThread.lockForWrite(); + u->qmTargetCache.insert(static_cast< int >(audioData.targetOrContext), + { channel, direct, cachedListeners }); - if (qhUsers.contains(uiSession)) - u->qmTargetCache.insert(static_cast< int >(audioData.targetOrContext), - { channel, direct, cachedListeners }); qrwlVoiceThread.unlock(); qrwlVoiceThread.lockForRead(); if (!qhUsers.contains(uiSession)) From da0bc018042028151294ef938e339215c6d7c8ee Mon Sep 17 00:00:00 2001 From: Robert Adam Date: Sat, 6 Apr 2024 16:51:06 +0200 Subject: [PATCH 2/3] REFAC(server): Compute whisper target caches in dedicated function This should improve the readability of the whisper code path in the important processMsg function. --- src/murmur/Server.cpp | 180 +++++++++++++++++++++++++----------------- src/murmur/Server.h | 2 + 2 files changed, 110 insertions(+), 72 deletions(-) diff --git a/src/murmur/Server.cpp b/src/murmur/Server.cpp index e90279e2dd5..204b283ba18 100644 --- a/src/murmur/Server.cpp +++ b/src/murmur/Server.cpp @@ -1238,86 +1238,23 @@ void Server::processMsg(ServerUser *u, Mumble::Protocol::AudioData audioData, Au return; } - const WhisperTarget &wt = u->qmTargets.value(static_cast< int >(audioData.targetOrContext)); - if (!wt.qlChannels.isEmpty()) { - QMutexLocker qml(&qmCache); - - foreach (const WhisperTarget::Channel &wtc, wt.qlChannels) { - Channel *wc = qhChannels.value(static_cast< unsigned int >(wtc.iId)); - if (wc) { - bool link = wtc.bLinks && !wc->qhLinks.isEmpty(); - bool dochildren = wtc.bChildren && !wc->qlChannels.isEmpty(); - bool group = !wtc.qsGroup.isEmpty(); - if (!link && !dochildren && !group) { - // Common case - if (ChanACL::hasPermission(u, wc, ChanACL::Whisper, &acCache)) { - foreach (User *p, wc->qlUsers) { channel.insert(static_cast< ServerUser * >(p)); } - - foreach (unsigned int currentSession, - m_channelListenerManager.getListenersForChannel(wc->iId)) { - ServerUser *pDst = static_cast< ServerUser * >(qhUsers.value(currentSession)); - - if (pDst) { - addListener(cachedListeners, *pDst, *wc); - } - } - } - } else { - QSet< Channel * > channels; - if (link) - channels = wc->allLinks(); - else - channels.insert(wc); - if (dochildren) - channels.unite(wc->allChildren()); - const QString &redirect = u->qmWhisperRedirect.value(wtc.qsGroup); - const QString &qsg = redirect.isEmpty() ? wtc.qsGroup : redirect; - foreach (Channel *tc, channels) { - if (ChanACL::hasPermission(u, tc, ChanACL::Whisper, &acCache)) { - foreach (User *p, tc->qlUsers) { - ServerUser *su = static_cast< ServerUser * >(p); - - if (!group || Group::appliesToUser(*tc, *tc, qsg, *su)) { - channel.insert(su); - } - } - - foreach (unsigned int currentSession, - m_channelListenerManager.getListenersForChannel(tc->iId)) { - ServerUser *pDst = static_cast< ServerUser * >(qhUsers.value(currentSession)); - - if (pDst && (!group || Group::appliesToUser(*tc, *tc, qsg, *pDst))) { - // Only send audio to listener if the user exists and it is in the group the - // speech is directed at (if any) - addListener(cachedListeners, *pDst, *tc); - } - } - } - } - } - } - } - } + // Create cache entry for the given target + // Note: We have to compute the cache entry and add it to the user's cache store in an atomic + // transaction (ensured by the lock) to avoid running into situations in which a user from the cache + // gets deleted without this particular cache entry being purged (which happens, if the cache entry is + // in the store at the point of deleting the user). + const WhisperTarget &wt = u->qmTargets.value(static_cast< int >(audioData.targetOrContext)); + WhisperTargetCache cache = createWhisperTargetCacheFor(*u, wt); - { - QMutexLocker qml(&qmCache); + u->qmTargetCache.insert(static_cast< int >(audioData.targetOrContext), std::move(cache)); - foreach (unsigned int id, wt.qlSessions) { - ServerUser *pDst = qhUsers.value(id); - if (pDst && ChanACL::hasPermission(u, pDst->cChannel, ChanACL::Whisper, &acCache) - && !channel.contains(pDst)) - direct.insert(pDst); - } - } - - u->qmTargetCache.insert(static_cast< int >(audioData.targetOrContext), - { channel, direct, cachedListeners }); qrwlVoiceThread.unlock(); qrwlVoiceThread.lockForRead(); if (!qhUsers.contains(uiSession)) return; } + // These users receive the audio because someone is shouting to their channel for (ServerUser *pDst : channel) { buffer.addReceiver(*u, *pDst, Mumble::Protocol::AudioContext::SHOUT, audioData.containsPositionalData); @@ -2416,5 +2353,104 @@ bool Server::canNest(Channel *newParent, Channel *channel) const { return (parentLevel + channelDepth) < iChannelNestingLimit; } +WhisperTargetCache Server::createWhisperTargetCacheFor(ServerUser &speaker, const WhisperTarget &target) { + ZoneScoped; + + QMutexLocker qml(&qmCache); + + WhisperTargetCache cache; + + if (!target.qlChannels.isEmpty()) { + for (const WhisperTarget::Channel ¤tTarget : target.qlChannels) { + Channel *targetChannel = qhChannels.value(static_cast< unsigned int >(currentTarget.iId)); + + if (targetChannel) { + bool includeLinks = currentTarget.bLinks && !targetChannel->qhLinks.isEmpty(); + bool includeChildren = currentTarget.bChildren && !targetChannel->qlChannels.isEmpty(); + bool restrictToGroup = !currentTarget.qsGroup.isEmpty(); + + if (!includeLinks && !includeChildren && !restrictToGroup) { + // Common case + if (ChanACL::hasPermission(&speaker, targetChannel, ChanACL::Whisper, &acCache)) { + for (User *p : targetChannel->qlUsers) { + // Add users of the target channel + cache.channelTargets.insert(static_cast< ServerUser * >(p)); + } + + for (unsigned int currentSession : + m_channelListenerManager.getListenersForChannel(targetChannel->iId)) { + // Add users that listen to the target channel (duplicates with users directly + // in this channel are handled further down) + ServerUser *pDst = static_cast< ServerUser * >(qhUsers.value(currentSession)); + + if (pDst) { + addListener(cache.listeningTargets, *pDst, *targetChannel); + } + } + } + } else { + QSet< Channel * > channels; + + if (includeLinks) { + // allLinks contains the channel itself + channels = targetChannel->allLinks(); + } else { + channels.insert(targetChannel); + } + + if (includeChildren) { + channels.unite(targetChannel->allChildren()); + } + + // The target group might be changed by a redirect set up via RPC (Ice/gRPC). In that + // case the shout is sent to the redirection target instead the originally specified group + const QString &redirect = speaker.qmWhisperRedirect.value(currentTarget.qsGroup); + const QString &targetGroup = redirect.isEmpty() ? currentTarget.qsGroup : redirect; + + for (Channel *subTargetChan : channels) { + if (ChanACL::hasPermission(&speaker, subTargetChan, ChanACL::Whisper, &acCache)) { + for (User *p : subTargetChan->qlUsers) { + ServerUser *su = static_cast< ServerUser * >(p); + + if (!restrictToGroup + || Group::appliesToUser(*subTargetChan, *subTargetChan, targetGroup, *su)) { + cache.channelTargets.insert(su); + } + } + + for (unsigned int currentSession : + m_channelListenerManager.getListenersForChannel(subTargetChan->iId)) { + ServerUser *pDst = static_cast< ServerUser * >(qhUsers.value(currentSession)); + + if (pDst + && (!restrictToGroup + || Group::appliesToUser(*subTargetChan, *subTargetChan, targetGroup, *pDst))) { + // Only send audio to listener if the user exists and it is in the group the + // speech is directed at (if any) + addListener(cache.listeningTargets, *pDst, *subTargetChan); + } + } + } + } + } + } + } + } + + for (unsigned int id : target.qlSessions) { + ServerUser *pDst = qhUsers.value(id); + if (pDst && ChanACL::hasPermission(&speaker, pDst->cChannel, ChanACL::Whisper, &acCache) + && !cache.channelTargets.contains(pDst)) + cache.directTargets.insert(pDst); + } + + // Make sure the speaker themselves is not contained in these lists + cache.channelTargets.remove(&speaker); + cache.directTargets.remove(&speaker); + cache.listeningTargets.remove(&speaker); + + return cache; +} + #undef SIO_UDP_CONNRESET #undef SENDTO diff --git a/src/murmur/Server.h b/src/murmur/Server.h index 2cee156c8c9..d8f8724ea0e 100644 --- a/src/murmur/Server.h +++ b/src/murmur/Server.h @@ -203,6 +203,8 @@ class Server : public QThread { QTimer qtTick; void initRegister(); + WhisperTargetCache createWhisperTargetCacheFor(ServerUser &speaker, const WhisperTarget &target); + private: int iChannelNestingLimit; int iChannelCountLimit; From b7a46b20ab6fe3048a7715a3b06a928936b85215 Mon Sep 17 00:00:00 2001 From: Robert Adam Date: Sat, 6 Apr 2024 16:58:02 +0200 Subject: [PATCH 3/3] REFAC(server): Refactor WhisperTarget struct Switch to using an unsigned integer as channel ID and rename member variables to no longer include their type as part of the name (and make names more meaningful in general). Additionally, some Qt containers were replaced with std ones. --- src/murmur/Messages.cpp | 26 +++++++++++++++----------- src/murmur/Server.cpp | 18 +++++++++--------- src/murmur/ServerUser.h | 15 +++++++++------ 3 files changed, 33 insertions(+), 26 deletions(-) diff --git a/src/murmur/Messages.cpp b/src/murmur/Messages.cpp index 50fbbc3f2df..d02fd572c66 100644 --- a/src/murmur/Messages.cpp +++ b/src/murmur/Messages.cpp @@ -2192,26 +2192,30 @@ void Server::msgVoiceTarget(ServerUser *uSource, MumbleProto::VoiceTarget &msg) const MumbleProto::VoiceTarget_Target &t = msg.targets(i); for (int j = 0; j < t.session_size(); ++j) { unsigned int s = t.session(j); - if (qhUsers.contains(s)) - wt.qlSessions << s; + if (qhUsers.contains(s)) { + wt.sessions.push_back(s); + } } if (t.has_channel_id()) { unsigned int id = t.channel_id(); if (qhChannels.contains(id)) { WhisperTarget::Channel wtc; - wtc.iId = static_cast< int >(id); - wtc.bChildren = t.children(); - wtc.bLinks = t.links(); - if (t.has_group()) - wtc.qsGroup = u8(t.group()); - wt.qlChannels << wtc; + wtc.id = id; + wtc.includeChildren = t.children(); + wtc.includeLinks = t.links(); + if (t.has_group()) { + wtc.targetGroup = u8(t.group()); + } + + wt.channels.push_back(wtc); } } } - if (wt.qlSessions.isEmpty() && wt.qlChannels.isEmpty()) + if (wt.sessions.empty() && wt.channels.empty()) { uSource->qmTargets.remove(target); - else - uSource->qmTargets.insert(target, wt); + } else { + uSource->qmTargets.insert(target, std::move(wt)); + } } } diff --git a/src/murmur/Server.cpp b/src/murmur/Server.cpp index 204b283ba18..224897ea0bb 100644 --- a/src/murmur/Server.cpp +++ b/src/murmur/Server.cpp @@ -2360,14 +2360,14 @@ WhisperTargetCache Server::createWhisperTargetCacheFor(ServerUser &speaker, cons WhisperTargetCache cache; - if (!target.qlChannels.isEmpty()) { - for (const WhisperTarget::Channel ¤tTarget : target.qlChannels) { - Channel *targetChannel = qhChannels.value(static_cast< unsigned int >(currentTarget.iId)); + if (!target.channels.empty()) { + for (const WhisperTarget::Channel ¤tTarget : target.channels) { + Channel *targetChannel = qhChannels.value(currentTarget.id); if (targetChannel) { - bool includeLinks = currentTarget.bLinks && !targetChannel->qhLinks.isEmpty(); - bool includeChildren = currentTarget.bChildren && !targetChannel->qlChannels.isEmpty(); - bool restrictToGroup = !currentTarget.qsGroup.isEmpty(); + bool includeLinks = currentTarget.includeLinks && !targetChannel->qhLinks.isEmpty(); + bool includeChildren = currentTarget.includeChildren && !targetChannel->qlChannels.isEmpty(); + bool restrictToGroup = !currentTarget.targetGroup.isEmpty(); if (!includeLinks && !includeChildren && !restrictToGroup) { // Common case @@ -2404,8 +2404,8 @@ WhisperTargetCache Server::createWhisperTargetCacheFor(ServerUser &speaker, cons // The target group might be changed by a redirect set up via RPC (Ice/gRPC). In that // case the shout is sent to the redirection target instead the originally specified group - const QString &redirect = speaker.qmWhisperRedirect.value(currentTarget.qsGroup); - const QString &targetGroup = redirect.isEmpty() ? currentTarget.qsGroup : redirect; + const QString &redirect = speaker.qmWhisperRedirect.value(currentTarget.targetGroup); + const QString &targetGroup = redirect.isEmpty() ? currentTarget.targetGroup : redirect; for (Channel *subTargetChan : channels) { if (ChanACL::hasPermission(&speaker, subTargetChan, ChanACL::Whisper, &acCache)) { @@ -2437,7 +2437,7 @@ WhisperTargetCache Server::createWhisperTargetCacheFor(ServerUser &speaker, cons } } - for (unsigned int id : target.qlSessions) { + for (unsigned int id : target.sessions) { ServerUser *pDst = qhUsers.value(id); if (pDst && ChanACL::hasPermission(&speaker, pDst->cChannel, ChanACL::Whisper, &acCache) && !cache.channelTargets.contains(pDst)) diff --git a/src/murmur/ServerUser.h b/src/murmur/ServerUser.h index dccf491c9a4..e91f4c879f3 100644 --- a/src/murmur/ServerUser.h +++ b/src/murmur/ServerUser.h @@ -27,6 +27,8 @@ # include #endif +#include + // Unfortunately, this needs to be "large enough" to hold // enough frames to account for both short-term and // long-term "maladjustments". @@ -52,13 +54,14 @@ struct BandwidthRecord { struct WhisperTarget { struct Channel { - int iId; - bool bChildren; - bool bLinks; - QString qsGroup; + unsigned int id; + bool includeChildren; + bool includeLinks; + QString targetGroup; }; - QList< unsigned int > qlSessions; - QList< WhisperTarget::Channel > qlChannels; + + std::vector< unsigned int > sessions; + std::vector< WhisperTarget::Channel > channels; }; class ServerUser;