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>

10 files changed, 81 insertions, 36 deletions

diff --git a/src/dlls/mscorrc/mscorrc.rc b/src/dlls/mscorrc/mscorrc.rc
index 355c902233..a0616a2e67 100644
--- a/src/dlls/mscorrc/mscorrc.rc
+++ b/src/dlls/mscorrc/mscorrc.rc
@@ -657,6 +657,7 @@ BEGIN
     IDS_EE_BADMARSHAL_BADMETADATA           "Invalid marshaling metadata."
     IDS_EE_BADMARSHAL_CUSTOMMARSHALER       "Custom marshalers are only allowed on classes, strings, arrays, and boxed value types."
     IDS_EE_BADMARSHAL_GENERICS_RESTRICTION  "Generic types cannot be marshaled."
+    IDS_EE_BADMARSHAL_STRING_OUT            "Cannot marshal a string by-value with the [Out] attribute."
 
     IDS_EE_BADMARSHALPARAM_STRINGBUILDER    "Invalid managed/unmanaged type combination (StringBuilders must be paired with LPStr, LPWStr, or LPTStr)."
     IDS_EE_BADMARSHALPARAM_NO_LPTSTR        "Invalid managed/unmanaged type combination (Strings cannot be paired with LPTStr for parameters and return types of methods in interfaces exposed to COM)."
diff --git a/src/dlls/mscorrc/resource.h b/src/dlls/mscorrc/resource.h
index 500ee74642..6d137f75bd 100644
--- a/src/dlls/mscorrc/resource.h
+++ b/src/dlls/mscorrc/resource.h
@@ -722,5 +722,4 @@
 
 #define IDS_EE_NDIRECT_GETPROCADDR_WIN_DLL         0x2644
 #define IDS_EE_NDIRECT_GETPROCADDR_UNIX_SO         0x2645
-
-
+#define IDS_EE_BADMARSHAL_STRING_OUT               0x2646
diff --git a/src/vm/ilmarshalers.cpp b/src/vm/ilmarshalers.cpp
index 7abfc019f3..1a253eca74 100644
--- a/src/vm/ilmarshalers.cpp
+++ b/src/vm/ilmarshalers.cpp
@@ -252,25 +252,60 @@ LocalDesc ILWSTRMarshaler::GetManagedType()
 void ILWSTRMarshaler::EmitConvertSpaceCLRToNative(ILCodeStream* pslILEmit)
 {
     LIMITED_METHOD_CONTRACT;
-    UNREACHABLE_MSG("Should be in-only and all other paths are covered by the EmitConvertSpaceAndContents* paths");
+    UNREACHABLE_MSG("All paths to this function are covered by the EmitConvertSpaceAndContents* paths");
 }
 
 void ILWSTRMarshaler::EmitConvertContentsCLRToNative(ILCodeStream* pslILEmit)
 {
-    LIMITED_METHOD_CONTRACT;
-    UNREACHABLE_MSG("Should be in-only and all other paths are covered by the EmitConvertSpaceAndContents* paths");
+    STANDARD_VM_CONTRACT;
+
+    // This code path should only be called by an out marshalling. Other codepaths that convert a string to native
+    // should all go through EmitConvertSpaceAndContentsCLRToNative
+    _ASSERTE(IsOut(m_dwMarshalFlags) && !IsCLRToNative(m_dwMarshalFlags) && !IsByref(m_dwMarshalFlags));
+    
+    ILCodeLabel* pNullRefLabel = pslILEmit->NewCodeLabel();
+
+    EmitLoadManagedValue(pslILEmit);
+    pslILEmit->EmitBRFALSE(pNullRefLabel);
+
+    EmitLoadManagedValue(pslILEmit);
+    EmitLoadNativeValue(pslILEmit);
+
+    EmitLoadManagedValue(pslILEmit);
+    EmitCheckManagedStringLength(pslILEmit);
+    
+    // static void System.String.InternalCopy(String src, IntPtr dest,int len)
+    pslILEmit->EmitCALL(METHOD__STRING__INTERNAL_COPY, 3, 0);
+    pslILEmit->EmitLabel(pNullRefLabel);
 }
 
 void ILWSTRMarshaler::EmitConvertSpaceNativeToCLR(ILCodeStream* pslILEmit)
 {
-    LIMITED_METHOD_CONTRACT;
-    UNREACHABLE_MSG("Should be in-only and all other paths are covered by the EmitConvertSpaceAndContents* paths");
+    STANDARD_VM_CONTRACT;
+    // We currently don't marshal strings from the native to the CLR side in a Reverse-PInvoke unless
+    // the parameter is explicitly annotated as an [In] parameter.
+    pslILEmit->EmitLDNULL();
+    EmitStoreManagedValue(pslILEmit);
 }    
 
 void ILWSTRMarshaler::EmitConvertContentsNativeToCLR(ILCodeStream* pslILEmit)
 {
-    LIMITED_METHOD_CONTRACT;
-    UNREACHABLE_MSG("Should be in-only and all other paths are covered by the EmitConvertSpaceAndContents* paths");
+    STANDARD_VM_CONTRACT;
+    
+    ILCodeLabel* pIsNullLabel = pslILEmit->NewCodeLabel();
+        
+    EmitLoadNativeValue(pslILEmit);
+    pslILEmit->EmitBRFALSE(pIsNullLabel);
+
+    EmitLoadNativeValue(pslILEmit);
+    pslILEmit->EmitDUP();
+    EmitCheckNativeStringLength(pslILEmit);
+    pslILEmit->EmitPOP();       // pop num chars
+    
+    pslILEmit->EmitNEWOBJ(METHOD__STRING__CTOR_CHARPTR, 1);
+    EmitStoreManagedValue(pslILEmit);
+    
+    pslILEmit->EmitLabel(pIsNullLabel);
 }    
 
 bool ILWSTRMarshaler::NeedsClearNative()
@@ -447,27 +482,6 @@ void ILWSTRMarshaler::EmitCheckNativeStringLength(ILCodeStream* pslILEmit)
     pslILEmit->EmitCALL(METHOD__STUBHELPERS__CHECK_STRING_LENGTH, 1, 0);
 }
 
-void ILWSTRMarshaler::EmitConvertSpaceAndContentsNativeToCLR(ILCodeStream* pslILEmit)
-{
-    STANDARD_VM_CONTRACT;
-
-    ILCodeLabel* pIsNullLabelByref = pslILEmit->NewCodeLabel();
-        
-    EmitLoadNativeValue(pslILEmit);
-    pslILEmit->EmitBRFALSE(pIsNullLabelByref);
-
-    EmitLoadNativeValue(pslILEmit);
-    pslILEmit->EmitDUP();
-    EmitCheckNativeStringLength(pslILEmit);
-    pslILEmit->EmitPOP();       // pop num chars
-    
-    pslILEmit->EmitNEWOBJ(METHOD__STRING__CTOR_CHARPTR, 1);
-    EmitStoreManagedValue(pslILEmit);
-    
-    pslILEmit->EmitLabel(pIsNullLabelByref);
-}
-
-
 LocalDesc ILOptimizedAllocMarshaler::GetNativeType()
 {
     LIMITED_METHOD_CONTRACT;
diff --git a/src/vm/ilmarshalers.h b/src/vm/ilmarshalers.h
index 1c58c0c2a7..3f2c16b4d5 100644
--- a/src/vm/ilmarshalers.h
+++ b/src/vm/ilmarshalers.h
@@ -1946,7 +1946,7 @@ class ILWSTRMarshaler : public ILMarshaler
 public:
     enum
     {
-        c_fInOnly               = TRUE,
+        c_fInOnly               = FALSE,
         c_nativeSize            = sizeof(void *),
         c_CLRSize               = sizeof(OBJECTREF),
     };
@@ -1961,6 +1961,18 @@ public:
     }
 #endif // _DEBUG
 
+    
+    virtual bool SupportsArgumentMarshal(DWORD dwMarshalFlags, UINT* pErrorResID)
+    {
+        if (IsOut(dwMarshalFlags) && !IsByref(dwMarshalFlags) && IsCLRToNative(dwMarshalFlags))
+        {
+            *pErrorResID = IDS_EE_BADMARSHAL_STRING_OUT;
+            return false;
+        }
+
+        return true;
+    }
+
 protected:
     virtual LocalDesc GetNativeType();
     virtual LocalDesc GetManagedType();
@@ -1972,7 +1984,6 @@ protected:
 
     virtual void EmitConvertSpaceNativeToCLR(ILCodeStream* pslILEmit);
     virtual void EmitConvertContentsNativeToCLR(ILCodeStream* pslILEmit);
-    virtual void EmitConvertSpaceAndContentsNativeToCLR(ILCodeStream* pslILEmit);
 
     virtual bool NeedsClearNative();
     virtual void EmitClearNative(ILCodeStream* pslILEmit);
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)]