From fd4bc0fe84bbba9b9987164e62c50d6d32dd16b1 Mon Sep 17 00:00:00 2001
From: Serge Camille <sergecamille@gmail.com>
Date: Wed, 19 Aug 2020 21:27:42 +0200
Subject: [PATCH] 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<ArgumentNullException>(() => 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<ArgumentNullException>(() => 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<ArgumentException>(() => 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
         /// <returns>A <see cref="T:byte[]" /> containing the string header and string value with a maximum length of <paramref name="reservedLength"/> + 2.</returns>
         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;
         }
     }
 }