From a1d1c49cf59b3ef2f320e4820daa7c06cea4dffd Mon Sep 17 00:00:00 2001 From: Robert Adam Date: Sat, 6 Jul 2024 14:10:51 +0200 Subject: [PATCH] FIX(server): Stale username cache-entries due to case differences Mumble treats usernames case-insensitively. However, in the server, there is a cache that maps usernames to IDs of registered users. We do lazy loading so when a given name is not yet in the cache, we check the database for a match and if that returns something, we insert the result into the cache. However, the DB does case-insensitive lookups whereas the cache used to be case-sensitive. Therefore, different casings of the same username could end up in the cache (all pointing to the same user ID). If that user got unregistered, only one of the possible ways of writing their name gets removed from the cache, leaving the other entries (which are now stale) untouched. This can cause problems when another user now wants to register with a name that corresponds to one of those stale cache entries. The server will think that the given username is already taken, ensuring leading to a rejection of the registration request. A server restart purges the stale entries from the cache. This commit ensures that the cache is now also operating in a case-insensitive manner such that we shouldn't ever create any duplicate entries in there in the first place. Fixes #6383 --- src/QtUtils.cpp | 74 +++++++++++++++ src/QtUtils.h | 47 ++++++++++ src/murmur/Server.h | 3 +- src/tests/CMakeLists.txt | 1 + .../TestCaseInsensitiveQString/CMakeLists.txt | 12 +++ .../TestCaseInsensitiveQString.cpp | 91 +++++++++++++++++++ 6 files changed, 227 insertions(+), 1 deletion(-) create mode 100644 src/tests/TestCaseInsensitiveQString/CMakeLists.txt create mode 100644 src/tests/TestCaseInsensitiveQString/TestCaseInsensitiveQString.cpp diff --git a/src/QtUtils.cpp b/src/QtUtils.cpp index fb08cce6687..41ddad6f948 100644 --- a/src/QtUtils.cpp +++ b/src/QtUtils.cpp @@ -3,6 +3,8 @@ // that can be found in the LICENSE file at the root of the // Mumble source tree or at . +#include "QtUtils.h" + #include #include #include @@ -23,6 +25,78 @@ namespace QtUtils { return QString(); } + CaseInsensitiveQString::CaseInsensitiveQString(const QString &str) : m_str(str) {} + + CaseInsensitiveQString::CaseInsensitiveQString(QString &&str) : m_str(std::move(str)) {} + + CaseInsensitiveQString &CaseInsensitiveQString::operator=(const QString &str) { + m_str = str; + + return *this; + } + + CaseInsensitiveQString &CaseInsensitiveQString::operator=(QString &&str) { + m_str = std::move(str); + + return *this; + } + + CaseInsensitiveQString::operator const QString &() const { return m_str; } + + CaseInsensitiveQString::operator QString &() { return m_str; } + + bool operator==(const QString &lhs, const CaseInsensitiveQString &rhs) { + return lhs.compare(rhs.m_str, Qt::CaseInsensitive) == 0; + } + + bool operator==(const CaseInsensitiveQString &lhs, const CaseInsensitiveQString &rhs) { return lhs.m_str == rhs; } + + bool operator==(const CaseInsensitiveQString &lhs, const QString &rhs) { return rhs == lhs; } + + bool operator!=(const QString &lhs, const CaseInsensitiveQString &rhs) { return !(lhs == rhs); } + + bool operator!=(const CaseInsensitiveQString &lhs, const CaseInsensitiveQString &rhs) { return !(lhs == rhs); } + + bool operator!=(const CaseInsensitiveQString &lhs, const QString &rhs) { return !(lhs == rhs); } + + bool operator<(const QString &lhs, const CaseInsensitiveQString &rhs) { + return lhs.compare(rhs.m_str, Qt::CaseInsensitive) < 0; + } + + bool operator<(const CaseInsensitiveQString &lhs, const CaseInsensitiveQString &rhs) { return lhs.m_str < rhs; } + + bool operator<(const CaseInsensitiveQString &lhs, const QString &rhs) { return rhs >= lhs; } + + bool operator<=(const QString &lhs, const CaseInsensitiveQString &rhs) { + return lhs.compare(rhs.m_str, Qt::CaseInsensitive) <= 0; + } + + bool operator<=(const CaseInsensitiveQString &lhs, const CaseInsensitiveQString &rhs) { return lhs.m_str <= rhs; } + + bool operator<=(const CaseInsensitiveQString &lhs, const QString &rhs) { return rhs >= lhs; } + + bool operator>(const QString &lhs, const CaseInsensitiveQString &rhs) { + return lhs.compare(rhs.m_str, Qt::CaseInsensitive) > 0; + } + + bool operator>(const CaseInsensitiveQString &lhs, const CaseInsensitiveQString &rhs) { return lhs.m_str > rhs; } + + bool operator>(const CaseInsensitiveQString &lhs, const QString &rhs) { + return rhs <= lhs; + } + + bool operator>=(const QString &lhs, const CaseInsensitiveQString &rhs) { + return lhs.compare(rhs.m_str, Qt::CaseInsensitive) >= 0; + } + + bool operator>=(const CaseInsensitiveQString &lhs, const CaseInsensitiveQString &rhs) { return lhs.m_str >= rhs; } + + bool operator>=(const CaseInsensitiveQString &lhs, const QString &rhs) { return rhs <= lhs; } } // namespace QtUtils } // namespace Mumble + +uint qHash(const Mumble::QtUtils::CaseInsensitiveQString &str, uint seed) { + const QString &lower = static_cast< const QString & >(str).toLower(); + return qHash(lower, seed); +} diff --git a/src/QtUtils.h b/src/QtUtils.h index 6fb0bd52216..546c73ceef1 100644 --- a/src/QtUtils.h +++ b/src/QtUtils.h @@ -34,9 +34,56 @@ namespace QtUtils { */ QString decode_first_utf8_qssl_string(const QStringList &list); + /** + * A wrapper around a QString object that ensures all comparisons and hashes are performed + * in a case-insensitive manner. + */ + class CaseInsensitiveQString { + public: + CaseInsensitiveQString() = default; + ~CaseInsensitiveQString() = default; + + CaseInsensitiveQString(const CaseInsensitiveQString &) = default; + CaseInsensitiveQString(CaseInsensitiveQString &&) = default; + CaseInsensitiveQString &operator=(const CaseInsensitiveQString &) = default; + CaseInsensitiveQString &operator=(CaseInsensitiveQString &&) = default; + + CaseInsensitiveQString(const QString &str); + CaseInsensitiveQString(QString &&str); + CaseInsensitiveQString &operator=(const QString &str); + CaseInsensitiveQString &operator=(QString &&str); + + operator const QString &() const; + operator QString &(); + + friend bool operator==(const QString &lhs, const CaseInsensitiveQString &rhs); + friend bool operator==(const CaseInsensitiveQString &lhs, const CaseInsensitiveQString &rhs); + friend bool operator==(const CaseInsensitiveQString &lhs, const QString &rhs); + friend bool operator!=(const QString &lhs, const CaseInsensitiveQString &rhs); + friend bool operator!=(const CaseInsensitiveQString &lhs, const CaseInsensitiveQString &rhs); + friend bool operator!=(const CaseInsensitiveQString &lhs, const QString &rhs); + friend bool operator<(const QString &lhs, const CaseInsensitiveQString &rhs); + friend bool operator<(const CaseInsensitiveQString &lhs, const CaseInsensitiveQString &rhs); + friend bool operator<(const CaseInsensitiveQString &lhs, const QString &rhs); + friend bool operator<=(const QString &lhs, const CaseInsensitiveQString &rhs); + friend bool operator<=(const CaseInsensitiveQString &lhs, const CaseInsensitiveQString &rhs); + friend bool operator<=(const CaseInsensitiveQString &lhs, const QString &rhs); + friend bool operator>(const QString &lhs, const CaseInsensitiveQString &rhs); + friend bool operator>(const CaseInsensitiveQString &lhs, const CaseInsensitiveQString &rhs); + friend bool operator>(const CaseInsensitiveQString &lhs, const QString &rhs); + friend bool operator>=(const QString &lhs, const CaseInsensitiveQString &rhs); + friend bool operator>=(const CaseInsensitiveQString &lhs, const CaseInsensitiveQString &rhs); + friend bool operator>=(const CaseInsensitiveQString &lhs, const QString &rhs); + + private: + QString m_str; + }; + } // namespace QtUtils } // namespace Mumble +uint qHash(const Mumble::QtUtils::CaseInsensitiveQString &str, uint seed = 0); + template< typename T > using qt_unique_ptr = std::unique_ptr< T, decltype(&Mumble::QtUtils::deleteQObject) >; /// Creates a new unique_ptr with custom deleter for any given QObject* diff --git a/src/murmur/Server.h b/src/murmur/Server.h index dd644e784e4..9887d022248 100644 --- a/src/murmur/Server.h +++ b/src/murmur/Server.h @@ -24,6 +24,7 @@ #include "User.h" #include "Version.h" #include "VolumeAdjustment.h" +#include "QtUtils.h" #include "database/ConnectionParameter.h" @@ -319,7 +320,7 @@ public slots: ChanACL::ACLCache acCache; QHash< int, QString > qhUserNameCache; - QHash< QString, int > qhUserIDCache; + QHash< Mumble::QtUtils::CaseInsensitiveQString, int > qhUserIDCache; std::vector< Ban > m_bans; diff --git a/src/tests/CMakeLists.txt b/src/tests/CMakeLists.txt index b32005175bd..dc7f8191175 100644 --- a/src/tests/CMakeLists.txt +++ b/src/tests/CMakeLists.txt @@ -27,6 +27,7 @@ if(server) endif() # Shared tests +add_subdirectory("TestCaseInsensitiveQString") add_subdirectory("TestCryptographicHash") add_subdirectory("TestCryptographicRandom") add_subdirectory("TestDatabase") diff --git a/src/tests/TestCaseInsensitiveQString/CMakeLists.txt b/src/tests/TestCaseInsensitiveQString/CMakeLists.txt new file mode 100644 index 00000000000..44a18407710 --- /dev/null +++ b/src/tests/TestCaseInsensitiveQString/CMakeLists.txt @@ -0,0 +1,12 @@ +# Copyright 2024 The Mumble Developers. All rights reserved. +# Use of this source code is governed by a BSD-style license +# that can be found in the LICENSE file at the root of the +# Mumble source tree or at . + +add_executable(TestCaseInsensitiveQString TestCaseInsensitiveQString.cpp) + +set_target_properties(TestCaseInsensitiveQString PROPERTIES AUTOMOC ON) + +target_link_libraries(TestCaseInsensitiveQString PRIVATE shared Qt5::Test) + +add_test(NAME TestCaseInsensitiveQString COMMAND $) diff --git a/src/tests/TestCaseInsensitiveQString/TestCaseInsensitiveQString.cpp b/src/tests/TestCaseInsensitiveQString/TestCaseInsensitiveQString.cpp new file mode 100644 index 00000000000..de9a2a8b679 --- /dev/null +++ b/src/tests/TestCaseInsensitiveQString/TestCaseInsensitiveQString.cpp @@ -0,0 +1,91 @@ +// Copyright 2024 The Mumble Developers. All rights reserved. +// Use of this source code is governed by a BSD-style license +// that can be found in the LICENSE file at the root of the +// Mumble source tree or at . + +#include "QtUtils.h" + +#include +#include +#include + +using namespace Mumble::QtUtils; + +class TestCaseInsensitiveQString : public QObject { + Q_OBJECT +private slots: + + void equality() const { + CaseInsensitiveQString istr("test"); + + QVERIFY(QString("Test") == istr); + QVERIFY(istr == QString("TEST")); + QVERIFY(istr == istr); + } + + void inequality() const { + CaseInsensitiveQString istr("test"); + + QVERIFY(QString("Tezt") != istr); + QVERIFY(istr != QString("TETS")); + QVERIFY(istr != CaseInsensitiveQString("test2")); + } + + void less_than() const { + CaseInsensitiveQString istr("test"); + + QVERIFY(QString("abc") < istr); + QVERIFY(istr < QString("xyz")); + QVERIFY(CaseInsensitiveQString("another") < istr); + } + + void less_equal() const { + CaseInsensitiveQString istr("test"); + + QVERIFY(QString("Abc") <= istr); + QVERIFY(istr <= QString("xyz")); + QVERIFY(CaseInsensitiveQString("Taxi") <= istr); + + QVERIFY(QString("teSt") <= istr); + QVERIFY(istr <= QString("Test")); + QVERIFY(CaseInsensitiveQString("TEST") <= istr); + } + + void greater_than() const { + CaseInsensitiveQString istr("test"); + + QVERIFY(QString("xyz") > istr); + QVERIFY(istr > QString("abc")); + QVERIFY(CaseInsensitiveQString("Xenia") > istr); + } + + void greater_equal() const { + CaseInsensitiveQString istr("test"); + + QVERIFY(QString("xyz") >= istr); + QVERIFY(istr >= QString("abc")); + QVERIFY(CaseInsensitiveQString("Xenia") >= istr); + + QVERIFY(QString("Test") >= istr); + QVERIFY(istr >= QString("teST")); + QVERIFY(CaseInsensitiveQString("TeSt") >= istr); + } + + void hash() const { + QVERIFY(qHash(CaseInsensitiveQString("test")) == qHash(CaseInsensitiveQString("TEST"))); + QVERIFY(qHash(CaseInsensitiveQString("test")) != qHash(CaseInsensitiveQString("TESTER"))); + } + + void use_in_qhash() const { + QHash< CaseInsensitiveQString, int > hash; + hash.insert(QString("TeSt"), 42); + + QVERIFY(hash.contains(QString("test"))); + QCOMPARE(hash[QString("test")], 42); + QCOMPARE(static_cast< const QString & >(hash.begin().key()), QString("TeSt")); + QVERIFY(static_cast< const QString & >(hash.begin().key()) != QString("test")); + } +}; + +QTEST_MAIN(TestCaseInsensitiveQString) +#include "TestCaseInsensitiveQString.moc"