search

legacyclonk / LegacyClonk

The LegacyClonk engine and the c4group command line tool.
https://clonkspot.org/lc-en
Other
83 stars 18 forks source link

Reserve space for public ipv4 address determined by netpuncher-client in the reference when hosting a game #67

Open Somebodyisnobody opened 2 years ago

Somebodyisnobody commented 2 years ago

To get the NATted public IPv4-Adress of a game host the client transmits 0.0.0.0:tcpport and 0.0.0.0:udpport to the masterserver. The masterserver replaces these two addresses with the clients remote address. In case of a connnection to the masterserver via IPv6 the masterserver replaces 0.0.0.0 with the client's IPv6-Adress used to send the reference. To get the public IPv4 address into the reference in this case, the embedded netpuncher-client asks the netpuncher server for the client's IPv4 address. Afterwards the netpuncher-client updates the reference with the IPv4 address and the update is sent to the masterserver. The original function letting the masterserver replace the public IPv4 address in the reference is only important for hosting clients who have connection issues with the netpuncher OR legacy clients like Clonk Rage (CR).

Here comes the problem: The Clonk engine (CR and LC) has a magic limit of 40 addresses with (at this time) unknown reason. When a hosting client has already >38 addresses in the reference AND transmits this reference via IPv6 to the masterserver, the netpuncher fails to add the public IPv4 address (1x tcp, 1x udp). This results in a reference wich has no public IPv4 address contained.

Suggestion: I suggest to reserve space in the 40 addresses for the public ipv4 address. It must be made sure that the netpuncher only adds two addresses (one IPv4 with tcp/udp) to the reference. (I have no use case for more than one public IPv4 address.) The netpuncher continues acting as resolver for the public IPv4 and the existing function replacing the IPv4 by the masterserver is legacy code. IPv4 and IPv6 are used to send the reference to the masterserver as it is in the current situation. @lluchs aggrees on this and would also remove the limit. @Fulgen301 aggrees on this, wouldn't remove a limit completely but raise it a bit.

Thoughts on raising the limit:

Somebodyisnobody commented 2 years ago

@maxmitti Do you still have the issue in mind (that it can be closed with [356]?). You did something on this in d91b75796c619a61f158c79ea3f89d9c772d7bf3. Does this also remove the address limit?

maxmitti commented 2 years ago

In the near future, I am not planning to invest much time on the network improvements branch. At the moment, I am too busy already. Anyway, it is probably best to cherry-pick the desired changes.

I believe that https://github.com/legacyclonk/LegacyClonk/commit/d91b75796c619a61f158c79ea3f89d9c772d7bf3 removes the address limit on the client side, but the masterserver will still limit it. I am also not too sure if removing the limit altogether is a good idea.

Edit: Sorry, I did not remember that the fix is already on master. In this case it should be fine already. Ideally you could test it?

Somebodyisnobody commented 2 years ago

I believe that d91b757 removes the address limit on the client side, but the masterserver will still limit it. I am also not too sure if removing the limit altogether is a good idea.

Edit: Sorry, I did not remember that the fix is already on master. In this case it should be fine already. Ideally you could test it?

Tested and i got 62 Addresses from the masterserver. Beginnign with 2 IPv4 Adresses. Are you sure that there is such an feature implemented in the masterserver to limit the addresses and this could be a bug or did you just assume that it has a limit?

Somebodyisnobody commented 2 years ago

Looked into clonkspot/league/ lib/game.class.php, seems to have no limit.

maxmitti commented 2 years ago

You are probably right. However, there is a limit for the size of the whole reference: https://github.com/clonkspot/league/blob/master/table_structure.sql#L218

The limit apparently is 64KB: https://electrictoolbox.com/maximum-length-mysql-text-field-types/

Somebodyisnobody commented 2 years ago

Okay personally i see this as problem of the backend. LC can also check for such problems but the backend is the critical point that must reject invalid requests. I'll open an issue there.