From fd4bc0fe84bbba9b9987164e62c50d6d32dd16b1 Mon Sep 17 00:00:00 2001 From: Serge Camille Date: Wed, 19 Aug 2020 21:27:42 +0200 Subject: [PATCH 1/3] Adjust StringEx ToByteArray. - Do not allow null string to be passed, raise ArgumentNullException. - Do not allow string whose ASCII representation is longer than the reserved length, since this currently leads to silent data loss. - Always write the full binary data length of 2 + reservedLength, since that is what the binary representation of that string is in S7 memory, even if some tail bytes are unused by the current string. I also suspect that S7WriteMultiple would have chocked on that last bit, but I am not sure. There aren't any tests for writing multiple Dataitems right now. Adjust tests accordingly. Mostly add some tail bytes where necessary, and assert on exceptions where this is now required. --- S7.Net.UnitTest/TypeTests/StringExTests.cs | 26 +++++++++------------- S7.Net/Types/StringEx.cs | 21 +++++++++-------- 2 files changed, 23 insertions(+), 24 deletions(-) diff --git a/S7.Net.UnitTest/TypeTests/StringExTests.cs b/S7.Net.UnitTest/TypeTests/StringExTests.cs index 164ba4f..471712a 100644 --- a/S7.Net.UnitTest/TypeTests/StringExTests.cs +++ b/S7.Net.UnitTest/TypeTests/StringExTests.cs @@ -1,5 +1,6 @@ using Microsoft.VisualStudio.TestTools.UnitTesting; using S7.Net.Types; +using System; namespace S7.Net.UnitTest.TypeTests { @@ -39,7 +40,7 @@ namespace S7.Net.UnitTest.TypeTests [TestMethod] public void WriteNullWithReservedLengthZero() { - AssertToByteArrayEquals(null, 0, 0, 0); + Assert.ThrowsException(() => AssertToByteArrayEquals(null, 0, 0, 0)); } [TestMethod] @@ -51,19 +52,19 @@ namespace S7.Net.UnitTest.TypeTests [TestMethod] public void WriteAWithReservedLengthZero() { - AssertToByteArrayEquals("A", 0, 0, 0); + AssertToByteArrayEquals("", 0, 0, 0); } [TestMethod] public void WriteNullWithReservedLengthOne() { - AssertToByteArrayEquals(null, 1, 1, 0); + Assert.ThrowsException(() => AssertToByteArrayEquals(null, 1, 1, 0)); } [TestMethod] public void WriteEmptyStringWithReservedLengthOne() { - AssertToByteArrayEquals("", 1, 1, 0); + AssertToByteArrayEquals("", 1, 1, 0, 0); } [TestMethod] @@ -75,19 +76,13 @@ namespace S7.Net.UnitTest.TypeTests [TestMethod] public void WriteAWithReservedLengthTwo() { - AssertToByteArrayEquals("A", 2, 2, 1, (byte) 'A'); + AssertToByteArrayEquals("A", 2, 2, 1, (byte) 'A', 0); } [TestMethod] - public void WriteAbcWithReservedLengthOne() + public void WriteAbcWithStringLargetThanReservedLength() { - AssertToByteArrayEquals("Abc", 1, 1, 1, (byte) 'A'); - } - - [TestMethod] - public void WriteAbcWithReservedLengthTwo() - { - AssertToByteArrayEquals("Abc", 2, 2, 2, (byte) 'A', (byte) 'b'); + Assert.ThrowsException(() => StringEx.ToByteArray("Abc", 2)); } [TestMethod] @@ -99,7 +94,7 @@ namespace S7.Net.UnitTest.TypeTests [TestMethod] public void WriteAbcWithReservedLengthFour() { - AssertToByteArrayEquals("Abc", 4, 4, 3, (byte) 'A', (byte) 'b', (byte) 'c'); + AssertToByteArrayEquals("Abc", 4, 4, 3, (byte) 'A', (byte) 'b', (byte) 'c', 0); } private static void AssertFromByteArrayEquals(string expected, params byte[] bytes) @@ -109,7 +104,8 @@ namespace S7.Net.UnitTest.TypeTests private static void AssertToByteArrayEquals(string value, int reservedLength, params byte[] expected) { - CollectionAssert.AreEqual(expected, StringEx.ToByteArray(value, reservedLength)); + var convertedData = StringEx.ToByteArray(value, reservedLength); + CollectionAssert.AreEqual(expected, convertedData); } } } diff --git a/S7.Net/Types/StringEx.cs b/S7.Net/Types/StringEx.cs index 81733ca..60ef2dc 100644 --- a/S7.Net/Types/StringEx.cs +++ b/S7.Net/Types/StringEx.cs @@ -43,18 +43,21 @@ namespace S7.Net.Types /// A containing the string header and string value with a maximum length of + 2. public static byte[] ToByteArray(string value, int reservedLength) { + if (value is null) + { + throw new ArgumentNullException(nameof(value)); + } + if (reservedLength > byte.MaxValue) throw new ArgumentException($"The maximum string length supported is {byte.MaxValue}."); - var length = value?.Length; - if (length > reservedLength) length = reservedLength; + var bytes = Encoding.ASCII.GetBytes(value); + if (bytes.Length > reservedLength) throw new ArgumentException($"The provided string length ({bytes.Length} is larger than the specified reserved length ({reservedLength})."); - var bytes = new byte[(length ?? 0) + 2]; - bytes[0] = (byte) reservedLength; - - if (value == null) return bytes; - - bytes[1] = (byte) Encoding.ASCII.GetBytes(value, 0, length.Value, bytes, 2); - return bytes; + var buffer = new byte[2 + reservedLength]; + Array.Copy(bytes, 0, buffer, 2, bytes.Length); + buffer[0] = (byte)reservedLength; + buffer[1] = (byte)bytes.Length; + return buffer; } } } From 1919b0083a5d6ecd7572dc8210332169840aa742 Mon Sep 17 00:00:00 2001 From: Serge Camille Date: Wed, 19 Aug 2020 21:49:32 +0200 Subject: [PATCH 2/3] StringEx more strict malformed checks when parsing byte array. - min length of 2. - capacity >= length --- S7.Net.UnitTest/TypeTests/StringExTests.cs | 48 ++++++++++++++++------ S7.Net/Types/StringEx.cs | 9 +++- 2 files changed, 44 insertions(+), 13 deletions(-) diff --git a/S7.Net.UnitTest/TypeTests/StringExTests.cs b/S7.Net.UnitTest/TypeTests/StringExTests.cs index 471712a..aecf905 100644 --- a/S7.Net.UnitTest/TypeTests/StringExTests.cs +++ b/S7.Net.UnitTest/TypeTests/StringExTests.cs @@ -1,6 +1,8 @@ using Microsoft.VisualStudio.TestTools.UnitTesting; using S7.Net.Types; using System; +using System.Collections; +using System.Linq; namespace S7.Net.UnitTest.TypeTests { @@ -25,6 +27,24 @@ namespace S7.Net.UnitTest.TypeTests AssertFromByteArrayEquals("", 1, 0, (byte) 'a'); } + [TestMethod] + public void ReadMalformedStringTooShort() + { + Assert.ThrowsException(() => AssertFromByteArrayEquals("", 1)); + } + + [TestMethod] + public void ReadMalformedStringSizeLargerThanCapacity() + { + Assert.ThrowsException(() => StringEx.FromByteArray(new byte[] { 3, 5, 0, 1, 2 })); + } + + [TestMethod] + public void ReadMalformedStringCapacityTooLarge() + { + Assert.ThrowsException(() => AssertToByteArrayAndBackEquals("", 300, 0)); + } + [TestMethod] public void ReadA() { @@ -34,49 +54,49 @@ namespace S7.Net.UnitTest.TypeTests [TestMethod] public void ReadAbc() { - AssertFromByteArrayEquals("Abc", 1, 3, (byte) 'A', (byte) 'b', (byte) 'c'); + AssertFromByteArrayEquals("Abc", 3, 3, (byte) 'A', (byte) 'b', (byte) 'c'); } [TestMethod] public void WriteNullWithReservedLengthZero() { - Assert.ThrowsException(() => AssertToByteArrayEquals(null, 0, 0, 0)); + Assert.ThrowsException(() => AssertToByteArrayAndBackEquals(null, 0, 0, 0)); } [TestMethod] public void WriteEmptyStringWithReservedLengthZero() { - AssertToByteArrayEquals("", 0, 0, 0); + AssertToByteArrayAndBackEquals("", 0, 0, 0); } [TestMethod] public void WriteAWithReservedLengthZero() { - AssertToByteArrayEquals("", 0, 0, 0); + AssertToByteArrayAndBackEquals("", 0, 0, 0); } [TestMethod] public void WriteNullWithReservedLengthOne() { - Assert.ThrowsException(() => AssertToByteArrayEquals(null, 1, 1, 0)); + Assert.ThrowsException(() => AssertToByteArrayAndBackEquals(null, 1, 1, 0)); } [TestMethod] public void WriteEmptyStringWithReservedLengthOne() { - AssertToByteArrayEquals("", 1, 1, 0, 0); + AssertToByteArrayAndBackEquals("", 1, 1, 0, 0); } [TestMethod] public void WriteAWithReservedLengthOne() { - AssertToByteArrayEquals("A", 1, 1, 1, (byte) 'A'); + AssertToByteArrayAndBackEquals("A", 1, 1, 1, (byte) 'A'); } [TestMethod] public void WriteAWithReservedLengthTwo() { - AssertToByteArrayEquals("A", 2, 2, 1, (byte) 'A', 0); + AssertToByteArrayAndBackEquals("A", 2, 2, 1, (byte) 'A', 0); } [TestMethod] @@ -88,24 +108,28 @@ namespace S7.Net.UnitTest.TypeTests [TestMethod] public void WriteAbcWithReservedLengthThree() { - AssertToByteArrayEquals("Abc", 3, 3, 3, (byte) 'A', (byte) 'b', (byte) 'c'); + AssertToByteArrayAndBackEquals("Abc", 3, 3, 3, (byte) 'A', (byte) 'b', (byte) 'c'); } [TestMethod] public void WriteAbcWithReservedLengthFour() { - AssertToByteArrayEquals("Abc", 4, 4, 3, (byte) 'A', (byte) 'b', (byte) 'c', 0); + AssertToByteArrayAndBackEquals("Abc", 4, 4, 3, (byte) 'A', (byte) 'b', (byte) 'c', 0); } private static void AssertFromByteArrayEquals(string expected, params byte[] bytes) { - Assert.AreEqual(expected, StringEx.FromByteArray(bytes)); + var convertedString = StringEx.FromByteArray(bytes); + Assert.AreEqual(expected, convertedString); } - private static void AssertToByteArrayEquals(string value, int reservedLength, params byte[] expected) + + private static void AssertToByteArrayAndBackEquals(string value, int reservedLength, params byte[] expected) { var convertedData = StringEx.ToByteArray(value, reservedLength); CollectionAssert.AreEqual(expected, convertedData); + var convertedBack = StringEx.FromByteArray(convertedData); + Assert.AreEqual(value, convertedBack); } } } diff --git a/S7.Net/Types/StringEx.cs b/S7.Net/Types/StringEx.cs index 60ef2dc..5f152b2 100644 --- a/S7.Net/Types/StringEx.cs +++ b/S7.Net/Types/StringEx.cs @@ -17,10 +17,17 @@ namespace S7.Net.Types /// public static string FromByteArray(byte[] bytes) { - if (bytes.Length < 2) return ""; + if (bytes.Length < 2) + { + throw new PlcException(ErrorCode.ReadData, "Malformed S7 String / too short"); + } int size = bytes[0]; int length = bytes[1]; + if (length > size) + { + throw new PlcException(ErrorCode.ReadData, "Malformed S7 String / length larger than capacity"); + } try { From cf493d47f0d9917ccd2d3f18e82cde0e275b304b Mon Sep 17 00:00:00 2001 From: Serge Camille Date: Sun, 6 Sep 2020 15:04:20 +0200 Subject: [PATCH 3/3] Apply some of the hints that the compiler is giving. --- S7.Net/PlcAsynchronous.cs | 4 ++-- S7.Net/PlcSynchronous.cs | 5 ++--- S7.Net/Protocol/S7WriteMultiple.cs | 1 - S7.Net/Protocol/Serialization.cs | 4 ++-- 4 files changed, 6 insertions(+), 8 deletions(-) diff --git a/S7.Net/PlcAsynchronous.cs b/S7.Net/PlcAsynchronous.cs index c6d7388..2f481e4 100644 --- a/S7.Net/PlcAsynchronous.cs +++ b/S7.Net/PlcAsynchronous.cs @@ -324,9 +324,9 @@ namespace S7.Net if (bitAdr != -1) { //Must be writing a bit value as bitAdr is specified - if (value is bool) + if (value is bool boolean) { - await WriteBitAsync(dataType, db, startByteAdr, bitAdr, (bool) value); + await WriteBitAsync(dataType, db, startByteAdr, bitAdr, boolean); } else if (value is int intValue) { diff --git a/S7.Net/PlcSynchronous.cs b/S7.Net/PlcSynchronous.cs index bf0a1a3..8cf5f89 100644 --- a/S7.Net/PlcSynchronous.cs +++ b/S7.Net/PlcSynchronous.cs @@ -289,9 +289,9 @@ namespace S7.Net if (bitAdr != -1) { //Must be writing a bit value as bitAdr is specified - if (value is bool) + if (value is bool boolean) { - WriteBit(dataType, db, startByteAdr, bitAdr, (bool) value); + WriteBit(dataType, db, startByteAdr, bitAdr, boolean); } else if (value is int intValue) { @@ -442,7 +442,6 @@ namespace S7.Net private byte[] BuildWriteBitPackage(DataType dataType, int db, int startByteAdr, bool bitValue, int bitAdr) { - var stream = GetStreamIfAvailable(); var value = new[] { bitValue ? (byte)1 : (byte)0 }; int varCount = 1; // first create the header diff --git a/S7.Net/Protocol/S7WriteMultiple.cs b/S7.Net/Protocol/S7WriteMultiple.cs index 2738a34..853e1e5 100644 --- a/S7.Net/Protocol/S7WriteMultiple.cs +++ b/S7.Net/Protocol/S7WriteMultiple.cs @@ -17,7 +17,6 @@ namespace S7.Net.Protocol (ushort) (2 + paramSize)); var paramOffset = Header.Template.Length; - var dataOffset = paramOffset + paramSize; var data = new ByteArray(); var itemCount = 0; diff --git a/S7.Net/Protocol/Serialization.cs b/S7.Net/Protocol/Serialization.cs index 5e4c98e..3ecee55 100644 --- a/S7.Net/Protocol/Serialization.cs +++ b/S7.Net/Protocol/Serialization.cs @@ -75,9 +75,9 @@ namespace S7.Net.Protocol { var start = startByte * 8 + bitNumber; buffer[index + 2] = (byte)start; - start = start >> 8; + start >>= 8; buffer[index + 1] = (byte)start; - start = start >> 8; + start >>= 8; buffer[index] = (byte)start; }