From 9bb17e67bcc84f6205ee066c5d03147ab2e5d303 Mon Sep 17 00:00:00 2001 From: mischa <16416509+miwarnec@users.noreply.github.com> Date: Tue, 23 Jul 2024 17:15:26 +0200 Subject: [PATCH] NetworkReader/Writer: all types of collections are now encoded via count-offsetting (null=0, []=1) for consistency and to prepare for VarUInt compression (this way we won't need zig zag VarInt) (#3869) * better comments about count offsetting * NetworkReader/Writer: all types of collections are now encoded via count-offsetting (null=0, []=1) for consistency and to prepare for VarUInt compression (this way we won't need zig zag VarInt) --------- Co-authored-by: mischa --- Assets/Mirror/Core/NetworkReaderExtensions.cs | 42 +++++++++------ Assets/Mirror/Core/NetworkWriterExtensions.cs | 51 ++++++++++++------- .../NetworkReaderWriter/NetworkWriterTest.cs | 3 +- 3 files changed, 61 insertions(+), 35 deletions(-) diff --git a/Assets/Mirror/Core/NetworkReaderExtensions.cs b/Assets/Mirror/Core/NetworkReaderExtensions.cs index 4493be3db..5655319c0 100644 --- a/Assets/Mirror/Core/NetworkReaderExtensions.cs +++ b/Assets/Mirror/Core/NetworkReaderExtensions.cs @@ -97,8 +97,9 @@ public static byte[] ReadBytes(this NetworkReader reader, int count) /// if count is invalid public static byte[] ReadBytesAndSize(this NetworkReader reader) { - // count = 0 means the array was null - // otherwise count -1 is the length of the array + // we offset count by '1' to easily support null without writing another byte. + // encoding null as '0' instead of '-1' also allows for better compression + // (ushort vs. short / varuint vs. varint) etc. uint count = reader.ReadUInt(); // Use checked() to force it to throw OverflowException if data is invalid return count == 0 ? null : reader.ReadBytes(checked((int)(count - 1u))); @@ -107,8 +108,9 @@ public static byte[] ReadBytesAndSize(this NetworkReader reader) /// if count is invalid public static ArraySegment ReadArraySegmentAndSize(this NetworkReader reader) { - // count = 0 means the array was null - // otherwise count - 1 is the length of the array + // we offset count by '1' to easily support null without writing another byte. + // encoding null as '0' instead of '-1' also allows for better compression + // (ushort vs. short / varuint vs. varint) etc. uint count = reader.ReadUInt(); // Use checked() to force it to throw OverflowException if data is invalid return count == 0 ? default : reader.ReadBytesSegment(checked((int)(count - 1u))); @@ -264,10 +266,12 @@ public static GameObject ReadGameObject(this NetworkReader reader) // note that Weaver/Readers/GenerateReader() handles this manually. public static List ReadList(this NetworkReader reader) { - int length = reader.ReadInt(); - - // 'null' is encoded as '-1' - if (length < 0) return null; + // we offset count by '1' to easily support null without writing another byte. + // encoding null as '0' instead of '-1' also allows for better compression + // (ushort vs. short / varuint vs. varint) etc. + uint length = reader.ReadUInt(); + if (length == 0) return null; + length -= 1; // prevent allocation attacks with a reasonable limit. // server shouldn't allocate too much on client devices. @@ -278,7 +282,7 @@ public static List ReadList(this NetworkReader reader) throw new EndOfStreamException($"NetworkReader attempted to allocate a List<{typeof(T)}> {length} elements, which is larger than the allowed limit of {NetworkReader.AllocationLimit}."); } - List result = new List(length); + List result = new List((checked((int)length))); for (int i = 0; i < length; i++) { result.Add(reader.Read()); @@ -294,9 +298,13 @@ public static List ReadList(this NetworkReader reader) /* public static HashSet ReadHashSet(this NetworkReader reader) { - int length = reader.ReadInt(); - if (length < 0) - return null; + // we offset count by '1' to easily support null without writing another byte. + // encoding null as '0' instead of '-1' also allows for better compression + // (ushort vs. short / varuint vs. varint) etc. + uint length = reader.ReadUInt(); + if (length == 0) return null; + length -= 1; + HashSet result = new HashSet(); for (int i = 0; i < length; i++) { @@ -308,10 +316,12 @@ public static HashSet ReadHashSet(this NetworkReader reader) public static T[] ReadArray(this NetworkReader reader) { - int length = reader.ReadInt(); - - // 'null' is encoded as '-1' - if (length < 0) return null; + // we offset count by '1' to easily support null without writing another byte. + // encoding null as '0' instead of '-1' also allows for better compression + // (ushort vs. short / varuint vs. varint) etc. + uint length = reader.ReadUInt(); + if (length == 0) return null; + length -= 1; // prevent allocation attacks with a reasonable limit. // server shouldn't allocate too much on client devices. diff --git a/Assets/Mirror/Core/NetworkWriterExtensions.cs b/Assets/Mirror/Core/NetworkWriterExtensions.cs index fef9f613a..9d937f6e7 100644 --- a/Assets/Mirror/Core/NetworkWriterExtensions.cs +++ b/Assets/Mirror/Core/NetworkWriterExtensions.cs @@ -51,10 +51,9 @@ public static class NetworkWriterExtensions public static void WriteString(this NetworkWriter writer, string value) { - // write 0 for null support, increment real size by 1 - // (note: original HLAPI would write "" for null strings, but if a - // string is null on the server then it should also be null - // on the client) + // we offset count by '1' to easily support null without writing another byte. + // encoding null as '0' instead of '-1' also allows for better compression + // (ushort vs. short / varuint vs. varint) etc. if (value == null) { writer.WriteUShort(0); @@ -94,9 +93,10 @@ public static void WriteBytesAndSize(this NetworkWriter writer, byte[] buffer) // (like an inventory with different items etc.) public static void WriteBytesAndSize(this NetworkWriter writer, byte[] buffer, int offset, int count) { - // null is supported because [SyncVar]s might be structs with null byte[] arrays - // write 0 for null array, increment normal size by 1 to save bandwidth - // (using size=-1 for null would limit max size to 32kb instead of 64kb) + // null is supported because [SyncVar]s might be structs with null byte[] arrays. + // we offset count by '1' to easily support null without writing another byte. + // encoding null as '0' instead of '-1' also allows for better compression + // (ushort vs. short / varuint vs. varint) etc. if (buffer == null) { writer.WriteUInt(0u); @@ -115,9 +115,17 @@ public static void WriteArraySegmentAndSize(this NetworkWriter writer, ArraySegm // writes ArraySegment of any type, and size header public static void WriteArraySegment(this NetworkWriter writer, ArraySegment segment) { - int length = segment.Count; - writer.WriteInt(length); - for (int i = 0; i < length; i++) + // we offset count by '1' to easily support null without writing another byte. + // encoding null as '0' instead of '-1' also allows for better compression + // (ushort vs. short / varuint vs. varint) etc. + // + // ArraySegment technically can't be null, but users may call: + // - WriteArraySegment + // - ReadArray + // in which case ReadArray needs null support. both need to be compatible. + int count = segment.Count; + writer.WriteUInt(checked((uint)count) + 1u); + for (int i = 0; i < count; i++) { writer.Write(segment.Array[segment.Offset + i]); } @@ -315,10 +323,12 @@ public static void WriteGameObject(this NetworkWriter writer, GameObject value) // note that Weaver/Writers/GenerateWriter() handles this manually. public static void WriteList(this NetworkWriter writer, List list) { - // 'null' is encoded as '-1' + // we offset count by '1' to easily support null without writing another byte. + // encoding null as '0' instead of '-1' also allows for better compression + // (ushort vs. short / varuint vs. varint) etc. if (list is null) { - writer.WriteInt(-1); + writer.WriteUInt(0); return; } @@ -326,7 +336,7 @@ public static void WriteList(this NetworkWriter writer, List list) if (list.Count > NetworkReader.AllocationLimit) throw new IndexOutOfRangeException($"NetworkWriter.WriteList - List<{typeof(T)}> too big: {list.Count} elements. Limit: {NetworkReader.AllocationLimit}"); - writer.WriteInt(list.Count); + writer.WriteUInt(checked((uint)list.Count) + 1u); for (int i = 0; i < list.Count; i++) writer.Write(list[i]); } @@ -339,12 +349,15 @@ public static void WriteList(this NetworkWriter writer, List list) /* public static void WriteHashSet(this NetworkWriter writer, HashSet hashSet) { + // we offset count by '1' to easily support null without writing another byte. + // encoding null as '0' instead of '-1' also allows for better compression + // (ushort vs. short / varuint vs. varint) etc. if (hashSet is null) { - writer.WriteInt(-1); + writer.WriteUInt(0); return; } - writer.WriteInt(hashSet.Count); + writer.WriteUInt(checked((uint)hashSet.Count) + 1u); foreach (T item in hashSet) writer.Write(item); } @@ -352,10 +365,12 @@ public static void WriteHashSet(this NetworkWriter writer, HashSet hashSet public static void WriteArray(this NetworkWriter writer, T[] array) { - // 'null' is encoded as '-1' + // we offset count by '1' to easily support null without writing another byte. + // encoding null as '0' instead of '-1' also allows for better compression + // (ushort vs. short / varuint vs. varint) etc. if (array is null) { - writer.WriteInt(-1); + writer.WriteUInt(0); return; } @@ -363,7 +378,7 @@ public static void WriteArray(this NetworkWriter writer, T[] array) if (array.Length > NetworkReader.AllocationLimit) throw new IndexOutOfRangeException($"NetworkWriter.WriteArray - Array<{typeof(T)}> too big: {array.Length} elements. Limit: {NetworkReader.AllocationLimit}"); - writer.WriteInt(array.Length); + writer.WriteUInt(checked((uint)array.Length) + 1u); for (int i = 0; i < array.Length; i++) writer.Write(array[i]); } diff --git a/Assets/Mirror/Tests/Editor/NetworkReaderWriter/NetworkWriterTest.cs b/Assets/Mirror/Tests/Editor/NetworkReaderWriter/NetworkWriterTest.cs index 55d9286bf..a1f5cbd38 100644 --- a/Assets/Mirror/Tests/Editor/NetworkReaderWriter/NetworkWriterTest.cs +++ b/Assets/Mirror/Tests/Editor/NetworkReaderWriter/NetworkWriterTest.cs @@ -1404,7 +1404,8 @@ public void TestArrayThrowsIfLengthIsWrong(int badLength) void WriteBadArray() { - writer.WriteInt(badLength); + // Reader/Writer encode null as count=0 and [] as count=1 (+1 offset) + writer.WriteUInt((uint)(badLength+1)); int[] array = new int[testArraySize] { 1, 2, 3, 4 }; for (int i = 0; i < array.Length; i++) writer.Write(array[i]);