From 08019ac94eb8cd57abad4d03c803371290cfe210 Mon Sep 17 00:00:00 2001
From: Jeremy Koritzinsky <jkoritzinsky@gmail.com>
Date: Thu, 3 Jan 2019 21:35:53 -0800
Subject: [PATCH] Add test for StringBuilder null terminator implementation
 detail (#21800)

* Remove some commented out code.

* Add test verifying that we put a null terminator 2-past the end of the native buffer allocated for a StringBuilder.
---
 src/vm/ilmarshalers.cpp                             |  14 +++-----------
 .../Interop/StringMarshalling/LPTSTR/LPTSTRTest.cs  | Bin 17934 -> 18964 bytes
 .../StringMarshalling/LPTSTR/LPTSTRTestNative.cpp   |   8 ++++++++
 .../LPTSTR/LPTSTRTestPInvokeDef.cs                  |   7 ++++++-
 4 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/src/vm/ilmarshalers.cpp b/src/vm/ilmarshalers.cpp
index 8b6b15e..d3869c2 100644
--- a/src/vm/ilmarshalers.cpp
+++ b/src/vm/ilmarshalers.cpp
@@ -772,17 +772,9 @@ void ILWSTRBufferMarshaler::EmitConvertContentsCLRToNative(ILCodeStream* pslILEm
     pslILEmit->EmitCALL(METHOD__STRING_BUILDER__GET_LENGTH, 1, 1);
 
     // stack: StringBuilder length
-
-    // if (!fConvertSpaceJustCalled)
-    {
-        // we don't need to double-check the length because the length
-        // must be smaller than the capacity and the capacity was already
-        // checked by EmitConvertSpaceCLRToNative
-        
-        pslILEmit->EmitDUP();
-        // static void StubHelpers.CheckStringLength(int length)
-        pslILEmit->EmitCALL(METHOD__STUBHELPERS__CHECK_STRING_LENGTH, 1, 0);
-    }
+    pslILEmit->EmitDUP();
+    // static void StubHelpers.CheckStringLength(int length)
+    pslILEmit->EmitCALL(METHOD__STUBHELPERS__CHECK_STRING_LENGTH, 1, 0);
 
     // stack: StringBuilder length 
 
diff --git a/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTest.cs b/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTest.cs
index cc7369f155102d3b1e3ab1506bd8614b2ebd0b88..f7684a3463eca8a24e95f4914689f259877aabb8 100644
GIT binary patch
delta 610
zcmeC{VVp9Bal;i?*-VB!h7tw^h8%`eAS)fn&H(akf!L72VDdsuWA!|SQlM-OLkLhs
z5koEmNJk=2T|SWK1eD7J%B3)*PQK5t%Z+5pWJeLn$qtfSll7d$>chZ#Ga1qtDjDJ#
z{P37q43rN5nqLgG!4+sz3WElaB+3Oq_voPcM3X_2ftP`6a;KTEI?PrDpvjm%DF*6H
z1Nmukp|HGh9*_kJm13Yu5Dk_A($Nf_K-a`VWpaV;1chEELo!40<U(=j$qmkIlOL!_
xQS9e<27jQRODOUC<VLelQv7U++s_cU;PbWA<T^IK$yc~VCVz94U;&0W0{}DLg9rcs

delta 17
ZcmbO-g|V-Pal;kY$#raelb?9%002Ud2Oj_c

diff --git a/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestNative.cpp b/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestNative.cpp
index df9a6d3..833c316 100644
--- a/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestNative.cpp
+++ b/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestNative.cpp
@@ -153,3 +153,11 @@ extern "C" DLL_EXPORT BOOL STDMETHODCALLTYPE ReverseP_MarshalStrB_Out(Test_Del_M
     return TRUE;
 
 }
+
+// Verify that we append extra null terminators to our StringBuilder native buffers.
+// Although this is a hidden implementation detail, it would be breaking behavior to stop doing this
+// so we have a test for it. In particular, this detail prevents us from optimizing marshalling StringBuilders by pinning.
+extern "C" DLL_EXPORT BOOL STDMETHODCALLTYPE Verify_NullTerminators_PastEnd(LPCWSTR buffer, int length)
+{
+    return buffer[length+1] == W('\0');
+}
diff --git a/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestPInvokeDef.cs b/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestPInvokeDef.cs
index f54397b..d8648df 100644
--- a/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestPInvokeDef.cs
+++ b/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestPInvokeDef.cs
@@ -63,5 +63,10 @@ namespace NativeDefs
         [DllImport(NativeBinaryName)]
         public static extern bool ReverseP_MarshalStrB_InOut(Del_MarshalStrB_InOut d, [MarshalAs(UnmanagedType.LPTStr)]string s);
 
+        [DllImport(NativeBinaryName)]
+        public static extern bool Verify_NullTerminators_PastEnd(StringBuilder builder, int length);
+
+        [DllImport(NativeBinaryName, EntryPoint = "Verify_NullTerminators_PastEnd")]
+        public static extern bool Verify_NullTerminators_PastEnd_Out([Out] StringBuilder builder, int length);
     }
-}
\ No newline at end of file
+}
-- 
2.7.4