BeamMP/BeamMP-Launcher

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

if(RemoteVer > FullVersion){

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

  1. 3 == 3
  2. 1 == 1
  3. 9 < 10 -> LOWER

Or, for 1.2.3 < 2.0.0:

  1. 1 < 2 -> LOWER

Or, for 1.2.3 < 1.3.0:

  1. 1 == 1
  2. 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.