fix: use display version in registration packet to avoid exceeding MAX_LEN_VERSION_TEXT

[mcfnord]

Bug

Dev builds from a dirty git tree silently fail to register with a directory server.

The build system produces APP_VERSION strings like 3.11.0dev-a91a593-dirty:1776526795 (34 chars) for dirty git builds. CreateCLRegisterServerExMes encodes this verbatim in the registration packet. The receiving directory calls GetStringFromStream(vecData, iPos, MAX_LEN_VERSION_TEXT, strVersion) where MAX_LEN_VERSION_TEXT = 30. When the string exceeds 30 chars, GetStringFromStream returns an error — the registration handler exits early and sends no response. The registering server sees only 500 ms timeouts, retries five times, and reports Registration failed.

Why it wasn't caught

• Release builds: 3.11.0 — 6 chars ✓
• Dev builds without git: 3.11.0dev-nogit — 15 chars ✓
• Dev builds, clean git tree: 3.11.0dev-<7chars>:<10chars> — 28 chars ✓ (just under the limit)
• Dev builds, dirty git tree: 3.11.0dev-<7chars>-dirty:<10chars> — 34 chars ✗

The -dirty suffix (6 chars) is what pushes it over. CI always builds clean, so this path was never exercised.

Fix

VERSION (GetDisplayVersion(APP_VERSION)) already exists to strip the :timestamp build artifact for external/display use. CreateCLRegisterServerExMes should use VERSION rather than APP_VERSION, consistent with the intent of that split.

3.11.0dev-a91a593-dirty → 23 chars, safely under the limit.

Testing

Confirmed on a dirty-tree dev build: before fix, five registration attempts with zero responses from directory; after fix, registers successfully on first attempt.

[mcfnord]

This might be by design. I can imagine keeping "dirty" builds of the server off all directories but one (AG3?). An error message would help if this behavior is intended.

[softins]

This might be by design. I can imagine keeping "dirty" builds of the server off all directories but one (AG3?). An error message would help if this behavior is intended.

No, I don't think it's by design, but rather an oversight. Thanks for catching this.

[softins]

This was changed in #3562, to allow the connect dialog to sort versions by age. However, to do this, the connect dialog fetches the version numbers directly from each server. The timestamp suffix does not need to be sent to the directory when registering, as I think it is just used to enable the directory to refuse registrations from old versions of server.

@pljones, do you agree?

[pljones]

Oddly I was looking at this earlier. I failed to think of the dirty case, though. Good spot!

Does it break the protocol?

[softins]

Does it break the protocol?

No, the protocol has a 2-byte field for the string length. However, there are multiple places where MAX_LEN_VERSION_TEXT is used to limit the length of the accepted string:

• CProtocol::EvaluateVersionAndOSMes()
• CProtocol::EvaluateCLRegisterServerExMes() (the one found by @mcfnord)
• CProtocol::EvaluateCLVersionAndOSMes()

So the same change as proposed would have to be made to the corresponding CProtocol::Create... functions. But that would then mean the connect dialog doesn't get to see the timestamp information it needs.

So I think the correct fix, instead of changing the version number sent, is to increase MAX_LEN_VERSION_TEXT from 30 to, say 40 or 50, There is precedent for that: in Nov 2020, Volker increased it from 20 to 30.

If we do that now, then any directory of 3.12.0+ would accept registrations with the longer version numbers that currently fail. And the connect dialog in any clients of 3.12.0+ would correctly accept such version numbers too.

I can't think of any other compatibility downside to doing that.

[pljones]

OK. So the vast majority of Servers register with a release cut, anyway, so they're not affected one way or the other. It's the Directories we're worried about updating -- and anyone running a Server with a long ("dirty") version not being able to register properly.

Get a fixed version done -- I'd go for 50 to be on the safe side -- and I'll get it onto my private Directories (jamulusdirectory2, 6 and 8, all .drealm.info - they always run off HEAD of main) once it's merged.