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

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)."

src/dlls/mscorrc/resource.h

@@ -721,5 +721,4 @@
 
 #define IDS_EE_NDIRECT_GETPROCADDR_WIN_DLL 0x2644
 #define IDS_EE_NDIRECT_GETPROCADDR_UNIX_SO 0x2645
-
-
+#define IDS_EE_BADMARSHAL_STRING_OUT 0x2646

src/vm/ilmarshalers.cpp

@@ -252,25 +252,56 @@ 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");
 } 
 
 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* 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);
 } 
 
 bool ILWSTRMarshaler::NeedsClearNative()
@@ -448,27 +479,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;

src/vm/ilmarshalers.h

@@ -1947,7 +1947,7 @@ class ILWSTRMarshaler : public ILMarshaler
 public:
  enum
  {
- c_fInOnly = TRUE,
+ c_fInOnly = FALSE,
  c_nativeSize = sizeof(void *),
  c_CLRSize = sizeof(OBJECTREF),
  };
@@ -1962,6 +1962,18 @@ class ILWSTRMarshaler : public ILMarshaler
  }
 #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();
@@ -1973,7 +1985,6 @@ class ILWSTRMarshaler : public ILMarshaler
 
  virtual void EmitConvertSpaceNativeToCLR(ILCodeStream* pslILEmit);
  virtual void EmitConvertContentsNativeToCLR(ILCodeStream* pslILEmit);
- virtual void EmitConvertSpaceAndContentsNativeToCLR(ILCodeStream* pslILEmit);
 
  virtual bool NeedsClearNative();
  virtual void EmitClearNative(ILCodeStream* pslILEmit);

tests/src/Interop/COM/NETClients/Primitives/StringTests.cs

@@ -191,8 +191,7 @@ private void Marshal_LPWString()
  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)

tests/src/Interop/COM/NETServer/StringTesting.cs

@@ -126,6 +126,9 @@ public void Reverse_LPWStr_Out([MarshalAs(UnmanagedType.LPWStr)] string a, [Mars
  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 void Reverse_BStr_OutAttr([MarshalAs(UnmanagedType.BStr)] string a, [Out]
  {
  b = Reverse(a);
  }
-}
+}

tests/src/Interop/StringMarshalling/LPSTR/LPSTRTest.cs

@@ -61,7 +61,6 @@ public static string Call_DelMarshal_InOut(string s)
  strRet = "\0\0\0";
  return strRet;
  }
- s = "Managed";
  strRet = "Return\0Return\0";
  return strRet;
  }
@@ -193,6 +192,7 @@ public static int Main(string[] args)
 
  #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");

tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestNative.cpp

@@ -33,6 +33,20 @@ extern "C" LPWSTR ReturnErrString()
 }
 
 //Test Method1
+extern "C" DLL_EXPORT LPWSTR Marshal_In(/*[In]*/LPWSTR s)
+{
+ //Check the Input
+ size_t len = wcslen(s);
+
+ if((len != lenstrManaged)||(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)

tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestPInvokeDef.cs

@@ -31,6 +31,10 @@ public static class PInvokeDef
  [return: MarshalAs(UnmanagedType.LPTStr)]
  public static extern string Marshal_Out([Out][MarshalAs(UnmanagedType.LPTStr)]string s);
 
+ [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);
@@ -64,4 +68,4 @@ public static class PInvokeDef
  public static extern bool ReverseP_MarshalStrB_InOut(Del_MarshalStrB_InOut d, [MarshalAs(UnmanagedType.LPTStr)]string s);
 
  }
-}
+}