search

Unity-Technologies / multiplayer-community-contributions

Community contributions to Unity Multiplayer Networking products and services.
MIT License
429 stars 161 forks source link

SteamNetworkingTransport accepts messages from unconnected clients #139

Closed A1win closed 1 year ago

A1win commented 2 years ago

The SteamNetworkingTransport accepts network messages from clients that aren't connected to the server. It appears to be a feature of Steam networking that messages can be sent and received without first establishing a connection. The SteamNetworkingTransport doesn't take this into account and can cause errors when it happens.

One such case can be reproduced as follows:

  1. Host a server using SteamNetworkingTransport layer by calling NetworkManager.Singleton.StartHost().
  2. Connect to the server on a different machine/Steam account by setting the ConnectToSteamID field in SteamNetworkingTransport to the host's Steam ID and then calling NetworkManager.Singleton.StartClient().
  3. Force quit the server, for example with Alt+F4 in Windows, by killing the process, or by exiting playmode if hosting in Unity Editor.
  4. Restart the server on the same Steam account.
  5. Shut down the client that was previously connected to the first instance of the hosted server.
  6. Wait for a while and observe the following error on the host (check log file if not hosting in Unity Editor and not using a developer build):
KeyNotFoundException: The given key was not present in the dictionary.
System.Collections.Generic.Dictionary`2[TKey,TValue].get_Item (TKey key) (at <695d1cc93cca45069c528c15c9fdd749>:0)
Unity.Netcode.NetworkManager.TransportIdToClientId (System.UInt64 transportId) (at Library/PackageCache/com.unity.netcode.gameobjects@1.0.0-pre.4/Runtime/Core/NetworkManager.cs:1285)
Unity.Netcode.NetworkManager.HandleRawTransportPoll (Unity.Netcode.NetworkEvent networkEvent, System.UInt64 clientId, System.ArraySegment`1[T] payload, System.Single receiveTime) (at Library/PackageCache/com.unity.netcode.gameobjects@1.0.0-pre.4/Runtime/Core/NetworkManager.cs:1354)
Unity.Netcode.NetworkManager.OnNetworkEarlyUpdate () (at Library/PackageCache/com.unity.netcode.gameobjects@1.0.0-pre.4/Runtime/Core/NetworkManager.cs:1177)
Unity.Netcode.NetworkManager.NetworkUpdate (Unity.Netcode.NetworkUpdateStage updateStage) (at Library/PackageCache/com.unity.netcode.gameobjects@1.0.0-pre.4/Runtime/Core/NetworkManager.cs:1152)
Unity.Netcode.NetworkUpdateLoop.RunNetworkUpdateStage (Unity.Netcode.NetworkUpdateStage updateStage) (at Library/PackageCache/com.unity.netcode.gameobjects@1.0.0-pre.4/Runtime/Core/NetworkUpdateLoop.cs:149)
Unity.Netcode.NetworkUpdateLoop+NetworkEarlyUpdate+<>c.<CreateLoopSystem>b__0_0 () (at Library/PackageCache/com.unity.netcode.gameobjects@1.0.0-pre.4/Runtime/Core/NetworkUpdateLoop.cs:172)

It also appears that the shutdown logic in SteamNetworkingTransport only works on a controlled shutdown (see Discord log below).

A client that isn't connected to a server shouldn't be able to cause errors on the server by sending it network messages. The server shouldn't accept any messages from such a client.

Initial conversation from Unity Multiplayer Networking Discord (#support):

[11:49 AM] A1win: @Luke Using the fixed Steam Networking Transport, I hosted a server, connected to it as a client, forced shutdown on the server by exiting playmode in editor (also reproduced using a build as the server and exiting with alt-F4), hosted a server again, and then shut down the client which had connected during the previous instance of the server. The server generated this error after a while:

KeyNotFoundException: The given key was not present in the dictionary.
System.Collections.Generic.Dictionary`2[TKey,TValue].get_Item (TKey key) (at <695d1cc93cca45069c528c15c9fdd749>:0)
Unity.Netcode.NetworkManager.TransportIdToClientId (System.UInt64 transportId) (at Library/PackageCache/com.unity.netcode.gameobjects@1.0.0-pre.4/Runtime/Core/NetworkManager.cs:1285)
Unity.Netcode.NetworkManager.HandleRawTransportPoll (Unity.Netcode.NetworkEvent networkEvent, System.UInt64 clientId, System.ArraySegment`1[T] payload, System.Single receiveTime) (at Library/PackageCache/com.unity.netcode.gameobjects@1.0.0-pre.4/Runtime/Core/NetworkManager.cs:1354)
Unity.Netcode.NetworkManager.OnNetworkEarlyUpdate () (at Library/PackageCache/com.unity.netcode.gameobjects@1.0.0-pre.4/Runtime/Core/NetworkManager.cs:1177)
Unity.Netcode.NetworkManager.NetworkUpdate (Unity.Netcode.NetworkUpdateStage updateStage) (at Library/PackageCache/com.unity.netcode.gameobjects@1.0.0-pre.4/Runtime/Core/NetworkManager.cs:1152)
Unity.Netcode.NetworkUpdateLoop.RunNetworkUpdateStage (Unity.Netcode.NetworkUpdateStage updateStage) (at Library/PackageCache/com.unity.netcode.gameobjects@1.0.0-pre.4/Runtime/Core/NetworkUpdateLoop.cs:149)
Unity.Netcode.NetworkUpdateLoop+NetworkEarlyUpdate+<>c.<CreateLoopSystem>b__0_0 () (at Library/PackageCache/com.unity.netcode.gameobjects@1.0.0-pre.4/Runtime/Core/NetworkUpdateLoop.cs:172)

We've also got client disconnected events firing on the server sometimes by following those steps. [11:52 AM] Luke: Does the client get disconnected when you alt-F4 the server or does it stay "connected" until the new server is started again? [11:53 AM] A1win: I think it gets disconnected, at least its player object gets destroyed [11:53 AM] A1win: Although I think I don't see the disconnected message that I've set up. I can check [11:54 AM] Luke: hosted a server again, and then shut down the client which had connected during the previous instance of the server. trying to understand this. You shouldn't have to shutdown the client if the disconnect happens properly. [11:54 AM] A1win: Our server isn't currently sending a shutdown notification to clients when it shuts down, but it should probably not allow clients from a previous session to continue to do stuff [11:55 AM] A1win: It doesn't do the disconnect properly at the moment [11:55 AM] Luke: Okay so my guess would be that it stays connected and still think it's connected when you restart a new server. [11:58 AM] A1win: I'll add some more logging to see what's going on better [12:00 PM] A1win: Neither OnClientConnectedCallback or OnClientDisconnectCallback got called on the 2nd instance of the server before getting the error above [12:02 PM] A1win: Is it safe to assume that the client isn't able to do anything on the new instance of the server though? It's able to cause that KeyNotFoundException on the server but maybe it doesn't break anything. [12:02 PM] Luke: That exception could break something [12:03 PM] A1win: Even if the server did normally do proper shutdown and disconnect, it's still possible that it sometimes doesn't, and a client from a previous session could cause that exception again [12:04 PM] Luke: https://github.com/Unity-Technologies/multiplayer-community-contributions/blob/a37a913b4c3ae3021ff86ab9b2402bacfcf1a0fb/Transports/com.community.netcode.transport.steamnetworking/Runtime/SteamNetworkingTransport.cs#L315 Pretty sure the fix would be to add a check here whether the data we received is from a connected client. [12:04 PM] A1win: Mm pretty sure the client was not a connected client [12:05 PM] Luke: Yeah the problem with steamnetworking is that anyone can send a message to anyone without needing to create a connection first. [12:05 PM] A1win: Though not sure how to properly check that, but at least the connected event didn't get fired [12:06 PM] Luke: So the client from the last session which hasn't been disconnected yet will still send messages to the already closed server. These messages then arrive on the newly started server to which the client never connected. [12:06 PM] A1win: Yeah, would be nice if those can be automatically ignored by Netcode :X [12:08 PM] Luke: Maybe it's just a matter of properly closing the connections. It could be that the transport does not get correctly shutdown if you force quit. [12:09 PM] A1win: Even then, a force quit could happen if the server suddenly loses network connection or power or whatever [12:11 PM] Luke: Yeah the shutdown code of the steam transport looks sketchy. That won't work on application exit/alt-f4. Someone would need to adjust that. [12:14 PM] A1win: Anything I can do to make sure this isn't forgotten (raise an issue or something) or will you take care of that? [12:15 PM] Luke: Yeah please open a bug in the contrib repository. Not sure though whether we'd have the resources to look into that.

JamesMcGhee commented 2 years ago

I made a few adjustments to the SteamNetworkingTransport that will check for connected user before handling the data and will notify the developer log if such happens

I also added an OnDestroy to try and close any open connections however this wouldn't be fool proof ... assuming that Steam API is shutdown before this is called it wont mean anything.

Beyond that the main push for #179 is the addition of the SteamNetworkingSocketsTransport ... it poles for network events by connection so this wouldn't be an issue in the first place.