FIX(client): Crash when opening connect dialog #6307
Conversation
|
@Krzmbrzl: My proposed fix would be this. It's cleaner and does not require a cast at all. What do you think? If you like it, I can create a proper patch with my author information. diff --git a/src/mumble/ConnectDialog.cpp b/src/mumble/ConnectDialog.cpp
index 8da030018..c07f3fce3 100644
--- a/src/mumble/ConnectDialog.cpp
+++ b/src/mumble/ConnectDialog.cpp
@@ -1468,7 +1468,7 @@ void ConnectDialog::fillList() {
while (!ql.isEmpty()) {
#if QT_VERSION >= QT_VERSION_CHECK(5, 10, 0)
ServerItem *si = static_cast< ServerItem * >(
- ql.takeAt(static_cast< int >(QRandomGenerator::global()->generate()) % ql.count()));
+ ql.takeAt(QRandomGenerator::global()->bounded(ql.count())));
#else
// Qt 5.10 introduces the QRandomGenerator class and in Qt 5.15 qrand got deprecated in its favor
ServerItem *si = static_cast< ServerItem * >(ql.takeAt(qrand() % ql.count()));
|
|
It will still require casts (as Qt uses signed sizes but the random API uses unsigned integers) but it saves the modulo operation (or at least hides it in Qt internals). |
|
No, it does not require casts. Feel free to adapt your patch with my suggestion if you give credits in the commit message with |
|
Ah indeed - there is an overload for signed integers. Thanks for pointing that out 👍 |
|
Regarding FreeBSD: |
Do you want me to submit a separate pull request instead? Can you not merge your PR when the FreeBSD build has failed? The crash makes Mumble unusable right now, a fast fix would be appreciated. |
When selecting new public servers, we randomize their order. The random number used for that is an unsigned integer from which we compute the modulus with the list size in order to get a random index into the list. This involved an implicit conversion from an unsigned integer (the random number) to a signed integer (the list index - as Qt uses signed integers for that). Thus, b5a67c0 added an explicit cast to a signed integer. However, it did this to the random number before the modulus operation, meaning that if a large enough random number is drawn, the cast would actually create a negative integer, yielding a negative list index after the modulus. This caused a crash of the application. The fix consists in switching to using the bounded() function that has an overload for signed integers and which can directly generate numbers in the desired range, making all of the above superfluous. Fixes mumble-voip#6306 Co-authored-by: Thomas Lange <[email protected]>
Krzmbrzl
force-pushed
the
fix-connectdialog-crash
branch
from
31cb390 to
41fac28
Compare
No - if we start to merge failing CIs, we might as well stop having CI in the first place. Imo CI is the same as (unit) tests: if you start accepting that some (sometimes) fail, they become mostly useless.
Agreed. I only was quite busy this week. Didn't get to work on this earlier. I expect to merge this now as soon as the CI has finished :) |
|
Thanks for the fix and sorry for my impatience. 😃 Since you removed the Qt version check now, I would recommend to require 5.10.0 with CMake. |
|
Yeah, at this point we should require 5.15 as a preparation for a migration to Qt6 |
When selecting new public servers, we randomize their order. The random number used for that is an unsigned integer from which we compute the modulus with the list size in order to get a random index into the list.
This involved an implicit conversion from an unsigned integer (the random number) to a signed integer (the list index - as Qt uses signed integers for that). Thus, b5a67c0 added an explicit cast to a signed integer. However, it did this to the random number before the modulus operation, meaning that if a large enough random number is drawn, the cast would actually create a negative integer, yielding a negative list index after the modulus. This caused a crash of the application.
The fix consists in switching to using the bounded() function that has an overload for signed integers and which can directly generate numbers in the desired range, making all of the above superfluous.
Fixes #6306
Checks