lidgren / lidgren-network-gen3

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

Large unreliable message cannot be sent properly #148

Open aienabled opened 4 years ago

aienabled commented 4 years ago

Hello!

While the project is no longer actively maintained it's important to mention this issue.

If you attempt to send a large unreliable message, by default Lidgren library attempts to send it as a single unfragmented message even if it's above MTU. However, the data format for the network message header allows a payload only of up to 65535 bits length (16 bits limit; that's less than 8192 bytes) as you can see here and in the code that write the size into the buffer https://github.com/lidgren/lidgren-network-gen3/blob/d0931dcf9f4c2959967e77d9badbc4afe642c02b/Lidgren.Network/NetOutgoingMessage.cs#L65

There are no checks to ensure that the max packet size is not exceeded. The int m_bitLength number is simply encoded into 16 bits.

This way any unreliable messages of larger size will be received with an incorrect size—even though the whole message is written and might be received on the client side, it's not properly parsed as the payload size cannot be properly read. And Lidgren doesn't actually check whether the received payload matches the size, the only check I see is this one: https://github.com/lidgren/lidgren-network-gen3/blob/3ab2d8d36867ccd9e10d7b1c61ed4ccc8ce5172f/Lidgren.Network/NetPeer.Internal.cs#L524 E. g. if you're sending a payload of 29914 bytes the library sees only 5332 bytes due to the (unchecked) 16-bit encoding of the payload length.

There is no proper support for fragmenting unreliable messages. While you can enable fragmentation for that case there is a critical drawback: https://github.com/lidgren/lidgren-network-gen3/blob/3ab2d8d36867ccd9e10d7b1c61ed4ccc8ce5172f/Lidgren.Network/NetPeerConfiguration.cs#L550

Bottom line: don't send large unreliable packages. Make sure you split your data—or use reliable mechanisms (as you should whenever you need to actually send something large). Also, I think at least a max packet size check is required in the library otherwise it's really easy to miss this issue and spend hours investigating it (that's what I did).

Regards!

aienabled commented 4 years ago

A simple check that seems to work very well and also notifies the library user about the problem:

if (message.LengthBits >= ushort.MaxValue
    && m_connection.m_peerConfiguration.UnreliableSizeBehaviour == NetUnreliableSizeBehaviour.IgnoreMTU)
{
    // drop message
    this.m_connection.m_peer.LogError(
        string.Format("Unreliable message max size exceeded: {0} bits (max {1})",
                      message.LengthBits, 
                      ushort.MaxValue));
    return NetSendResult.Dropped;
}

it should be inserted in NetUnreliableSenderChannel right before this line: https://github.com/lidgren/lidgren-network-gen3/blob/d3458db59c082e8541f07cd738523996753e2f7c/Lidgren.Network/NetUnreliableSenderChannel.cs#L62 This way only the unreliable messages exceeding the max allowed message size are prohibited (either way they cannot be received properly so it's best to drop them and notify the library user). Please let me know if a pull request would be useful.

PJB3005 commented 4 years ago

Oh so that explains why my game state system got overloaded even over localhost.

As for pull requests: the library is unmaintained now, see #147 for maintained forks.