From 7c07da2f4b14ef3bf8f7dac73dce667f7f2ba77c Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Sat, 6 Jun 2020 18:27:15 -0600 Subject: [PATCH] Fix MessagePackStreamReader reading when string or binary headers are incomplete When reading headers we would throw if the header was incomplete where we should have returned false. Fixes #924 --- .../Scripts/MessagePack/MessagePackReader.cs | 117 +++++++++++++----- .../MessagePackStreamReaderTests.cs | 6 +- 2 files changed, 92 insertions(+), 31 deletions(-) diff --git a/src/MessagePack.UnityClient/Assets/Scripts/MessagePack/MessagePackReader.cs b/src/MessagePack.UnityClient/Assets/Scripts/MessagePack/MessagePackReader.cs index ce94e841..17565038 100644 --- a/src/MessagePack.UnityClient/Assets/Scripts/MessagePack/MessagePackReader.cs +++ b/src/MessagePack.UnityClient/Assets/Scripts/MessagePack/MessagePackReader.cs @@ -149,6 +149,7 @@ namespace MessagePack /// /// The entire primitive is skipped, including content of maps or arrays, or any other type with payloads. /// To get the raw MessagePack sequence that was skipped, use instead. + /// WARNING: when false is returned, the position of the reader is undefined. /// internal bool TrySkip() { @@ -187,11 +188,11 @@ namespace MessagePack case MessagePackCode.Str8: case MessagePackCode.Str16: case MessagePackCode.Str32: - return this.reader.TryAdvance(this.GetStringLengthInBytes()); + return this.TryGetStringLengthInBytes(out int length) && this.reader.TryAdvance(length); case MessagePackCode.Bin8: case MessagePackCode.Bin16: case MessagePackCode.Bin32: - return this.reader.TryAdvance(this.GetBytesLength()); + return this.TryGetBytesLength(out length) && this.reader.TryAdvance(length); case MessagePackCode.FixExt1: case MessagePackCode.FixExt2: case MessagePackCode.FixExt4: @@ -220,7 +221,7 @@ namespace MessagePack if (code >= MessagePackCode.MinFixStr && code <= MessagePackCode.MaxFixStr) { - return this.reader.TryAdvance(this.GetStringLengthInBytes()); + return this.TryGetStringLengthInBytes(out length) && this.reader.TryAdvance(length); } // We don't actually expect to ever hit this point, since every code is supported. @@ -956,77 +957,135 @@ namespace MessagePack private int GetBytesLength() { - ThrowInsufficientBufferUnless(this.reader.TryRead(out byte code)); + ThrowInsufficientBufferUnless(this.TryGetBytesLength(out int length)); + return length; + } + + private bool TryGetBytesLength(out int length) + { + if (!this.reader.TryRead(out byte code)) + { + length = 0; + return false; + } // In OldSpec mode, Bin didn't exist, so Str was used. Str8 didn't exist either. - int length; switch (code) { case MessagePackCode.Bin8: - ThrowInsufficientBufferUnless(this.reader.TryRead(out byte byteLength)); - length = byteLength; + if (this.reader.TryRead(out byte byteLength)) + { + length = byteLength; + return true; + } + break; case MessagePackCode.Bin16: case MessagePackCode.Str16: // OldSpec compatibility - ThrowInsufficientBufferUnless(this.reader.TryReadBigEndian(out short shortLength)); - length = unchecked((ushort)shortLength); + if (this.reader.TryReadBigEndian(out short shortLength)) + { + length = unchecked((ushort)shortLength); + return true; + } + break; case MessagePackCode.Bin32: case MessagePackCode.Str32: // OldSpec compatibility - ThrowInsufficientBufferUnless(this.reader.TryReadBigEndian(out length)); + if (this.reader.TryReadBigEndian(out length)) + { + return true; + } + break; default: // OldSpec compatibility if (code >= MessagePackCode.MinFixStr && code <= MessagePackCode.MaxFixStr) { length = code & 0x1F; - break; + return true; } throw ThrowInvalidCode(code); } - return length; + length = 0; + return false; + } + + /// + /// Gets the length of the next string. + /// + /// Receives the length of the next string, if there were enough bytes to read it. + /// true if there were enough bytes to read the length of the next string; false otherwise. + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private bool TryGetStringLengthInBytes(out int length) + { + if (!this.reader.TryRead(out byte code)) + { + length = 0; + return false; + } + + if (code >= MessagePackCode.MinFixStr && code <= MessagePackCode.MaxFixStr) + { + length = code & 0x1F; + return true; + } + + return this.TryGetStringLengthInBytesSlow(code, out length); } /// /// Gets the length of the next string. /// /// The length of the next string. - [MethodImpl(MethodImplOptions.AggressiveInlining)] private int GetStringLengthInBytes() { - ThrowInsufficientBufferUnless(this.reader.TryRead(out byte code)); - - if (code >= MessagePackCode.MinFixStr && code <= MessagePackCode.MaxFixStr) - { - return code & 0x1F; - } - - return this.GetStringLengthInBytesSlow(code); + ThrowInsufficientBufferUnless(this.TryGetStringLengthInBytes(out int length)); + return length; } - private int GetStringLengthInBytesSlow(byte code) + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private bool TryGetStringLengthInBytesSlow(byte code, out int length) { switch (code) { case MessagePackCode.Str8: - ThrowInsufficientBufferUnless(this.reader.TryRead(out byte byteValue)); - return byteValue; + if (this.reader.TryRead(out byte byteValue)) + { + length = byteValue; + return true; + } + + break; case MessagePackCode.Str16: - ThrowInsufficientBufferUnless(this.reader.TryReadBigEndian(out short shortValue)); - return unchecked((ushort)shortValue); + if (this.reader.TryReadBigEndian(out short shortValue)) + { + length = unchecked((ushort)shortValue); + return true; + } + + break; case MessagePackCode.Str32: - ThrowInsufficientBufferUnless(this.reader.TryReadBigEndian(out int intValue)); - return intValue; + if (this.reader.TryReadBigEndian(out int intValue)) + { + length = intValue; + return true; + } + + break; default: if (code >= MessagePackCode.MinFixStr && code <= MessagePackCode.MaxFixStr) { - return code & 0x1F; + length = code & 0x1F; + return true; } throw ThrowInvalidCode(code); } + + length = 0; + return false; } /// diff --git a/src/MessagePack.UnityClient/Assets/Scripts/Tests/ShareTests/MessagePackStreamReaderTests.cs b/src/MessagePack.UnityClient/Assets/Scripts/Tests/ShareTests/MessagePackStreamReaderTests.cs index c6dd466a..f32c4a17 100644 --- a/src/MessagePack.UnityClient/Assets/Scripts/Tests/ShareTests/MessagePackStreamReaderTests.cs +++ b/src/MessagePack.UnityClient/Assets/Scripts/Tests/ShareTests/MessagePackStreamReaderTests.cs @@ -31,9 +31,11 @@ namespace MessagePack.Tests positions.Add(sequence.AsReadOnlySequence.End); // Second message is more interesting. - writer.WriteArrayHeader(2); + writer.WriteArrayHeader(4); writer.Write("Hi"); - writer.Write("There"); + writer.Write("There + " + new string('3', 300)); // a long enough string that a multi-byte header is required. + writer.Write(new byte[300]); // a long enough byte array that a multi-byte header is required. + writer.WriteExtensionFormat(new ExtensionResult(1, new byte[300])); writer.Flush(); positions.Add(sequence.AsReadOnlySequence.End);