lidgren / lidgren-network-gen3

Lidgren Network Library
https://groups.google.com/forum/#!forum/lidgren-network-gen3
MIT License
1.19k stars 332 forks source link

BUG GetNetworkInterface has non-deterministic behaviour #115

Open aznarepse opened 5 years ago

aznarepse commented 5 years ago

This method returns the first NIC with a valid IP without actually checking if it is the right one. For instance, if the machine is running some virtualization or has multiple NICs.

In my case, in Windows, it just returns a HyperV NIC used by docker and does not return the real NIC.

It is a bit naif, the 'best' adapter as per the code is just the first one in the list having a valid IP... Because of this problem, DiscoverLocalPeers doesn't work either.

jswigart commented 2 years ago

Did you find a solution to this?

aznarepse commented 2 years ago

If I remember correctly, I believe we did not find a solution within this library and wrote our own.

jswigart commented 2 years ago

Wrote your own GetNetworkInterface replacement or networking library? If it's the former, can you share what additional filtering you might have used to more specifically identify the most appropriate adapter?

aznarepse commented 2 years ago

That was a long ago and am not part of that project any longer. Unfortunately, I cannot post the exact code. The 'best' way to get the Network interface that the machine is using to connect to the gateway is to actually open a socket and connect to a well known address, for example:

IPEndPoint localEP = null;
using (Socket socket = new Socket(AddressFamily.InterNetwork, SocketType.Dgram, 0))
{
        socket.Connect("8.8.8.8", 65530);
        localEP = socket.LocalEndPoint as IPEndPoint;
}
return localEP;

Then you can obtain the info from the IPEndPoint object and you can find what NetworkInterface is using the IP address found with the above, for example:

foreach (NetworkInterface adapter in NetworkInterface.GetAllNetworkInterfaces())
            {
                if (adapter.NetworkInterfaceType == NetworkInterfaceType.Loopback || adapter.NetworkInterfaceType == NetworkInterfaceType.Unknown)
                    continue;
                if (!adapter.Supports(NetworkInterfaceComponent.IPv4))
                    continue;
                if (adapter.OperationalStatus != OperationalStatus.Up)
                    continue;
                IPInterfaceProperties properties = adapter.GetIPProperties();
                foreach (UnicastIPAddressInformation unicastAddress in properties.UnicastAddresses)
                {
                    if (unicastAddress != null && unicastAddress.Address != null && unicastAddress.Address.AddressFamily == AddressFamily.InterNetwork
                        && unicastAddress.Address.Equals(localIP)) //where localIP is obtained form the above code
                    {
                        // Yes it does, return this network interface.
                        return adapter;
                    }
                }
            }
jswigart commented 2 years ago

For future people with this problem here is how I ended up implementing this

private static NetworkInterface _cachedNetworkInterface = null;

        public static NetworkInterface CacheValidNetworkInterface()
        {
            string[] tryEndPoints = new string[]
            {
                "8.8.8.8:65530",
                "8.8.4.4:65530"
            };

            IPEndPoint localEP = null;
            foreach (var ep in tryEndPoints)
            {
                try
                {
                    IPEndPoint endpoint;
                    if (IPEndPoint.TryParse(ep, out endpoint))
                    {
                        using (Socket socket = new Socket(AddressFamily.InterNetwork, SocketType.Dgram, 0))
                        {
                            socket.Connect(endpoint);
                            localEP = socket.LocalEndPoint as IPEndPoint;
                        }
                    }
                }
                catch (Exception)
                {
                }

                if (localEP != null)
                    break;
            }

            if (localEP != null)
            {
                var adapters = NetworkInterface.GetAllNetworkInterfaces();

                foreach (NetworkInterface adapter in adapters)
                {
                    try
                    {
                        if (adapter.NetworkInterfaceType == NetworkInterfaceType.Loopback || adapter.NetworkInterfaceType == NetworkInterfaceType.Unknown)
                            continue;
                        if (!adapter.Supports(NetworkInterfaceComponent.IPv4))
                            continue;
                        if (adapter.OperationalStatus != OperationalStatus.Up)
                            continue;
                        IPInterfaceProperties properties = adapter.GetIPProperties();
                        foreach (UnicastIPAddressInformation unicastAddress in properties.UnicastAddresses)
                        {
                            if (unicastAddress?.Address == null)
                                continue;
                            if (unicastAddress.Address.AddressFamily != AddressFamily.InterNetwork)
                                continue;

                            if (unicastAddress.Address.Equals(localEP.Address)) //where localIP is obtained form the above code
                            {
                                // Yes it does, return this network interface.
                                _cachedNetworkInterface = adapter;
                                break;
                            }
                        }
                    }
                    catch (Exception)
                    {
                    }
                }
            }

            return _cachedNetworkInterface;
        }

And then in the GetNetworkInterface function I added an early out check.

private static NetworkInterface GetNetworkInterface()
        {
            if (_cachedNetworkInterface != null)
                return _cachedNetworkInterface;

In this way, I'm not being destructive to the lidgren code, and it would operate normally until I call my new function in my server code

 var cachedNetworkInterface = Lidgren.Network.NetUtility.CacheValidNetworkInterface();
            if (cachedNetworkInterface != null)
            {
                _log.Info($"Network using Interface {cachedNetworkInterface.Name} : {cachedNetworkInterface.Description}");
            }
            else
            {
                _log.Warn($"CacheValidNetworkInterface failed to determine a valid network interface. It's possible an invalid one could be chosen.");
            }

The problem with stock lidgren code is that it can end up choosing the wrong adapter as your network interface because by default it doesn't actually check whether or not the particular adapter can talk to the internet. This issue will manifest in a game being unable to talk to steam servers for example, because the ip you tell steam to bind to may be from an adapter that can't talk to steam, such as a virtual adapter from development tools or virtualbox/VPN type adapters that may satisfy all the other criteria.

In this case, I'm just confirming the chosen adapter with the additional criteria that it is able to connect to a google DNS ip. It only needs to do this once when you call the cache function, and that adapter will be returned in subsequent calls to GetNetworkInterface, which is used in a variety of places.

PJB3005 commented 2 years ago

I was looking through the issue list and would like to point out I actually fixed this with the same approach in our fork