Build number exceeds version format

[Krzmbrzl]

Description

In the latest stable release (1.4.274) the client will advertise its version as 1.5.18 to the server. This causes issues in case one is connecting to a server that is running Mumble 1.5 or more recent as in that version a new audio protocol has been introduced that is used for all clients advertising themselves as 1.5.0 or higher. However, as the 1.4.274 doesn't know about this new protocol, this will effectively cause a loss of audio for that client.

Steps to reproduce

1. Connect to any server
2. Check the reported client version on that server

Mumble version

1.4.274

Mumble component

Both

OS

Linux

Reproducible?

Yes

Additional information

The issue boils down to how we encode version numbers as integers:

mumble/src/Version.cpp

Line 42 in 789f2d7

return (major << 16) | (minor << 8) | patch;

From this it can be seen that we divide an integer (in the most general case a 32-bit integer) into parts: 16 bits encoding the major version number, 8 bits for the minor version number and 8 bits for the patch number (which we substitute with the build number).

That means that the major version number can be in range 0-65535 and the others can be in range 0-255.

In 1.4.274 the build (patch) number exceeds this range and therefore produces an overflow in this scheme, incrementing the minor version number by one, leading to the observed result.

Relevant log output

No response

Screenshots

No response

[Krzmbrzl]

In order to fix this, I see only 2 choices:

1. Change the versioning scheme (again) to ensure that we never encounter version numbers >255
2. Change the version encoding scheme to allow for larger patch numbers (at the cost of the max value of the other version numbers)

Since I wouldn't want to change the versioning scheme again, I think the only viable option is 2. However, this has to somehow be done in a backwards compatible way in order to not screw up the communication with older clients/servers.

[Hartmnt]

I have not yet looked at the code, yet. If we are lucky, the server advertises its version before the client in the handshake. That would mean a >= 1.4.(274+1) client could conditionally use a new version scheme for a >= 1.4.(274+1) server and report '1.4.255' for all previous server versions, which should be backward compatible as long as patches do not contain breaking changes.

If the client advertises before the server... well that would probably require a hacky workaround.

[ghost]

What happened to this:

https://docs.microsoft.com/en-us/windows/win32/msi/version

Does Linux and macOS not respect this version scheme? The reason for the change initially was because of that and upgrade issues with the installer, I thought. We dropped the final segment because it breaks upgrades for MSI's. I guess I don't understand the reason for the 8 bit type instead of the 16 bit one.

[davidebeatrici]

Exactly, that is the problem: a limitation of our protocol.

If we're lucky we can just shift 1 byte and use it for the third component (build number).

[Hartmnt]

I have looked at the current situation:

The client sends its version to the server here

The server sends its version to the client here

It appears these messages are sent independently, as soon as the connection is established in an non-deterministic order.

Furthermore, every use of uiVersion is also affected by this. set here and used for example here
These would probably have to be updated by using something like this

[davidebeatrici]

A potential solution I proposed in our team chat would be to treat the "blob" sent as the protocol version.

There is no technical requirement in knowing the specific software (client/server) version as I'm pretty sure all checks we have in the code strictly refer to the protocol one.

Also, this change would be beneficial for the eventual switch to libmumble.

[Krzmbrzl]

After having thought about this a bit more, I believe that this would only cure the symptoms and not the underlying issue. We also use this version format inside the client (without sending it anywhere) and there we really want to represent the client's version. So we'd still have to deal with that.
Plus it seems to explicitly using a "protocol version" would require to separately keep track whether a given release has introduced protocol changes and only then incrementing the protocol version. Thus, over time the actual client/server version and the protocol version will diverge, which I believe will cause confusion.

If at all possible, I would much prefer if we find a way to fix the underlying problem without having to resort to tricks like sending a different kind of version instead 👀

[Hartmnt]

Another thing: The version message parsing in client and server seem to be, at first glance, without side effect.

That means the that new server and clients could initially send 1.4.255 and after learning that the other side also reports 1.4.255 simply resend the version message again with an updated version format.

Edit: Of course every instance of the version check in server and client need to be considered and maybe even delayed until the final version is known.

Edit2: Wait, clamping to 255 (for internal protocolVersion checks) and resending the patch separately if applicable for accurate client information would not break any existing code as far as i can imagine!

[davidebeatrici]

Excellent idea.

[Hartmnt]

Quick implementation question: Is the order of optional fields in src/Mumble.proto important? I assume it is.

[Krzmbrzl]

The order itself is not important, but the assigned ID (the numeric value assigned to the field) is important and must not be changed. And it kinda makes sense to have the fields ordered based on their ID 🤷

[Hartmnt]

The order itself is not important, but the assigned ID (the numeric value assigned to the field) is important and must not be changed. And it kinda makes sense to have the fields ordered based on their ID shrug

Yeah, that's what I figured. At first I was thinking to add a separate message for patch information, but since the fields are all optional, we might as well add an optional field to the regular version packet. And aesthetically it would have been nice to put the patch right after the version, but that would break compatibility i think... Anyway I added it to the bottom

[Hartmnt]

Before I have to search for a long time: Where do I set the patch version in the build process for testing purposes? (I can only build the 1.5 branch, so I want to set the patch level manually to >255)

[Krzmbrzl]

Before I have to search for a long time: Where do I set the patch version in the build process for testing purposes? (I can only build the 1.5 branch, so I want to set the patch level manually to >255)

Use -DBUILD_NUMBER=274 when invoking cmake

At first I was thinking to add a separate message for patch information, but since the fields are all optional, we might as well add an optional field to the regular version packet. And aesthetically it would have been nice to put the patch right after the version, but that would break compatibility i think... Anyway I added it to the bottom

Mind creating a draft PR with your changes so far? I am not quite sure I understand what you are currently doing. I currently don't quite see the need to alter the .proto file 🤔

[makedir]

My mumble server is still

can someone here maybe help me where or how I can get a newer package of mumble-server for Ubuntu 20.04.6?

https://launchpad.net/~mumble/+archive/ubuntu/release

There was not an update anymore for the server in some time.

[Hartmnt]

@makedir This is very much the wrong issue for this topic, but here is a rundown:

The PPA is unmaintained, but it will be again eventually. There is no ETA for that though. We also did not manage to get 1.5 into the Debian package freeze, which means it will take a long time to drip down to Debian based distros. For the foreseeable future, you will have to compile Mumble yourself, use a docker image, or stick with the old version.

PPA: mumble-voip/mumble-ubuntu-ppa#16
Docker: https://github.com/mumble-voip/mumble-docker