From dba0f645adde02044ef468ed79701d7e59c49b20 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 16 Jun 2026 14:57:51 +0000 Subject: [PATCH 1/4] fix: handle malformed batches in Unbatcher to prevent queue growth attack - Add Unbatcher.Clear() to drain queued batches and return writers to pool - Make GetNextMessage() throw when message size prefix exceeds remaining bytes, clearing all batches before throwing - Wrap batch processing loops in NetworkServer and NetworkClient with try-catch to handle malformed batch exceptions and disconnect - Disconnect (not just log) when batches remain after processing loop - Add tests for malformed batch detection and Clear() --- Assets/Mirror/Core/Batching/Unbatcher.cs | 22 ++++- Assets/Mirror/Core/NetworkClient.cs | 83 ++++++++++++------- Assets/Mirror/Core/NetworkServer.cs | 83 ++++++++++++------- .../Tests/Editor/Batching/UnbatcherTests.cs | 77 +++++++++++++++++ 4 files changed, 199 insertions(+), 66 deletions(-) diff --git a/Assets/Mirror/Core/Batching/Unbatcher.cs b/Assets/Mirror/Core/Batching/Unbatcher.cs index 6b2c405ad40..5af608c0ebf 100644 --- a/Assets/Mirror/Core/Batching/Unbatcher.cs +++ b/Assets/Mirror/Core/Batching/Unbatcher.cs @@ -18,6 +18,19 @@ public class Unbatcher public int BatchesCount => batches.Count; + // clear all batches and return writers to pool. + // called when a malformed batch is detected or connection disconnects. + public void Clear() + { + while (batches.Count > 0) + { + NetworkWriterPooled writer = batches.Dequeue(); + NetworkWriterPool.Return(writer); + } + // reset reader so it doesn't point to returned batch + reader.SetBuffer(new byte[0]); + } + // NetworkReader is only created once, // then pointed to the first batch. readonly NetworkReader reader = new NetworkReader(new byte[0]); @@ -117,9 +130,14 @@ public bool GetNextMessage(out ArraySegment message, out double remoteTime // see Batcher.AddMessage comments for explanation. int size = (int)Compression.DecompressVarUInt(reader); - // validate size prefix, in case attackers send malicious data + // validate size prefix, in case attackers send malicious data. + // a size larger than remaining bytes means the batch is malformed. + // clear all batches and throw so the caller can disconnect. if (reader.Remaining < size) - return false; + { + Clear(); + throw new InvalidOperationException($"GetNextMessage: malformed batch with message size {size} > remaining {reader.Remaining}. All batches cleared."); + } // return the message of size message = reader.ReadBytesSegment(size); diff --git a/Assets/Mirror/Core/NetworkClient.cs b/Assets/Mirror/Core/NetworkClient.cs index fd2bf465123..feba8f931aa 100644 --- a/Assets/Mirror/Core/NetworkClient.cs +++ b/Assets/Mirror/Core/NetworkClient.cs @@ -354,53 +354,68 @@ internal static void OnTransportData(ArraySegment data, int channelId) // would only be processed when OnTransportData is called // the next time. // => consider moving processing to NetworkEarlyUpdate. - while (!isLoadingScene && - unbatcher.GetNextMessage(out ArraySegment message, out double remoteTimestamp)) + // + // GetNextMessage may throw for malformed batches + // (invalid varint, message size > remaining bytes). + // catch and disconnect to prevent queue growth attacks. + try { - using (NetworkReaderPooled reader = NetworkReaderPool.Get(message)) + while (!isLoadingScene && + unbatcher.GetNextMessage(out ArraySegment message, out double remoteTimestamp)) { - // enough to read at least header size? - if (reader.Remaining >= NetworkMessages.IdSize) + using (NetworkReaderPooled reader = NetworkReaderPool.Get(message)) { - // make remoteTimeStamp available to the user - connection.remoteTimeStamp = remoteTimestamp; + // enough to read at least header size? + if (reader.Remaining >= NetworkMessages.IdSize) + { + // make remoteTimeStamp available to the user + connection.remoteTimeStamp = remoteTimestamp; - // handle message - if (!UnpackAndInvoke(reader, channelId)) + // handle message + if (!UnpackAndInvoke(reader, channelId)) + { + // warn, disconnect and return if failed + // -> warning because attackers might send random data + // -> messages in a batch aren't length prefixed. + // failing to read one would cause undefined + // behaviour for every message afterwards. + // so we need to disconnect. + // -> return to avoid the below unbatches.count error. + // we already disconnected and handled it. + if (exceptionsDisconnect) + { + Debug.LogError($"NetworkClient: failed to unpack and invoke message. Disconnecting."); + connection.Disconnect(); + } + else + Debug.LogWarning($"NetworkClient: failed to unpack and invoke message."); + + return; + } + } + // otherwise disconnect + else { - // warn, disconnect and return if failed - // -> warning because attackers might send random data - // -> messages in a batch aren't length prefixed. - // failing to read one would cause undefined - // behaviour for every message afterwards. - // so we need to disconnect. - // -> return to avoid the below unbatches.count error. - // we already disconnected and handled it. if (exceptionsDisconnect) { - Debug.LogError($"NetworkClient: failed to unpack and invoke message. Disconnecting."); + Debug.LogError($"NetworkClient: received Message was too short (messages should start with message id). Disconnecting."); connection.Disconnect(); } else - Debug.LogWarning($"NetworkClient: failed to unpack and invoke message."); - + Debug.LogWarning("NetworkClient: received Message was too short (messages should start with message id)"); return; } } - // otherwise disconnect - else - { - if (exceptionsDisconnect) - { - Debug.LogError($"NetworkClient: received Message was too short (messages should start with message id). Disconnecting."); - connection.Disconnect(); - } - else - Debug.LogWarning("NetworkClient: received Message was too short (messages should start with message id)"); - return; - } } } + catch (Exception e) + { + // malformed batch: invalid varint, message size > remaining, etc. + // unbatcher already cleared batches when it detected the error. + Debug.LogError($"NetworkClient: failed to parse batch: {e.Message}. Disconnecting."); + connection.Disconnect(); + return; + } // if we weren't interrupted by a scene change, // then all batched messages should have been processed now. @@ -421,6 +436,10 @@ internal static void OnTransportData(ArraySegment data, int channelId) if (!isLoadingScene && unbatcher.BatchesCount > 0) { Debug.LogError($"Still had {unbatcher.BatchesCount} batches remaining after processing, even though processing was not interrupted by a scene change. This should never happen, as it would cause ever growing batches.\nPossible reasons:\n* A message didn't deserialize as much as it serialized\n*There was no message handler for a message id, so the reader wasn't read until the end."); + + // disconnect and clear to prevent memory leak / queue growth + unbatcher.Clear(); + connection.Disconnect(); } } else Debug.LogError("Skipped Data message handling because connection is null."); diff --git a/Assets/Mirror/Core/NetworkServer.cs b/Assets/Mirror/Core/NetworkServer.cs index 0ba7cfcb070..14237418349 100644 --- a/Assets/Mirror/Core/NetworkServer.cs +++ b/Assets/Mirror/Core/NetworkServer.cs @@ -951,54 +951,69 @@ internal static void OnTransportData(int connectionId, ArraySegment data, // would only be processed when OnTransportData is called // the next time. // => consider moving processing to NetworkEarlyUpdate. - while (!isLoadingScene && - connection.unbatcher.GetNextMessage(out ArraySegment message, out double remoteTimestamp)) + // + // GetNextMessage may throw for malformed batches + // (invalid varint, message size > remaining bytes). + // catch and disconnect to prevent queue growth attacks. + try { - using (NetworkReaderPooled reader = NetworkReaderPool.Get(message)) + while (!isLoadingScene && + connection.unbatcher.GetNextMessage(out ArraySegment message, out double remoteTimestamp)) { - // enough to read at least header size? - if (reader.Remaining >= NetworkMessages.IdSize) + using (NetworkReaderPooled reader = NetworkReaderPool.Get(message)) { - // make remoteTimeStamp available to the user - connection.remoteTimeStamp = remoteTimestamp; + // enough to read at least header size? + if (reader.Remaining >= NetworkMessages.IdSize) + { + // make remoteTimeStamp available to the user + connection.remoteTimeStamp = remoteTimestamp; - // handle message - if (!UnpackAndInvoke(connection, reader, channelId)) + // handle message + if (!UnpackAndInvoke(connection, reader, channelId)) + { + // warn, disconnect and return if failed + // -> warning because attackers might send random data + // -> messages in a batch aren't length prefixed. + // failing to read one would cause undefined + // behaviour for every message afterwards. + // so we need to disconnect. + // -> return to avoid the below unbatches.count error. + // we already disconnected and handled it. + if (exceptionsDisconnect) + { + Debug.LogError($"NetworkServer: failed to unpack and invoke message. Disconnecting {connectionId}."); + connection.Disconnect(); + } + else + Debug.LogWarning($"NetworkServer: failed to unpack and invoke message from connectionId:{connectionId}."); + + return; + } + } + // otherwise disconnect + else { - // warn, disconnect and return if failed - // -> warning because attackers might send random data - // -> messages in a batch aren't length prefixed. - // failing to read one would cause undefined - // behaviour for every message afterwards. - // so we need to disconnect. - // -> return to avoid the below unbatches.count error. - // we already disconnected and handled it. if (exceptionsDisconnect) { - Debug.LogError($"NetworkServer: failed to unpack and invoke message. Disconnecting {connectionId}."); + Debug.LogError($"NetworkServer: received message from connectionId:{connectionId} was too short (messages should start with message id). Disconnecting."); connection.Disconnect(); } else - Debug.LogWarning($"NetworkServer: failed to unpack and invoke message from connectionId:{connectionId}."); + Debug.LogWarning($"NetworkServer: received message from connectionId:{connectionId} was too short (messages should start with message id)."); return; } } - // otherwise disconnect - else - { - if (exceptionsDisconnect) - { - Debug.LogError($"NetworkServer: received message from connectionId:{connectionId} was too short (messages should start with message id). Disconnecting."); - connection.Disconnect(); - } - else - Debug.LogWarning($"NetworkServer: received message from connectionId:{connectionId} was too short (messages should start with message id)."); - - return; - } } } + catch (Exception e) + { + // malformed batch: invalid varint, message size > remaining, etc. + // unbatcher already cleared batches when it detected the error. + Debug.LogError($"NetworkServer: failed to parse batch from connectionId:{connectionId}: {e.Message}. Disconnecting."); + connection.Disconnect(); + return; + } // if we weren't interrupted by a scene change, // then all batched messages should have been processed now. @@ -1018,6 +1033,10 @@ internal static void OnTransportData(int connectionId, ArraySegment data, if (!isLoadingScene && connection.unbatcher.BatchesCount > 0) { Debug.LogError($"Still had {connection.unbatcher.BatchesCount} batches remaining after processing, even though processing was not interrupted by a scene change. This should never happen, as it would cause ever growing batches.\nPossible reasons:\n* A message didn't deserialize as much as it serialized\n*There was no message handler for a message id, so the reader wasn't read until the end."); + + // disconnect and clear to prevent memory leak / queue growth + connection.unbatcher.Clear(); + connection.Disconnect(); } } else Debug.LogError($"HandleData Unknown connectionId:{connectionId}"); diff --git a/Assets/Mirror/Tests/Editor/Batching/UnbatcherTests.cs b/Assets/Mirror/Tests/Editor/Batching/UnbatcherTests.cs index 0a440f2115e..f61c62dee84 100644 --- a/Assets/Mirror/Tests/Editor/Batching/UnbatcherTests.cs +++ b/Assets/Mirror/Tests/Editor/Batching/UnbatcherTests.cs @@ -133,5 +133,82 @@ public void RetireBatchAndTryNewBatch() Assert.That(reader.ReadByte(), Is.EqualTo(0x04)); Assert.That(remoteTimeStamp, Is.EqualTo(TimeStamp + 1)); } + + // malformed batch: message size prefix larger than remaining bytes. + // GetNextMessage should throw and clear all batches. + [Test] + public void GetNextMessage_MalformedBatch_SizeLargerThanRemaining() + { + // craft a batch with timestamp + varint size prefix claiming 1,000,000 bytes + // but only a few bytes actually remaining + NetworkWriter writer = new NetworkWriter(); + writer.WriteDouble(TimeStamp); + Compression.CompressVarUInt(writer, 1000000); // claim 1M bytes + writer.WriteByte(0xFF); // but only 1 byte of data + byte[] batch = writer.ToArray(); + + unbatcher.AddBatch(new ArraySegment(batch)); + Assert.That(unbatcher.BatchesCount, Is.EqualTo(1)); + + // GetNextMessage should throw InvalidOperationException + Assert.Throws(() => + { + unbatcher.GetNextMessage(out _, out _); + }); + + // all batches should be cleared + Assert.That(unbatcher.BatchesCount, Is.EqualTo(0)); + } + + // malformed batch should not allow queue growth when repeated + [Test] + public void GetNextMessage_MalformedBatch_NoQueueGrowth() + { + // add multiple malformed batches + for (int i = 0; i < 10; i++) + { + NetworkWriter writer = new NetworkWriter(); + writer.WriteDouble(TimeStamp); + Compression.CompressVarUInt(writer, 1000000); + writer.WriteByte(0xFF); + byte[] batch = writer.ToArray(); + + unbatcher.AddBatch(new ArraySegment(batch)); + + // each GetNextMessage should throw and clear + Assert.Throws(() => + { + unbatcher.GetNextMessage(out _, out _); + }); + + // batches should be cleared after each malformed batch + Assert.That(unbatcher.BatchesCount, Is.EqualTo(0)); + } + } + + [Test] + public void Clear() + { + // add batches + byte[] batch1 = BatcherTests.MakeBatch(TimeStamp, new byte[] {0x01, 0x02}); + byte[] batch2 = BatcherTests.MakeBatch(TimeStamp + 1, new byte[] {0x03, 0x04}); + unbatcher.AddBatch(new ArraySegment(batch1)); + unbatcher.AddBatch(new ArraySegment(batch2)); + Assert.That(unbatcher.BatchesCount, Is.EqualTo(2)); + + // clear should remove all batches + unbatcher.Clear(); + Assert.That(unbatcher.BatchesCount, Is.EqualTo(0)); + + // should be able to add and read new batches after clear + byte[] batch3 = BatcherTests.MakeBatch(TimeStamp + 2, new byte[] {0x05, 0x06}); + unbatcher.AddBatch(new ArraySegment(batch3)); + bool result = unbatcher.GetNextMessage(out ArraySegment message, out double remoteTimeStamp); + Assert.That(result, Is.True); + NetworkReader reader = new NetworkReader(message); + Assert.That(reader.ReadByte(), Is.EqualTo(0x05)); + Assert.That(reader.ReadByte(), Is.EqualTo(0x06)); + Assert.That(remoteTimeStamp, Is.EqualTo(TimeStamp + 2)); + } } } From d9c64f7f038563dff771bae2f119912e74ff58b4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 16 Jun 2026 14:59:54 +0000 Subject: [PATCH 2/4] refactor: use Array.Empty() instead of new byte[0] in Unbatcher.Clear --- Assets/Mirror/Core/Batching/Unbatcher.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Assets/Mirror/Core/Batching/Unbatcher.cs b/Assets/Mirror/Core/Batching/Unbatcher.cs index 5af608c0ebf..13edfd97d97 100644 --- a/Assets/Mirror/Core/Batching/Unbatcher.cs +++ b/Assets/Mirror/Core/Batching/Unbatcher.cs @@ -28,7 +28,7 @@ public void Clear() NetworkWriterPool.Return(writer); } // reset reader so it doesn't point to returned batch - reader.SetBuffer(new byte[0]); + reader.SetBuffer(Array.Empty()); } // NetworkReader is only created once, From 13249a1c3b24c25ee08b6b01f0aff20ed259e302 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 17 Jun 2026 11:21:26 +0000 Subject: [PATCH 3/4] fix: use explicit ArraySegment in Unbatcher.Clear for cross-version Unity compatibility --- Assets/Mirror/Core/Batching/Unbatcher.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Assets/Mirror/Core/Batching/Unbatcher.cs b/Assets/Mirror/Core/Batching/Unbatcher.cs index 13edfd97d97..6edffe2d0cd 100644 --- a/Assets/Mirror/Core/Batching/Unbatcher.cs +++ b/Assets/Mirror/Core/Batching/Unbatcher.cs @@ -28,7 +28,7 @@ public void Clear() NetworkWriterPool.Return(writer); } // reset reader so it doesn't point to returned batch - reader.SetBuffer(Array.Empty()); + reader.SetBuffer(new ArraySegment(Array.Empty())); } // NetworkReader is only created once, From 028c43c50c0f81f3b59226985ede1458aabb3462 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 17 Jun 2026 15:03:41 +0000 Subject: [PATCH 4/4] Use #if UNITY_2021_3_OR_NEWER for implicit ArraySegment conversion in Unbatcher.Clear() --- Assets/Mirror/Core/Batching/Unbatcher.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Assets/Mirror/Core/Batching/Unbatcher.cs b/Assets/Mirror/Core/Batching/Unbatcher.cs index 6edffe2d0cd..dc49d54da65 100644 --- a/Assets/Mirror/Core/Batching/Unbatcher.cs +++ b/Assets/Mirror/Core/Batching/Unbatcher.cs @@ -28,7 +28,11 @@ public void Clear() NetworkWriterPool.Return(writer); } // reset reader so it doesn't point to returned batch +#if UNITY_2021_3_OR_NEWER + reader.SetBuffer(Array.Empty()); +#else reader.SetBuffer(new ArraySegment(Array.Empty())); +#endif } // NetworkReader is only created once,