From 7f0b2c529012d931d51c957e6981d5ab46799f3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mathias=20Haugsb=C3=B8?= Date: Tue, 17 Dec 2024 12:48:32 +0100 Subject: [PATCH 1/3] #180 Additional serializer infinityLoop when using FieldBitLength Infinity loop occurs when trying to parse a binary message that does not fit into the provided type. This commit adds an additional check when parsing to throw exception if stream.AvailableForReading < 0. This commit breaks following unit tests: - Issue55Tests - ImmutableTest2 - IOExceptionTest - ShouldThrowIOExceptionNotInvalidOperationExceptionTest --- .../Issues/Issue180/Issue180Tests.cs | 51 +++++++++++++++++++ .../Graph/ValueGraph/ValueNode.cs | 5 ++ 2 files changed, 56 insertions(+) diff --git a/BinarySerializer.Test/Issues/Issue180/Issue180Tests.cs b/BinarySerializer.Test/Issues/Issue180/Issue180Tests.cs index 68a751f6..909956be 100644 --- a/BinarySerializer.Test/Issues/Issue180/Issue180Tests.cs +++ b/BinarySerializer.Test/Issues/Issue180/Issue180Tests.cs @@ -71,5 +71,56 @@ public void TestOutOfMemory() var _ = Deserialize(serialized); } } + + public class ObservationWrapper + { + [FieldOrder(1)] + public ObservationData[] Observations; + } + public class ObservationData + { + [FieldOrder(0)] + [FieldCount(1)] + public byte[] OneByte; // 1 + + [FieldOrder(1)] + [FieldBitLength(7)] + public byte SevenBits; // 67 => 0b1000011 + + [FieldOrder(2)] + [FieldBitLength(1)] + public byte OneBit; // 0 => Eight bit of 67 which is 0 + } + + [TestMethod] + [ExpectedException(typeof(InvalidOperationException))] + public void TestInfinityLoop_ShouldNotBeInfinityLoop_ShouldCrash() + { + var msg = new byte[] + { + 1, // 1 Byte which does not fit into the provided type + 67, // OneByteArray + 75, // 7 bits + 1 bit + }; + var ser = new BinarySerializer(); + ser.Deserialize(msg); // Infinite loop + } + + [TestMethod] + public void TestInfinityLoop_DoesNotEndUpInInfinityLoop() + { + var msg = new byte[] + { + 67, // OneByte + 75, // 7 bits + 1 bit + }; + var ser = new BinarySerializer(); + var message = ser.Deserialize(msg); + + Assert.IsNotNull(message); + Assert.AreEqual(67, message[0].OneByte[0]); + Assert.AreEqual(0, message[0].OneBit); + Assert.AreEqual(75, message[0].SevenBits); + } } } diff --git a/BinarySerializer/Graph/ValueGraph/ValueNode.cs b/BinarySerializer/Graph/ValueGraph/ValueNode.cs index 67d71f01..0f1c2cbe 100644 --- a/BinarySerializer/Graph/ValueGraph/ValueNode.cs +++ b/BinarySerializer/Graph/ValueGraph/ValueNode.cs @@ -315,6 +315,11 @@ internal void Deserialize(BoundedStream stream, SerializationOptions options, Ev return; } + if (stream.AvailableForReading < 0) + { + throw new Exception("Read passed end of stream, binary message does not fit the provided type."); + } + AlignLeft(stream); var offset = GetFieldOffset(); From b46b4c6d804a28d7bbd302cd759106df8489aee3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mathias=20Haugsb=C3=B8?= Date: Tue, 17 Dec 2024 13:25:28 +0100 Subject: [PATCH 2/3] Issue55Tests add Assertions to verify output --- .../Issues/Issue55/Issue55Tests.cs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/BinarySerializer.Test/Issues/Issue55/Issue55Tests.cs b/BinarySerializer.Test/Issues/Issue55/Issue55Tests.cs index 53c1bef4..a8ed67d2 100644 --- a/BinarySerializer.Test/Issues/Issue55/Issue55Tests.cs +++ b/BinarySerializer.Test/Issues/Issue55/Issue55Tests.cs @@ -37,16 +37,15 @@ public void Test() outputStream.Seek(0, SeekOrigin.Begin); var inputStream = new LooksLikeANetworkStream(outputStream); //non-seekable stream - //var inputStream = outputStream; //successful var roundtrip = serializer.Deserialize(inputStream); - //Assert.That(roundtrip.Chunk, Is.InstanceOf()); - - //var sourceChunk = (TestChunk)source.Chunk; - //var testChunk = (TestChunk)roundtrip.Chunk; - //Assert.That(testChunk.Customs.Length, Is.EqualTo(sourceChunk.Customs.Length)); - //Assert.That(testChunk.Customs.ElementAt(0).Value, Is.EqualTo(sourceChunk.Customs.ElementAt(0).Value)); - //Assert.That(testChunk.Customs.ElementAt(1).Value, Is.EqualTo(sourceChunk.Customs.ElementAt(1).Value)); + Assert.IsInstanceOfType(roundtrip.Chunk); + var sourceChunk = (TestChunk)source.Chunk; + var testChunk = (TestChunk)roundtrip.Chunk; + Assert.AreEqual(testChunk.Customs.Length, sourceChunk.Customs.Length); + + Assert.AreEqual(testChunk.Customs[0].Value, sourceChunk.Customs[0].Value); + Assert.AreEqual(testChunk.Customs[1].Value, sourceChunk.Customs[1].Value); } } } \ No newline at end of file From 3bf2591715f0e826ea68155f0db7c96aa0e0e2e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mathias=20Haugsb=C3=B8?= Date: Tue, 17 Dec 2024 14:23:48 +0100 Subject: [PATCH 3/3] Throw IndexOutOfRange instead of Exception + add to DeserializeAsync --- .../Issues/Issue124/Issue124Tests.cs | 4 +++- .../Issues/Issue180/Issue180Tests.cs | 17 +++++++++++++++-- BinarySerializer/Graph/ValueGraph/ValueNode.cs | 9 +++++++-- 3 files changed, 25 insertions(+), 5 deletions(-) diff --git a/BinarySerializer.Test/Issues/Issue124/Issue124Tests.cs b/BinarySerializer.Test/Issues/Issue124/Issue124Tests.cs index 852541d3..ae142b1e 100644 --- a/BinarySerializer.Test/Issues/Issue124/Issue124Tests.cs +++ b/BinarySerializer.Test/Issues/Issue124/Issue124Tests.cs @@ -25,7 +25,9 @@ public void Test() }; var actual = Roundtrip(expected); - + Assert.AreEqual(expected.Packets.Count, actual.Packets.Count); + Assert.IsInstanceOfType(actual.Packets[0].PacketBody, typeof(Packet1)); + Assert.IsInstanceOfType(actual.Packets[1].PacketBody, typeof(Packet1)); } } } diff --git a/BinarySerializer.Test/Issues/Issue180/Issue180Tests.cs b/BinarySerializer.Test/Issues/Issue180/Issue180Tests.cs index 909956be..d577f15e 100644 --- a/BinarySerializer.Test/Issues/Issue180/Issue180Tests.cs +++ b/BinarySerializer.Test/Issues/Issue180/Issue180Tests.cs @@ -1,4 +1,5 @@ using System; +using System.Threading; using Microsoft.VisualStudio.TestTools.UnitTesting; namespace BinarySerialization.Test.Issues.Issue180 @@ -93,7 +94,6 @@ public class ObservationData } [TestMethod] - [ExpectedException(typeof(InvalidOperationException))] public void TestInfinityLoop_ShouldNotBeInfinityLoop_ShouldCrash() { var msg = new byte[] @@ -103,7 +103,20 @@ public void TestInfinityLoop_ShouldNotBeInfinityLoop_ShouldCrash() 75, // 7 bits + 1 bit }; var ser = new BinarySerializer(); - ser.Deserialize(msg); // Infinite loop + Assert.ThrowsException(() => ser.Deserialize(msg)); + } + + [TestMethod] + public void TestInfinityLoop_ShouldNotBeInfinityLoop_ShouldCrashOnAsync() + { + var msg = new byte[] + { + 1, // 1 Byte which does not fit into the provided type + 67, // OneByteArray + 75, // 7 bits + 1 bit + }; + var ser = new BinarySerializer(); + Assert.ThrowsExceptionAsync(() => ser.DeserializeAsync(msg)); } [TestMethod] diff --git a/BinarySerializer/Graph/ValueGraph/ValueNode.cs b/BinarySerializer/Graph/ValueGraph/ValueNode.cs index 0f1c2cbe..5fd94b2d 100644 --- a/BinarySerializer/Graph/ValueGraph/ValueNode.cs +++ b/BinarySerializer/Graph/ValueGraph/ValueNode.cs @@ -315,9 +315,9 @@ internal void Deserialize(BoundedStream stream, SerializationOptions options, Ev return; } - if (stream.AvailableForReading < 0) + if (stream.AvailableForReading < -1) // Checking less than -1 as some cases returns -1 on unreadable stream see `ShouldThrowIOExceptionNotInvalidOperationExceptionTest` { - throw new Exception("Read passed end of stream, binary message does not fit the provided type."); + throw new IndexOutOfRangeException("Read passed end of stream, binary message does not fit the provided type."); } AlignLeft(stream); @@ -371,6 +371,11 @@ internal async Task DeserializeAsync(BoundedStream stream, SerializationOptions return; } + if (stream.AvailableForReading < -1) // Checking less than -1 as some cases returns -1 on unreadable stream see `ShouldThrowIOExceptionNotInvalidOperationExceptionTest` + { + throw new IndexOutOfRangeException("Read passed end of stream, binary message does not fit the provided type."); + } + AlignLeft(stream); var offset = GetFieldOffset();