#3953 closed enhancement (fixed)
[PATCH] Network cleanup - IP address conversion — at Version 6
Reported by: | elexis | Owned by: | elexis |
---|---|---|---|
Priority: | Nice to Have | Milestone: | Alpha 21 |
Component: | Network | Keywords: | patch |
Cc: | Patch: |
Description (last modified by )
- This patch was done because leper didn't like r17772 (#2854) converting the IP-address from a 32bit number to a string and then comparing that against 127.0.0.1.
(I'm not convinced this is less elegant than the proposed patch changing the protocol).
The function
GetIPAddress()
is thus deleted (might be introduced for other purposes later like showing IPs in the GUI). The otherGetIPAddress()
was introduced in r17217 (#3241) and has the same issue, can be removed in a follow-up patch.
- Previously, the timeouts were disabled if the IP is
127.0.0.1
. Now it is disabled only if the server and client are started from the same process, thus working better in some edgecases when clients for example connect via a reverse proxy and thus also had127.0.0.1
.
- To avoid accessing globals the
g_NetServer
andg_NetClient
from inside the others thread (thread safety), the client now needs to inform the server that it is running in the same process, which is now transmitted on authentication.
- There are no security measures taken to prevent arbitrary clients from setting that disableTimeout flag to true, but could be implemented trivially by changing the boolean that is transmitted via network to a shared secret.
- The compiler didn't want to consume a default argument for the ctor of
CNetClient
, thus changes all occurances where it is instantiated.
- Notice
m_HostGUID
is only used to prevent the host client from kicking itself.
Change History (8)
by , 8 years ago
Attachment: | disable_timeouts_differently_v1.patch added |
---|
by , 8 years ago
Attachment: | disable_timeouts_differently_v2.patch added |
---|
comment:1 by , 8 years ago
Cc: | added |
---|
comment:3 by , 8 years ago
Cc: | removed |
---|---|
Keywords: | review removed |
Big thanks to Philip for discussing thoroughly on 2016-05-01 which implementation is to be prefered. This patch wouldn't have been possible without him!
Thanks leper for the proposed improvements and reviewing this on 2016-05-04:
15:54 <@leper> elexis: Re #3953: Should we move the timeout disabling to some function? (or just define that function to nothing in case the used enet version doesn't support it?); IsLocalClient() can be const 15:55 <@leper> looks a lot better 15:55 <@leper> (can be committed with just the const fix, the other thing probably isn't needed, just something to think about)
Defining the function only if enet supports it would also mean that the if-directive would have to be placed in the header and the calls to that function as well, which is a bit meh IMO.
Did the const change and also reverted the removal of m_HostGUID
from the previous patch, as clients can freely impersonate the localhost right now and thus would be able to become unkickable. (The host should remain unkickable as doing so would kill the game)
comment:6 by , 3 years ago
Description: | modified (diff) |
---|
See Phab:D3075 for a further step towards enabling these by making authentication use a shared secret.
Renames the new variable to
isLocalClient
, replacesg_HostGUID
and changes the other mentionedGetIPAddress
function to not convert to string.