Old and new (v3+) Launcher does incorrect version comparison
lionkor opened this issue · 2 comments
The current launcher and the new v3 launcher both to incorrect version checking.
The launcher compares two version strings, like "3.1.8" and "3.1.9", and compares them by lexical ordering. This does NOT work for semantic version numbers, as they may extend beyond one digit per part, such as "3.1.10". see
BeamMP-Launcher/src/Network/Update.cpp
Line 35 in 674aae6
Below is some example code which demonstrates two version numbers, 3.1.9 and 3.1.10, in which case the 3.1.9 is supposed to be LOWER (<) than 3.1.10, but due to lexical ordering is not (9 > 1). (In the code we use >
, but the SAME applies)
#include <iostream>
#include <string>
int main() {
std::string a = "3.1.9";
std::string b = "3.1.10";
std::cout << a << " < " << b << " => " << std::boolalpha << (a < b)
<< std::endl;
}
This outputs: 3.1.9 < 3.1.10 => false
, which is incorrect. This is semantically the same operation the launcher does.
The correct way to compare version numbers is to compare them from the largest to the smallest, so for 3.1.9 < 3.1.10
, it would be:
EDIT: code provided in the next message, which is clearer than this
- 3 == 3
- 1 == 1
- 9 < 10 -> LOWER
Or, for 1.2.3 < 2.0.0
:
- 1 < 2 -> LOWER
Or, for 1.2.3 < 1.3.0
:
- 1 == 1
- 2 < 3 -> LOWER
Any other way is going to introduce bugs. We cannot silently rely on a version number never going beyond 1 digit (especially since it, in the past, has).
A fix would be to use std::string::find
or a similar API to split the string at .
, then converting the resulting substrings to numbers using the <charconv>
function std::from_chars
. To avoid any issues with parsing and error checking, you can regex check with something simple like ^\d+\.\d+\.\d+$
with std::regex
or something like that.
You can then the following logic to properly check for version compatibility:
struct Version {
uint8_t major, minor, patch;
};
static bool NeedsUpdate(Version self, Version remote) {
return self.major < remote.major || // major outdated
(self.major == remote.major &&
self.minor < remote.minor) || // minor outdated
(self.major == remote.major && self.minor == remote.minor &&
self.patch < remote.patch); // patch outdated
}
This code has been tested with the following unit test cases:
assert(!NeedsUpdate({1, 0, 0}, {1, 0, 0}));
assert(!NeedsUpdate({1, 0, 0}, {0, 9, 0}));
assert(!NeedsUpdate({1, 0, 0}, {0, 9, 9}));
assert(!NeedsUpdate({1, 0, 0}, {0, 0, 9}));
assert(!NeedsUpdate({1, 0, 0}, {0, 0, 0}));
assert(NeedsUpdate({1, 0, 0}, {1, 1, 1}));
assert(NeedsUpdate({1, 0, 0}, {1, 1, 0}));
assert(NeedsUpdate({1, 0, 0}, {1, 0, 1}));
assert(NeedsUpdate({1, 1, 1}, {2, 0, 0}));
assert(NeedsUpdate({2, 3, 4}, {2, 5, 2}));
assert(NeedsUpdate({1, 2, 9}, {1, 2, 10}));
assert(!NeedsUpdate({1, 2, 9}, {0, 0, 0}));
None of this was used, but the issue was fixed by instead providing a non-semver version that works with semver. I dont care anymore, fixed.