Throw an exception when passing strings by-value as out parameters. (#21513)

* Throw an exception when passing strings by-value as out parameters.

* Fix encoding

* Don't use override in this PR.

* Clean up Marshal_In

Marshal_In was copied back into existence from Marshal_InOut. Clean it up a bit.

* Remove extraneous whitespace.

* Fix failing test.

* Remove out attribute in COM string tests.

* Add back attribute and check for exception thow in COM tests.

* Add block comment to explain the implementation of Reverse_LPWStr_OutAttr in the NETServer.

* Only throw in a CLR->Native marshalling situation.

* Fix asserts from changed code-paths used in ILWSTRMarshaler.

* Add comment and explicitly load in a null value (instead of leaving it uninitialized).

* Apply suggestions from code review

Co-Authored-By: jkoritzinsky <jkoritzinsky@gmail.com>


Co-authored-by: Jan Vorlicek <janvorli@microsoft.com>

6 files changed, 24 insertions, 4 deletions

diff --git a/tests/src/Interop/COM/NETClients/Primitives/StringTests.cs b/tests/src/Interop/COM/NETClients/Primitives/StringTests.cs
index 46cd5d9701..0c1212ceae 100644
--- a/tests/src/Interop/COM/NETClients/Primitives/StringTests.cs
+++ b/tests/src/Interop/COM/NETClients/Primitives/StringTests.cs
@@ -191,8 +191,7 @@ namespace NetClient
                 Assert.AreEqual(expected, actual);
 
                 actual = local;
-                this.server.Reverse_LPWStr_OutAttr(local, actual); // No-op for strings
-                Assert.AreEqual(local, actual);
+                Assert.Throws<MarshalDirectiveException>( () => this.server.Reverse_LPWStr_OutAttr(local, actual));
             }
 
             foreach (var s in reversableStrings)
diff --git a/tests/src/Interop/COM/NETServer/StringTesting.cs b/tests/src/Interop/COM/NETServer/StringTesting.cs
index 3a510f5e9b..c47a155f29 100644
--- a/tests/src/Interop/COM/NETServer/StringTesting.cs
+++ b/tests/src/Interop/COM/NETServer/StringTesting.cs
@@ -126,6 +126,9 @@ public class StringTesting : Server.Contract.IStringTesting
         b = Reverse(a);
     }
 
+    // This behavior is the "desired" behavior for a string passed by-value with an [Out] attribute.
+    // However, block calling a COM or P/Invoke stub with an "[Out] string" parameter since that would allow users to
+    // edit an immutable string value in place. So, in the NetClient.Primitives.StringTests tests, we expect a MarshalDirectiveException.
     public void Reverse_LPWStr_OutAttr([MarshalAs(UnmanagedType.LPWStr)] string a, [Out][MarshalAs(UnmanagedType.LPWStr)] string b)
     {
         b = Reverse(a);
@@ -188,4 +191,4 @@ public class StringTesting : Server.Contract.IStringTesting
     {
         b = Reverse(a);
     }
-}
\ No newline at end of file
+}
diff --git a/tests/src/Interop/StringMarshalling/LPSTR/LPSTRTest.cs b/tests/src/Interop/StringMarshalling/LPSTR/LPSTRTest.cs
index 74a2087eca..2ea8b33555 100644
--- a/tests/src/Interop/StringMarshalling/LPSTR/LPSTRTest.cs
+++ b/tests/src/Interop/StringMarshalling/LPSTR/LPSTRTest.cs
@@ -61,7 +61,6 @@ class Test
             strRet = "\0\0\0";
             return strRet;
         }
-        s = "Managed";
         strRet = "Return\0Return\0";
         return strRet;
     }
@@ -193,6 +192,7 @@ class Test
 
         #region ReversePinvoke
         DelMarshal_InOut d1 = new DelMarshal_InOut(Call_DelMarshal_InOut);
+
         if (!PInvokeDef.RPinvoke_DelMarshal_InOut(d1, "ň"))
         {
             ReportFailure("Method RPinvoke_DelMarshal_InOut[Managed Side],Return value is false");
diff --git a/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTest.cs b/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTest.cs
index f7684a3463..0cc7b26366 100644
--- a/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTest.cs
+++ b/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTest.cs
diff --git a/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestNative.cpp b/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestNative.cpp
index 72df3d5bb3..5ecf88a4bd 100644
--- a/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestNative.cpp
+++ b/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestNative.cpp
@@ -34,6 +34,20 @@ extern "C" LPWSTR ReturnErrString()
 }
 
 //Test Method1
+extern "C" DLL_EXPORT LPWSTR Marshal_In(/*[In]*/LPWSTR s)
+{
+    //Check the Input
+    size_t len = TP_slen(s);
+
+    if((len != lenstrManaged)||(TP_wmemcmp(s,(WCHAR*)strManaged,len)!=0))
+    {
+        printf("Error in Function Marshal_In(Native Client)\n");
+        return ReturnErrString();
+    }
+
+    //Return
+    return ReturnString();
+}
 
 //Test Method2
 extern "C" DLL_EXPORT LPWSTR Marshal_InOut(/*[In,Out]*/LPWSTR s)
diff --git a/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestPInvokeDef.cs b/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestPInvokeDef.cs
index 88c3e74c4a..548c51135d 100644
--- a/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestPInvokeDef.cs
+++ b/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestPInvokeDef.cs
@@ -33,6 +33,10 @@ namespace NativeDefs
 
         [DllImport(NativeBinaryName)]
         [return: MarshalAs(UnmanagedType.LPTStr)]
+        public static extern string Marshal_In([In][MarshalAs(UnmanagedType.LPTStr)]string s);
+
+        [DllImport(NativeBinaryName)]
+        [return: MarshalAs(UnmanagedType.LPTStr)]
         public static extern string Marshal_InOut([In, Out][MarshalAs(UnmanagedType.LPTStr)]string s);
 
         [DllImport(NativeBinaryName)]