diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index 3f6df83f11..0c5e47d1a0 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/com.unity.netcode.gameobjects/CHANGELOG.md @@ -21,6 +21,8 @@ Additional documentation and release notes are available at [Multiplayer Documen ### Fixed + +- Issue when FastBufferReader is attempting to read a string and all or a portion of the character count has already been read by user script, it could read a character length that results in a negative byte length which could result in an editor crash. (#4052) - Issue where NetworkRigidbodyBase was not applying rotation correctly when using Rigidbody2D. (#4012) - Issue where NetworkRigidbodyBase was always checking the 3D rigid body's interpolation mode when determining if it is kinematic and needs to put the rigid body to sleep and then switch to interpolation. (#4012) diff --git a/com.unity.netcode.gameobjects/Runtime/Serialization/FastBufferReader.cs b/com.unity.netcode.gameobjects/Runtime/Serialization/FastBufferReader.cs index d420ccd28e..d132f775fd 100644 --- a/com.unity.netcode.gameobjects/Runtime/Serialization/FastBufferReader.cs +++ b/com.unity.netcode.gameobjects/Runtime/Serialization/FastBufferReader.cs @@ -564,14 +564,36 @@ public void ReadNetworkSerializableInPlace(ref T value) where T : INetworkSer } /// - /// Reads a string - /// NOTE: ALLOCATES + /// Validates the string's total byte count based on whether we are + /// using one or two byte characters. + /// + /// + /// Will throw an overflow exception if the size is greater than . + /// + /// Character count + /// If false(default) 2 byte characters and if true 1 byte characters + /// total size in bytes to read + /// + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private int ValidateStringByteCount(int length, bool oneByteChars) + { + var readSize = oneByteChars ? length : length * sizeof(char); + if (int.MaxValue < (uint)readSize) + { + throw new OverflowException($"Invalid reader position detected when trying to read a string of size {(uint)readSize}! This can result from an error in the serialization. Ensure deserialization exactly matches what was serialized!"); + } + return readSize; + } + + /// + /// Commonly shared string read method between . /// - /// Stores the read string - /// Whether or not to use one byte per character. This will only allow ASCII - public unsafe void ReadValue(out string s, bool oneByteChars = false) + /// The output of the string read. + /// The number of characters in the string. + /// If false(default) 2 byte characters and if true 1 byte characters. + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private unsafe void ReadString(out string s, int length, bool oneByteChars) { - ReadLength(out int length); s = "".PadRight(length); int target = s.Length; fixed (char* native = s) @@ -592,56 +614,50 @@ public unsafe void ReadValue(out string s, bool oneByteChars = false) } /// - /// Reads a string. - /// NOTE: ALLOCATES - /// - /// "Safe" version - automatically performs bounds checking. Less efficient than bounds checking - /// for multiple reads at once by calling TryBeginRead. + /// Reads a string without bounds checking. + /// NOTE: This method ALLOCATES memory. /// + /// + /// This is the un-safe string read which requires invoking prior to invoking this method.
+ /// Using one byte characters only allows ASCII characters. + ///
/// Stores the read string - /// Whether or not to use one byte per character. This will only allow ASCII - public unsafe void ReadValueSafe(out string s, bool oneByteChars = false) + /// If false(default) 2 byte characters and if true 1 byte characters. + public unsafe void ReadValue(out string s, bool oneByteChars = false) { -#if DEBUG - if (Handle->InBitwiseContext) - { - throw new InvalidOperationException( - "Cannot use BufferReader in bytewise mode while in a bitwise context."); - } -#endif + ReadLength(out int length); - if (!TryBeginReadInternal(SizeOfLengthField())) - { - throw new OverflowException("Reading past the end of the buffer"); - } + // Validate the string's byte count based on the character count. + ValidateStringByteCount(length, oneByteChars); - ReadLength(out int length); + // Read the string + ReadString(out s, length, oneByteChars); + } + + /// + /// Reads a string after it performs bounds checking automatically. + /// NOTE: This method ALLOCATES memory. + /// + /// + /// This is the safe string read which invokes prior to reading the string.
+ /// Using one byte characters only allows ASCII characters. + ///
+ /// The string re the read string + /// If false(default) 2 byte characters and if true 1 byte characters. + public unsafe void ReadValueSafe(out string s, bool oneByteChars = false) + { + ReadLengthSafe(out int length); - if (!TryBeginReadInternal(length * (oneByteChars ? 1 : sizeof(char)))) + // Validate the string's byte count based on the character count and if it is valid begin reading based on the returned + // byte count. + if (!TryBeginReadInternal(ValidateStringByteCount(length, oneByteChars))) { throw new OverflowException("Reading past the end of the buffer"); } - s = "".PadRight(length); - int target = s.Length; - fixed (char* native = s) - { - if (oneByteChars) - { - for (int i = 0; i < target; ++i) - { - ReadByte(out byte b); - native[i] = (char)b; - } - } - else - { - ReadBytes((byte*)native, target * sizeof(char)); - } - } - } - [MethodImpl(MethodImplOptions.AggressiveInlining)] - private static int SizeOfLengthField() => sizeof(uint); + // Read the string + ReadString(out s, length, oneByteChars); + } [MethodImpl(MethodImplOptions.AggressiveInlining)] private void ReadLengthSafe(out uint length) => ReadUnmanagedSafe(out length); diff --git a/com.unity.netcode.gameobjects/Tests/Editor/Serialization/FastBufferReaderTests.cs b/com.unity.netcode.gameobjects/Tests/Editor/Serialization/FastBufferReaderTests.cs index 85aabbb52e..d38c10e0a7 100644 --- a/com.unity.netcode.gameobjects/Tests/Editor/Serialization/FastBufferReaderTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Editor/Serialization/FastBufferReaderTests.cs @@ -924,6 +924,50 @@ public void GivenFastBufferWriterContainingValue_WhenReadingString_ValueMatchesW } } + /// + /// This validates that catches a potential + /// scenario where the string's character count value has already been read + /// due to an error within user script and the resultant character count + /// multiplied times 2 (when using 2 bytes vs 1) causes the length to roll over + /// to a negative value which, in turn, causes the reader to attempt to read + /// into restricted memory and causes the editor to crash. + /// + [Test] + public void ReadingStringAfterStringLengthHasAlreadyBeenRead([Values] bool isSafeRead) + { + // This was an issue uncovered in UUM-145752 that resulted + // in the below text to result in a length that when using + // 2 bytes per character would cause the skewed size to roll + // over into a negative value causing the editor to crash + // when it attempted to read a large negative offset value. + string valueToTest = "true"; + + var serializedValueSize = FastBufferWriter.GetWriteSize(valueToTest); + + using var writer = new FastBufferWriter(serializedValueSize, Allocator.Temp); + writer.WriteValueSafe(valueToTest); + + using var reader = new FastBufferReader(writer, Allocator.Temp); + + // Read the value of the character count before trying to read the string + // This mocks user code having read too far into a stream causing the position to be skewed such that + // the string reader reads the some of the bytes for the actual text as the length. + reader.ReadByteSafe(out byte count); + Assert.True(count == valueToTest.Length, $"Count ({count}) is not the expected size of {valueToTest.Length}!"); + if (isSafeRead) + { + // This should throw an overflow exception but should not crash the editor. + Assert.Throws(() => reader.ReadValueSafe(out string valueRead)); + } + else + { + // Assume user does a pre-calculation of the size to be read: + Assert.IsTrue(reader.TryBeginRead(count), "Reader denied read permission"); + + // This should throw an overflow exception but should not crash the editor. + Assert.Throws(() => reader.ReadValue(out string valueRead)); + } + } [TestCase(1, 0)] [TestCase(2, 0)]