From 393a1b6264ecfc0d8570f773382df92bb7121d14 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Wed, 6 Feb 2019 13:05:28 -0800 Subject: [PATCH] Throw an exception when passing strings by-value as out parameters. (dotnet/coreclr#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 Co-authored-by: Jan Vorlicek Commit migrated from https://github.com/dotnet/coreclr/commit/2f88f32ffb6752c89e6751b4747f1b9cd971304d --- src/coreclr/src/dlls/mscorrc/mscorrc.rc | 1 + src/coreclr/src/dlls/mscorrc/resource.h | 3 +- src/coreclr/src/vm/ilmarshalers.cpp | 70 ++++++++++++--------- src/coreclr/src/vm/ilmarshalers.h | 15 ++++- .../COM/NETClients/Primitives/StringTests.cs | 3 +- .../src/Interop/COM/NETServer/StringTesting.cs | 5 +- .../Interop/StringMarshalling/LPSTR/LPSTRTest.cs | 2 +- .../Interop/StringMarshalling/LPTSTR/LPTSTRTest.cs | Bin 18964 -> 9504 bytes .../StringMarshalling/LPTSTR/LPTSTRTestNative.cpp | 14 +++++ .../LPTSTR/LPTSTRTestPInvokeDef.cs | 4 ++ 10 files changed, 81 insertions(+), 36 deletions(-) diff --git a/src/coreclr/src/dlls/mscorrc/mscorrc.rc b/src/coreclr/src/dlls/mscorrc/mscorrc.rc index 355c902..a0616a2 100644 --- a/src/coreclr/src/dlls/mscorrc/mscorrc.rc +++ b/src/coreclr/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/coreclr/src/dlls/mscorrc/resource.h b/src/coreclr/src/dlls/mscorrc/resource.h index 500ee74..6d137f7 100644 --- a/src/coreclr/src/dlls/mscorrc/resource.h +++ b/src/coreclr/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/coreclr/src/vm/ilmarshalers.cpp b/src/coreclr/src/vm/ilmarshalers.cpp index 7abfc01..1a253ec 100644 --- a/src/coreclr/src/vm/ilmarshalers.cpp +++ b/src/coreclr/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/coreclr/src/vm/ilmarshalers.h b/src/coreclr/src/vm/ilmarshalers.h index 1c58c0c..3f2c16b 100644 --- a/src/coreclr/src/vm/ilmarshalers.h +++ b/src/coreclr/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/src/coreclr/tests/src/Interop/COM/NETClients/Primitives/StringTests.cs b/src/coreclr/tests/src/Interop/COM/NETClients/Primitives/StringTests.cs index 46cd5d9..0c1212c 100644 --- a/src/coreclr/tests/src/Interop/COM/NETClients/Primitives/StringTests.cs +++ b/src/coreclr/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( () => this.server.Reverse_LPWStr_OutAttr(local, actual)); } foreach (var s in reversableStrings) diff --git a/src/coreclr/tests/src/Interop/COM/NETServer/StringTesting.cs b/src/coreclr/tests/src/Interop/COM/NETServer/StringTesting.cs index 3a510f5..c47a155 100644 --- a/src/coreclr/tests/src/Interop/COM/NETServer/StringTesting.cs +++ b/src/coreclr/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/src/coreclr/tests/src/Interop/StringMarshalling/LPSTR/LPSTRTest.cs b/src/coreclr/tests/src/Interop/StringMarshalling/LPSTR/LPSTRTest.cs index 74a2087..2ea8b33 100644 --- a/src/coreclr/tests/src/Interop/StringMarshalling/LPSTR/LPSTRTest.cs +++ b/src/coreclr/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/src/coreclr/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTest.cs b/src/coreclr/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTest.cs index f7684a3463eca8a24e95f4914689f259877aabb8..0cc7b263665e9ab6160678681fa2e7cf14147a88 100644 GIT binary patch literal 9504 zcmeHN%Tn7&6kU(MqQ_Z8B#gljUPG|};?z(Dgu+y^fD|R9wyhai^0eAKD#a?lmJgeA zyVX)lmcXy7*@OkMTdmvY-h1w&TV7jZr-8?#g!@d!Ob$3}op-L-r+6Ist_e zjyQ`28^(e&w=Xy!@<=AF#@ZUYs(2{WLK8>~64nbsPU4^9aVm}$Iy<>y={zN;%P|S# zrzgjq^KM5!6+~)mB;vohCz*(2$$ADz5aHuc!E7}Eg^);|NY70;W_e?*kFQF?QCAA~ z5Xb@J!#ijjjl+;7QUp=I(HJMVuP{5s$iD+(VmWzc8!#t#!$J zA@IRor5jiLQJSH1Aid`wc@MH0Ug#zXy8^t%_eO)^?~UMnGI;ZnLtV)}xj{G0Sv*$9-E?tBTZknqYT9@8pTHi+Q4)u|^;HCtp9T?kn(W}6M{cN(} zax5a|ynnB-r+1#GTA4qNVA$2vRm}C|*bQew!#YX?_Z2QJ4*;7t5hX=J19 znV`p<-;;LhD%LZdm6j5fILU@JxT2pUXXG5CZz*+pZkX`gC7L?ceJRH2ZGGzvk9-|Y zrP%t5ha)a@Od&qrE%2x43mKP+-IVH(8b=7-kqBJL8Gj5U^J1Tyi3_at?p--pI*?A; z!>O{k3NyqbTd%WgMf_eIcDqe0YAy4&*Vn6z&*2n>i>4O+Rw4h>;mIlcgAsi-KE$B< zge>GFD=hZBu@Ky^#n3KenD8~>P=HJ>z2r!M3BL%U`}iM?bu_+1)j>W{3AM=IV>b+M zKl1SQ%oWMN4RQDA_9Xf|mL=+o)atY#{bg1t$^&~7l$`?>@rPolI89v*Zr1hHR=OyI zGKTcHUn}V6kLGII@|y}dVFj0oI7^asw>I>Og`jz+DhR7L(e;@dxqXz@Zs7B8rhK(h zy&3udKj`(iK;wW|aU%o>rFJ-cl{HBTW`njAvZ5V@b`-iEci#T|@phHH9lo{Zp&C}3 zIt7LmcC6`XYSBK@)Oyz0VrGgELE^D2x|kV~ER`HfQn8#Miz-u_apne5TK=!UF<0~_ zdH^tz+9q6#=MPxZ*gB8Zd>S$(9Mp81=znzFu#(9z_ejsGh!__Rw27Vz)-gn~SsG6H z^4((1!UEasiZ#shnkpH}igb2OkwE%ioT%i2zOI#~4Wx{3c0lJqFI?faffFOCp>m-4 zELeahRSG7TC@B_`b&-{Vo-@IYY4|#0a96Gfau-8;mHKQ5r=Bhajo^L|any&0^up$9 z`zwtu^8C#91c-u3Ra=6PJ9_%!hGS0cD)q#rK0%77%!S{xPXr*vjSCrbqsApPv(6^Q z5gV2x@=WEq(i<=*CB(-7eL6M&okx$4Xq!-1mnT)qTN^@S*-&}CG`~)4buO<^4R%JF z#`Xvs5IGR>19OoHR56@XqGglZLWZ3fIul(mHZ!x&=yM<`c)X=!uDK=+q=p;!?#1XUEu)~?AL+Y6GY zoIY}L5Phv$Rvv&YW!OW*fjlKEkkl4x&@Y4(Ku2QC8Aw;(1Vk z9-mBDIRI0HXbY2RqnKq*XRs8>S;du_=!LnJuRTAtcI=&I-s@JgY{vvgr5O$H7VU9f zRBHy_)vA_x$7bMeHac5TRA><+uk&wL=d?=OO=dR8Em9ba*{!D_5ZKvmx18OI9Nblz zet9IU&JjR7%@$;LDhgT(P=VN0QDi8W`L4K$n6|4nKCB3%8Tj_99vMoTcYE3B%)I-W zo_)Pey}egbpQ1y`!PY5{`f^|qsEUdtu`(MPP$nJjS%NZEE7IcLQYmRrRCshsR_@Ki z%DpAC@&z?!AZD#9nLo^NERI%rA7|tCc`R)uY{ItN*<;p8Y@zvNJ{^xxqN|(XOme3n z!6HqPonlekw~b^yY;}e3*YUs=heEhd&Pt^KxL!T)x9zP(+dc!aTEX^WXEk-I&rit$ z(CP|Zz&uxWoB{R>6f8l{7!<#DLS(_YMU`Pb9R-z`MK>x4MZ*>irafzFzjWB-Zy*qN0+a=eFiC|Oz(=b)_F{_#pC8Se0P8yp@$R%-t(OQ-0a8ehjew8RVB zuVu&USk5ZGvxlCPFh?AYMsV%N`1>BzXVx>g(w?;&@k_B@?Jrssoen@T>D86Qubx8} z)JP9=bgka0HCjU-SK>9|t6b^Ku5^Ayk8to+FmId#YD!0k{I{(n)xKzLcY^suzDl)z zu7MKbQQvC6V_tv)d zv^eUofZCgKPuo1h`n2bJmP-1ldRo`EUqDm6hNYAN!yF|<1SdJ4#tgE_Ak&x7hRV7WDT9 zUQR#H5KD&O<4^b|J)k{`VB#ICXeFvr4$L%$X5Q*;8(gRd75hA>b& zqfzXVp2&zqe_>{NiO=*X=B<7u@)EY}U#GnxJ*VIn^Geo-%;xIT^yL>V#MAktFDgdd z0X@jY%qXm0$l}?7*oe{O3J?&bXJH^N&D^=?C*hYm{(D6Fj zU7gp-X`R&_BhEfP5AkBwB0C=vg}Sg6r#IFE)Ejj`1QBg;_aG&&I{3#~L{_YEX(4Sh zhc)KxYICUM=aPD7M6YMv^iwA@;b5KH7tM;Iwrp?wSP z6Kz7P53N`AADfS!NtziyT9~CZisU|ZI=;*TW$sn$={jGvc*He{J8ATf(d|bqr9R8W zvbYqga8aDS8ZUVY$d^N(m`tl>tGibbM6F@VW*(aJJb@MufwQy&m!c@P%AVa{ z-m~MhI?`3&ffw#$a-TOQxtLXKS^m4Ij>C+|@2g%J`iw-nHJjfJkWn)m?qctmHB3rw z{;cYs;!nDMT`ehXj*ryZE+Y~ublrR&06z$z)H7D2-{*F{u*-3vv`{H>6W*X}KOCwyVvTr50 z5oZ1=B)gsh!8KjK3a-VZp!n+eQHo+3b%>Tf#UZ>YP4`Bqt^1U-;aM^Mr-5lphyd|3 zCGnksdPv9F!`!&KZRXK%7OMR5wbl$8j;FntH>GON^|CB}V3)RgA2YcL?rJYUg-)!uS~1gwv&qR(br3 zdF{ujcj`VLB{9i8&b&odi&xDc-(>U>lW7VGSE90RK2v;Qn#dJbZkQ{&-GmS!J2;ib zQzu8Lpx-8uNi3->rxh<&DywIdd1>Wy%9SKdF*{|R2wEoBdR2K^-k%mLn~{1m2zkBqt7Fsa(dls_k)9gTiZm`09Z?N; zw0Fu2%TY!tl3rnU7vB?#&8MENvysz$E;f(06peADC_@REt?6B&a{WvP7t&I!h6|yN z<1%(ph^u`LQdWwaJM&Ryzj4V|Ic==DKt7YANBPgc`1wb5?Af-zY=#vjaY)Z~mE%xs zbo=U@YMm!%uJ`+RhLx8>KJSZDNd8!{%5x6Q+wVA3mSe{N+Slg_GuVNilhu<;?)lC1 znP0KVaco8Q-)JX46e+l8q+X0>vVlP&XJZYZqTlM)jpSE%B>NiUdvyu-u54VY( zcs|<|qjHG*l7sRYc|Pmh#(TPd$Q4m4q<8YMR;lNy|Du$6&R_p?Ou44yzs&Gd5-mW- z?Apqh&%N-@3A9HG;nSGIlIIcP@zQ01@n~=pt!S3~f76cO%Xs>U#f~sC#hwSxqk!mI G-v0%NsT1x1 diff --git a/src/coreclr/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestNative.cpp b/src/coreclr/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestNative.cpp index 72df3d5..5ecf88a 100644 --- a/src/coreclr/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestNative.cpp +++ b/src/coreclr/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/src/coreclr/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestPInvokeDef.cs b/src/coreclr/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestPInvokeDef.cs index 88c3e74..548c511 100644 --- a/src/coreclr/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestPInvokeDef.cs +++ b/src/coreclr/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)] -- 2.7.4