#3949 closed defect (fixed)
[PATCH] Ensure the unique client IDs
Reported by: | elexis | Owned by: | sbirmi |
---|---|---|---|
Priority: | Should Have | Milestone: | Alpha 22 |
Component: | Network | Keywords: | patch |
Cc: | sharadbirmiwal@… | Patch: |
Description (last modified by )
The current network code relies on GUIDs being unique, but doesn't ensure that this is actually the case.
Currently, the clients just pick a random GUID in the constructor of NetClient.cpp
. If a player rejoins, he will have a new GUID too.
The server should disconnect clients with a duplicate GUID or even decide the GUID of the clients (rather than the clients).
Attachments (2)
Change History (23)
comment:1 by , 8 years ago
Component: | Core engine → Network |
---|
comment:2 by , 8 years ago
So if I understand this correctly, it would be very rare that GUIDs are duplicated, but you'd like to make sure it never happens?
You want to focus on the server disconnecting the client, as opposed to doing a check for a duplicate GUID, and if duplicate, generate another GUID?
Right now the client is deciding the GUID, but you'd like the server to decide upon it.
follow-up: 5 comment:3 by , 8 years ago
"The term GUID is generally used by developers working with Microsoft technologies, while UUID is used everywhere else. 128-bits is big enough and the generation algorithm is unique enough that if 1,000,000,000 GUIDs per second were generated for 1 year the probability of a duplicate would be only 50%."
Sounds like a wasted effort to me, unless you think you might have 1 billion people joining your game within 1 second.
comment:4 by , 8 years ago
Disconnecting a client that comes with a GUID that already exists shouldn't be too hard to do though. Furthermore the chance can become 100% if someone would modify the code.
comment:5 by , 8 years ago
Replying to rockytriton:
Sounds like a wasted effort to me, unless you think you might have 1 billion people joining your game within 1 second.
It's only probability, not the real case, so you could get a duplication even on a first pair. Also you should know that GUID is generated by PRNG, not the real RNG, so probability is even more.
comment:6 by , 8 years ago
Cc: | added |
---|---|
Owner: | set to |
comment:7 by , 8 years ago
Keywords: | patch rfc added |
---|---|
Milestone: | Backlog → Alpha 22 |
Summary: | Ensure the unique client IDs → [PATCH] Ensure the unique client IDs |
Yep, looks good. There is a typo though and I'd rather append the new disconnect reason to the end of the enum (so that the existing disconnect reasons keep their ID). Also noone but coders know what a GUID is, so better speak of "Player identifier" and mention that the player should retry.
Bonus question: What does actually happen if a malicious client was to use an existing GUID, (i.e. if the NetClient
replaces that get-guid part with a fixed string)?
by , 8 years ago
Attachment: | unique_guid_rev1.patch added |
---|
First attempt at adding a "unique" guid check when a new client is completing authentication
comment:8 by , 8 years ago
Keywords: | review added; rfc removed |
---|
Updated the diff with the recommendations (also adding my name to credits in the new diff)
comment:9 by , 8 years ago
For the bonus question, changing m_GUID(ps_generate_guid())
to m_GUID("bla")
in the NetClient
is required.
In order to test the bonus question (or the patch itself), one has to open 0AD once to host a game and join in a second window. On Windows this is only possible as a late-observer (as the filesystem prohibits simultaenous file access when starting the game in two windows simultaneously)
comment:10 by , 8 years ago
Hi elexis,
Applied the following diff (as suggested):
Index: network/NetClient.cpp =================================================================== --- network/NetClient.cpp (revision 19084) +++ network/NetClient.cpp (working copy) @@ -69,7 +69,7 @@ CNetClient::CNetClient(CGame* game, bool isLocalClient) : m_Session(NULL), m_UserName(L"anonymous"), - m_GUID(ps_generate_guid()), m_HostID((u32)-1), m_ClientTurnManager(NULL), m_Game(game), + m_GUID("foo"), m_HostID((u32)-1), m_ClientTurnManager(NULL), m_Game(game), m_GameAttributes(game->GetSimulation2()->GetScriptInterface().GetContext()), m_IsLocalClient(isLocalClient), m_LastConnectionCheck(0),
If you had a unit test in mind for this, can you point me to an existing test that spawn a server and a client instance?
Thanks, Sharad
comment:11 by , 8 years ago
Adding a unit test is actually recommended. We do have test_Net.h
that sets up a server and some clients. Since there is no use-case to change m_GUID
of a client, there is no such function (f.e. SetGUID
) to accomplish that. It would have to be added just for that test. But if it were added, then it would incentivize developers to use it outside of the tests, so I would actually argue against adding that function and the test (We shouldn't add functions that shouldn't be used).
I rather meant just compiling your code with the client patch and without the server patch. Then starting pyrogenesis, enabling "Late Observers" in the options, then clicking on multiplayer->host->start game. Then starting pyrogenesis again, clicking on multiplayer -> join to join the running game as observer. Thus either the server crashes somewhere or you got some impersonation thing going on I expect, giving reason to merge your proposed server patch.
comment:12 by , 8 years ago
Ah okay.
Tried it out (only the client patch, no server fix). When a duplicate guid user connects, there is no crash but the original (first) user isn't able to send any instructions anymore. I wasn't able to assign tasks to units, get buildings to do anything. The new "observer" also doesn't have any control over the units.
Closing the observer first didn't result in a crash. Control wasn't resumed to the first player.
Note that I tested this with a multiplayer game with 1 human and 1 AI originally (and added a spectator later).
follow-up: 15 comment:14 by , 8 years ago
Hmm.. tried doing this again. Now, after the observer connects, within a few seconds, I end up getting a "out of sync in turn N" message.
Specifically about chat messages, I don't see a crash. They appear to originate from the same username (I had to provide different names otherwise the game wouldn't let the observer connect).
Messages sent from an observer to observers-only appears in the player's screen.
Messages from the player to allies or enemies don't appear on the observer's screen, messages sent to everyone or observers-only appear on the observer's screen.
comment:15 by , 8 years ago
Replying to sbirmi:
Hmm.. tried doing this again. Now, after the observer connects, within a few seconds, I end up getting a "out of sync in turn N" message.
Wouldn't be surprised if an out of sync is due to sending simulation commands.
(I had to provide different names otherwise the game wouldn't let the observer connect)
(That's intended)
Messages from the player to allies or enemies don't appear on the observer's screen, messages sent to everyone or observers-only appear on the observer's screen.
(As intended)
Specifically about chat messages, I don't see a crash. They appear to originate from the same username Messages sent from an observer to observers-only appears in the player's screen.
So the worst that could happen is someone joining as observer, grabbing the GUID of some player and then being able to chat with the allies, despite not being an assigned player?
comment:16 by , 8 years ago
No. The worst (and this happens everytime) is a denial of service. The original player isn't able to control his/her units anymore. S/he can try clicking on his/her units and passing instructions but none of the commands get followed.
comment:17 by , 8 years ago
Also, if the observer disconnects, control isn't restored to the original player. I didn't check if the original player could disconnect and reconnect to resume control but by that time, precious time may have been lost already.
comment:18 by , 8 years ago
Thanks for the patch and thorough testing, the patch receives my recommendation to be added! :-)
comment:20 by , 8 years ago
Keywords: | simple review removed |
---|
Thanks again and a sorry from the team for letting your patch rot!
comment:21 by , 7 years ago
Description: | modified (diff) |
---|
In Phab:rP20341:
The server should choose the guid
Reviewed by: elexis
Differential Revision: https://code.wildfiregames.com/D943
(changed component to network)