From 0b3fc197fb017a1b7c19b60f31744bb8d63ca710 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Wed, 12 Dec 2018 15:03:15 -0800 Subject: [PATCH 01/13] Throw an exception when passing strings by-value as out parameters. --- src/dlls/mscorrc/mscorrc.rc | 1 + src/dlls/mscorrc/resource.h | 3 +-- src/vm/ilmarshalers.h | 14 +++++++++++++- .../StringMarshalling/LPTSTR/LPTSTRTest.cs | Bin 17934 -> 18492 bytes .../LPTSTR/LPTSTRTestNative.cpp | 15 +++++++++++++++ .../LPTSTR/LPTSTRTestPInvokeDef.cs | 6 +++++- 6 files changed, 35 insertions(+), 4 deletions(-) diff --git a/src/dlls/mscorrc/mscorrc.rc b/src/dlls/mscorrc/mscorrc.rc index 59aa7c338fce..f5b9e934d7d5 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 735f315c33c8..1ad2c418cb99 100644 --- a/src/dlls/mscorrc/resource.h +++ b/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 diff --git a/src/vm/ilmarshalers.h b/src/vm/ilmarshalers.h index 6a67bf686e47..ce095fea1d03 100644 --- a/src/vm/ilmarshalers.h +++ b/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 + + bool SupportsArgumentMarshal(DWORD dwMarshalFlags, UINT* pErrorResID) override + { + if (IsOut(dwMarshalFlags) && !IsByref(dwMarshalFlags)) + { + *pErrorResID = IDS_EE_BADMARSHAL_STRING_OUT; + return false; + } + + return true; + } + protected: virtual LocalDesc GetNativeType(); virtual LocalDesc GetManagedType(); diff --git a/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTest.cs b/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTest.cs index cc7369f155102d3b1e3ab1506bd8614b2ebd0b88..94e024d824aeb370160762711109c6e5a312fb19 100644 GIT binary patch delta 1118 zcmd^7K}!N*5FKF+l?^ORT~}Ev6jPBhgAQq08AKFz=@f;jkRfVnr8h5iiU=MAT{?R- zymsl*Il2e-59$^{(71UjC_Brtvv2mBdEfiqH|J-X+Y3!|--4(-tC_-wvdZy!!Z)%* zE0iY>m8n7{N>Gwx5<_MgQOV3Tv4{gr*D2nmI)=?Q)E8}7SGHh~L=-J{-!2vS$&r)e zA#*HHjW(Gj$61D!8D$xlc|D9P{kXQXOAW8$G}?+C+g2R}rxyuP2b1iED1~^30kqx5 z`4sn7C`EhR>!CGOS(&pA_Y$*~m_Nl8Svq1Arz&R|Dyn}9l*f&0X7q1OPkIRtb7Op3 zgX7m~<~zZQtb;KuY-B0Ts`7m13V$oCs>Jx1agyJEYo!1VEVAmYQ?cUScG-jSpKey= zVAn}M-ehlIJE_hPu0y7lQ+agsW+(D7bN7m?-v-qf5zq)vcI?%rDM%jNMPq$AqF=@~ O;{H}3))s_8GSDZu3=4Vy delta 531 zcmbV{ze>YU6vofRib2{^p*A&b)6{}P4brAkTl^R4QbZj31g2@MT5U0j+Ra695h)xH z$3B4wT{}2Bi;Iik<|saaCpReQ=x}pSzVCeJ{O+jFuCLhrfryeci-Szy+<|@QfC&~T z;KCliyKq9a0tS6uu&JZL6$>0vo6w~$hf-e88U5MNrfQd03ORnJ11(TVcSu)ghM)9P zCf{U2TPFGM+EZG5NDD1kz+cyqe-&T!;y2tFX@qBDf6Y&Nh#$K43-MhK6XMQK&wns!=1{E$$G-sd CD~LS+ diff --git a/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestNative.cpp b/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestNative.cpp index df9a6d364948..b582e3646f6c 100644 --- a/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestNative.cpp +++ b/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestNative.cpp @@ -33,6 +33,21 @@ extern "C" LPWSTR ReturnErrString() } //Test Method1 +extern "C" DLL_EXPORT LPWSTR Marshal_In(/*[In,Out]*/LPWSTR s) +{ + + //Check the Input + size_t len = wcslen(s); + + if((len != lenstrManaged)||(wmemcmp(s,(WCHAR*)strManaged,len)!=0)) + { + printf("Error in Function Marshal_InOut(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 f54397ba6bca..151879934521 100644 --- a/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestPInvokeDef.cs +++ b/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); } -} \ No newline at end of file +} From 30ee1d9208e058cb6b6befec3e3241deb88ccd7f Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Wed, 12 Dec 2018 15:26:59 -0800 Subject: [PATCH 02/13] Fix encoding --- .../StringMarshalling/LPTSTR/LPTSTRTest.cs | Bin 18492 -> 8994 bytes 1 file changed, 0 insertions(+), 0 deletions(-) diff --git a/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTest.cs b/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTest.cs index 94e024d824aeb370160762711109c6e5a312fb19..b66a6de6f7599b47326d747d0cb9b2ca8dd98f2e 100644 GIT binary patch literal 8994 zcmeHM%~IP)5I(P;V%9kz64qb{V3y(l#Hn=^BowALhmfL#)V8%NOWx6lpGtActL4GA zdqyLTbVDdA)&IJ*zT zgu(O{wT;p+WQpKGG;X!h1TSp(m|wZ6An3_ou;jLG2bOF(r6`{)K?%AqZ9CIT0G zEQnBj98cI0+d5>Mn;g8Dqwi328b`HIJD$ij=$p@Xz{A+}*%ZVi_R$q+I}Wp$3!0AN zf2)ee-SKX=xuYz^l65)lyZk1wr*V|TA#}d*K)_iLfzxJ3M@JI$pKbOIR~+V(If#@; z%z5`tV$be8y=rH8n4w`eGFLIz6R8_ELc{t}1o#q{l7|4>*AXeAKF3n5oRICX8uUIS z9uICI$G-w~$humt(&Jjxs(3AyRP8p7LpDhI|Do9(j3Eq^lQak^CnVhsRvFo@dL|e# z=l7)Bxr)_HXT2juB~G$!46dtn~%$PE)HT%xFB+!s8}ZmU~wAo5i>RbuNe z2xq`mOksSyUE(9@i!v@HyD8OSYMf!{&UoMoVDJzK=EXkfiA(JD_H8v-`Y4^Uhhww2 z3^R;JwpC|WOZ;9O4~K0dY7O&tx3+A?2Xu(p_ci3>W1OX2MBM@ zU7k$b5bvJc45H7es8FA0R;LZ=lUt!I580bw**Rhn+?PYeX{%~*y``?E(q$QxF@y(x zt)QPj+8bTNZ!+Y#5nMXr3`xe_+R!T(g7&$rAgta*U(emh9b;)720nb#<*SqF&9D#f zgV6{$HVzmoZiE3ssU01@!P=w*GeH{)8PSe~b{4uGIBP$DtZlHh>6$SQtD)W088F;o z- zg&X9vEqmzoiYhruOFH{XkwE%i3}kY_zOIv{4NMu|%z!R{Ubx)tB2FC13{?OvW}yXW zl2tIdL`pH3tctAQv`z;*PQ$+qgWI|zlDiz*8`Ni`aO&wY&=}nJJdVcbA+@o-n*Pe7 zi#*@ho(Q63QrVVZ$eld??S?5(yGlKArB4w2F?Zqj{1*YpaN~uHxn1KDnptNP?TBr| z5k;o*T;WZalM&)WfPFeO|NRFKX7rd)SC{8iDq0&tW7tr7y)wT}tUZ@oR6{#sn#RW? zd;k#>9^W$;Gl49I^Gb9~k{ihINenv^RWNpPvp4iPA}M*iqUqw!-f8A+#Gzfi^UKx5 zK_sEP;8*^`bop*R+RWt_G1eX6s^Kd6z8tN)H4ZFhVRE8L^ZYJt;cysHBA^VgYq-`( zr@Jdor=|TD&#ETc!WkO3)W-m>>fO?%;3e!Ac%XsZU*?5!MNklAQK(wGCU5L5Nup}{ zD9Awo_73sE?I>$LI;ZjMkzzb6$p#gQRY4`S4<~6@&;roqc1v3GbQaW$fjZb09eXLIBcGSs1&k~^qB7`t+bk3k5r)L z^9ic}po8Q9B5n-xWg7GvaP{`FK&v)XPxv!UEFg^@A6dI}-}GrRSw*{#gMp3L;C zBcVMt*U4ypZ}N9WoAf&LA3#i9sM66-lBs8){HG9Ud5ha#by9@nEHt)F><-eI+Xo7GdSV zidp%L8V!h9t4i(=sAPpmrjTbI5${jm`_JR6=u;53!!M(puua;n!3*&Hu>8+uXTT& zE2%?R)jf?v{aUWEl;>^6JRn zQ7_O$J_0;+w)bf;r literal 18492 zcmeHPTW=Fb6rQS2th@cfC%-kxBb33 zKAD;I%zD;jv5}DF*tD#|=a9x;= zd18JwJNUMN`{!u4XS!%}gsT(Wl{@=pfbYD+K7D(2Y(IJ3HHWyfkJmBYC+Ix{-S5n| z;D9{rn7pxi0k1>!JVVbTP%F#oK+D1FJ)EUVz%+uAMMC$LED`|&c68*eRt8fuvdNi?O;qR>?wDTJ&g(%s+*k=$7i581f5;9 zq@Bn?A6MsAc75vjT?3zI)(Y>S7jjpi*RgqxzqIiQdiL-tEY$(poWMpLm2Xm&I|b-m z*f#9lF~3`V()zSy2U?*uY5l%AMO%J*i>LIcq4}kfTJ=+owGSz2Q|eOuZBm~dw0@7) z@A9$L!EA0^(!IMggXi8QEd-v4l^lLAlpx(*5Vp)2+OmgQ6IPK$4V4n6VQ zwrkn`+Lu%%c6QN{8fK4!Q4fxNAMYQ*ePJVmBkfwhk+>A^)%IdVvFQLDlV2T4;_4}E zL5uW2qigL>tI-?!xRR)mSmj8kcBHdQenf(A0=jVtXesR-%HOt{RR5y4-3aK3ewAze zTn8t_qrSC%$2@}!^x}PsR>YXLX<2REw;Izif3|mT;t9tQA5fb5NlDMFb%H|W24$Xo%e;U$)wd^m&4c}3 zBgz@)1#-y{a{P_hWCXNF8BC&M4R?vE)B`Jxp;@$c+lCYxLE~-iWW~`D;-5%x%~I_E z|NFt%ZBXfWoDI=-Xzt)%tmTc;m>Mr}-_WNaDiM!*7-K(yVIeBTX;3fcV-&)k*YTY7 z97orTMt}BwdOY9#hSoicV*~i7-&V5D_hvS)aZZ?*6ziGov`XuOd&$`RdjxGWx3EHb zk1OV6W?lN)IL+C}#!-D<;+cpjK>oFYq38O&;y$SU@3J845UE7M{_1IQI{Gv(dv5`K zw()9#$6o?2m5zRfv9XByh!v7Am$nSOTkq0a8B4JqS0Tv;B-un|QkWm`R>bzTDOuE2 zdQw>tMQ)%6>v&f30~?2XQ)y%|OCzCvQqiw^^Yv9m0Y;Y8melMPyzT_oee+jI&!>>< zHT2IJfy^~r{ac0I>lOif=9R70`S}=kSmnO4Po(b}?r&n`rx@jILdF=3#e&MIpT4|$ zuZlsb8qzccw^&zle#mOBK2KkN)j~d9Ova*O z#5Kqo&wJ$zsWnu{Q@rnVIg>t^nsv0`FK(t&VD8S0TF;-JTc0~Y&qJ$K+V#k0g_p4L z2JX9gUZ-Sr&hD6T_V9Ut7poRo`H(Esg|9ffaXvu1(FQ~iu?ANUQu?Zcf21P1;vAPA z(l!eiW5JHLfSLS4((cUY^}L&L>U1U&-AmG!7$WvjA95w$8%ik1cexxxMdbOw3*kZ8m|My6G`b1J>@F& zZ^3<{P3ZNZ_p1G4^cb1snfapyDy>l__qo&lRURm`*KDLKy}W$HF^M~A^pElFXD#Kv zDCM%a7ILH*r=BNEnFGq@&?lzTylVCNWd>1i*s_&});tek#RK3h{lHZzQk}enep5AW zOtV{-Mi=%Fy-J(V!{VBaQ4{}CTKQaS>^N>xGPv46#@7+c6@mY>`HY`0so;E7w!eiL zm8`vrY?`k^eM`oF2m5pN@^BPYzSQ(a|2iFUd(9&H?=~F$ zqM35!-~AH#7Q?O~Za8B~WrZ)1uPCaYvAQ&c1WTVB3ASKQnOSia!(VZ$!iK8syi_|r zW7MP;Q(8HApO&O;^DOs)!a8Cp8PhJ$Ne|^_{;S@!7G_Zcr!^d}m&$tAif;+CFSmOr zdvvT}{F&r@%sM_+|CE>~Rpca37#US_5ML8+CgSv0UANZ3sO6bs)a2SF)LR(!%c$UG zFO#b>+sI|Q$K1g>08ctyT&^vJ^YWAqW1|UTERV2o8vN2aMDlGJyOwKW*K#6)gwOem z9nruna4W6ddbpL+x*x5dzwVpCtVuQ?X4S#1d(qZS&awns!hOuDs>;Wr@$)XkDaG@9 zBV5T@Hx+csL$b*n2wv#;Ik*SQoofR!-3(>vv$a002@0(>boQFB&^j2#I4X3kS9eiAwAwEv7=)@kfKU0m3D?xpi z?}b;3kSjGRKj&-JHQ!1`%~yqxD|RPP;&xs!a&|S}$`#{d+!BhHw1rxtjDNAH{Sb{# zU1MSfJjL8)fZ0rVc7{I%u+HN+-%u&Vbq%O07RubM%=tFcQ=r>RxA)u^tUuN0qQ zRW)}7PArszhpQi1E4tN$5FtA_70FXihw!;KNo1PLr&MpsWxjWowN%h7++FAKB%|o3vL#+)czwECn*HS46+AGeL z(U^0)bs#a%C_TV?fNxyMyM-q*Y~HbIh)aXGYC0Z`!Q_lSMG}EotB7e>DA~L5JWRQ`J}i|-6IdX zlzu9bx$bz?C%6_bC6VY4u`G^Onl-2e#XyIcUruJ}?;NW=aURyz)Z=DzGE4nPKXcBd zyS4*c;pqPdU{>>gDdo?sV<-`i*KNB88P5Hv8$Ib%9qqB+?aLF*b$(-&LI9V` Date: Wed, 12 Dec 2018 15:51:39 -0800 Subject: [PATCH 03/13] Don't use override in this PR. --- src/vm/ilmarshalers.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vm/ilmarshalers.h b/src/vm/ilmarshalers.h index ce095fea1d03..b2a5c7da1e84 100644 --- a/src/vm/ilmarshalers.h +++ b/src/vm/ilmarshalers.h @@ -1963,7 +1963,7 @@ class ILWSTRMarshaler : public ILMarshaler #endif // _DEBUG - bool SupportsArgumentMarshal(DWORD dwMarshalFlags, UINT* pErrorResID) override + virtual bool SupportsArgumentMarshal(DWORD dwMarshalFlags, UINT* pErrorResID) { if (IsOut(dwMarshalFlags) && !IsByref(dwMarshalFlags)) { From 753c5a240938e7fc68c82d1c216fc37635cac5f4 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Wed, 12 Dec 2018 16:29:48 -0800 Subject: [PATCH 04/13] Clean up Marshal_In Marshal_In was copied back into existence from Marshal_InOut. Clean it up a bit. --- .../Interop/StringMarshalling/LPTSTR/LPTSTRTestNative.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestNative.cpp b/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestNative.cpp index b582e3646f6c..5dcf7fb706b0 100644 --- a/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestNative.cpp +++ b/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestNative.cpp @@ -33,15 +33,14 @@ extern "C" LPWSTR ReturnErrString() } //Test Method1 -extern "C" DLL_EXPORT LPWSTR Marshal_In(/*[In,Out]*/LPWSTR s) +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_InOut(Native Client)\n"); + printf("Error in Function Marshal_In(Native Client)\n"); return ReturnErrString(); } From bb99d157ccb8f8d58defcb4c2bbb397197a78cc6 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Wed, 12 Dec 2018 16:30:44 -0800 Subject: [PATCH 05/13] Remove extraneous whitespace. --- .../src/Interop/StringMarshalling/LPTSTR/LPTSTRTestNative.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestNative.cpp b/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestNative.cpp index 5dcf7fb706b0..b4fb3a4ae860 100644 --- a/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestNative.cpp +++ b/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestNative.cpp @@ -36,14 +36,14 @@ extern "C" LPWSTR ReturnErrString() extern "C" DLL_EXPORT LPWSTR Marshal_In(/*[In]*/LPWSTR s) { //Check the Input - size_t len = wcslen(s); + 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(); } From af6fdbbc71d516d4cebdd5f8c60f04d244459b75 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Wed, 12 Dec 2018 18:01:20 -0800 Subject: [PATCH 06/13] Fix failing test. --- .../Interop/StringMarshalling/LPSTR/LPSTRTest.cs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/tests/src/Interop/StringMarshalling/LPSTR/LPSTRTest.cs b/tests/src/Interop/StringMarshalling/LPSTR/LPSTRTest.cs index 74a2087eca08..893d23c95bff 100644 --- a/tests/src/Interop/StringMarshalling/LPSTR/LPSTRTest.cs +++ b/tests/src/Interop/StringMarshalling/LPSTR/LPSTRTest.cs @@ -192,10 +192,19 @@ public static int Main(string[] args) } #region ReversePinvoke - DelMarshal_InOut d1 = new DelMarshal_InOut(Call_DelMarshal_InOut); - if (!PInvokeDef.RPinvoke_DelMarshal_InOut(d1, "ň")) + bool inOutByValueThrows = false; + try { - ReportFailure("Method RPinvoke_DelMarshal_InOut[Managed Side],Return value is false"); + DelMarshal_InOut d1 = new DelMarshal_InOut(Call_DelMarshal_InOut); + PInvokeDef.RPinvoke_DelMarshal_InOut(d1, "ň"); + } + catch (MarshalDirectiveException) + { + inOutByValueThrows = true; + } + if (!inOutByValueThrows) + { + ReportFailure("Method RPinvoke_DelMarshal_InOut didn't throw a MarshalDirectiveException."); } DelMarshalPointer_Out d2 = new DelMarshalPointer_Out(Call_DelMarshalPointer_Out); From cc6286133b7a91f1add72699d3f3859754e488e7 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 13 Dec 2018 13:21:33 -0800 Subject: [PATCH 07/13] Remove out attribute in COM string tests. --- tests/src/Interop/COM/ServerContracts/Server.Contracts.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/src/Interop/COM/ServerContracts/Server.Contracts.cs b/tests/src/Interop/COM/ServerContracts/Server.Contracts.cs index cc003edf6855..b2074cbd332d 100644 --- a/tests/src/Interop/COM/ServerContracts/Server.Contracts.cs +++ b/tests/src/Interop/COM/ServerContracts/Server.Contracts.cs @@ -146,7 +146,7 @@ string Add_BStr( void Reverse_LPWStr_Out([MarshalAs(UnmanagedType.LPWStr)] string a, [MarshalAs(UnmanagedType.LPWStr)] out string b); - void Reverse_LPWStr_OutAttr([MarshalAs(UnmanagedType.LPWStr)] string a, [Out][MarshalAs(UnmanagedType.LPWStr)] string b); + void Reverse_LPWStr_OutAttr([MarshalAs(UnmanagedType.LPWStr)] string a, [MarshalAs(UnmanagedType.LPWStr)] string b); [return: MarshalAs(UnmanagedType.LPWStr)] StringBuilder Reverse_SB_LPWStr([MarshalAs(UnmanagedType.LPWStr)] StringBuilder a); From 0e952f4f152cf6b68eaf7f79435b38315305902b Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 13 Dec 2018 14:51:10 -0800 Subject: [PATCH 08/13] Add back attribute and check for exception thow in COM tests. --- tests/src/Interop/COM/NETClients/Primitives/StringTests.cs | 3 +-- tests/src/Interop/COM/ServerContracts/Server.Contracts.cs | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/src/Interop/COM/NETClients/Primitives/StringTests.cs b/tests/src/Interop/COM/NETClients/Primitives/StringTests.cs index 46cd5d970169..0c1212ceaed4 100644 --- a/tests/src/Interop/COM/NETClients/Primitives/StringTests.cs +++ b/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( () => this.server.Reverse_LPWStr_OutAttr(local, actual)); } foreach (var s in reversableStrings) diff --git a/tests/src/Interop/COM/ServerContracts/Server.Contracts.cs b/tests/src/Interop/COM/ServerContracts/Server.Contracts.cs index b2074cbd332d..cc003edf6855 100644 --- a/tests/src/Interop/COM/ServerContracts/Server.Contracts.cs +++ b/tests/src/Interop/COM/ServerContracts/Server.Contracts.cs @@ -146,7 +146,7 @@ string Add_BStr( void Reverse_LPWStr_Out([MarshalAs(UnmanagedType.LPWStr)] string a, [MarshalAs(UnmanagedType.LPWStr)] out string b); - void Reverse_LPWStr_OutAttr([MarshalAs(UnmanagedType.LPWStr)] string a, [MarshalAs(UnmanagedType.LPWStr)] string b); + void Reverse_LPWStr_OutAttr([MarshalAs(UnmanagedType.LPWStr)] string a, [Out][MarshalAs(UnmanagedType.LPWStr)] string b); [return: MarshalAs(UnmanagedType.LPWStr)] StringBuilder Reverse_SB_LPWStr([MarshalAs(UnmanagedType.LPWStr)] StringBuilder a); From f9f3753d92205ee7d0e0ecad406727cb381c74fd Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 13 Dec 2018 15:21:34 -0800 Subject: [PATCH 09/13] Add block comment to explain the implementation of Reverse_LPWStr_OutAttr in the NETServer. --- tests/src/Interop/COM/NETServer/StringTesting.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/src/Interop/COM/NETServer/StringTesting.cs b/tests/src/Interop/COM/NETServer/StringTesting.cs index 3a510f5e9b26..c47a155f29b3 100644 --- a/tests/src/Interop/COM/NETServer/StringTesting.cs +++ b/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); } -} \ No newline at end of file +} From 1310f7e9b6f0dab5459300d7b5be57a0a33f1d7c Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 13 Dec 2018 16:24:05 -0800 Subject: [PATCH 10/13] Only throw in a CLR->Native marshalling situation. --- src/vm/ilmarshalers.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vm/ilmarshalers.h b/src/vm/ilmarshalers.h index b2a5c7da1e84..3b0b81084781 100644 --- a/src/vm/ilmarshalers.h +++ b/src/vm/ilmarshalers.h @@ -1965,7 +1965,7 @@ class ILWSTRMarshaler : public ILMarshaler virtual bool SupportsArgumentMarshal(DWORD dwMarshalFlags, UINT* pErrorResID) { - if (IsOut(dwMarshalFlags) && !IsByref(dwMarshalFlags)) + if (IsOut(dwMarshalFlags) && !IsByref(dwMarshalFlags) && IsCLRToNative(dwMarshalFlags)) { *pErrorResID = IDS_EE_BADMARSHAL_STRING_OUT; return false; From 0b9d310e38a7a3b2ca60e6d7e0e92c8dad05589a Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 14 Dec 2018 11:50:46 -0800 Subject: [PATCH 11/13] Fix asserts from changed code-paths used in ILWSTRMarshaler. --- src/vm/ilmarshalers.cpp | 64 +++++++++++-------- src/vm/ilmarshalers.h | 1 - .../StringMarshalling/LPSTR/LPSTRTest.cs | 17 ++--- 3 files changed, 41 insertions(+), 41 deletions(-) diff --git a/src/vm/ilmarshalers.cpp b/src/vm/ilmarshalers.cpp index 58319d85e452..6f2981735fad 100644 --- a/src/vm/ilmarshalers.cpp +++ b/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; diff --git a/src/vm/ilmarshalers.h b/src/vm/ilmarshalers.h index 3b0b81084781..3f45c98008c5 100644 --- a/src/vm/ilmarshalers.h +++ b/src/vm/ilmarshalers.h @@ -1985,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); diff --git a/tests/src/Interop/StringMarshalling/LPSTR/LPSTRTest.cs b/tests/src/Interop/StringMarshalling/LPSTR/LPSTRTest.cs index 893d23c95bff..2ea8b3355523 100644 --- a/tests/src/Interop/StringMarshalling/LPSTR/LPSTRTest.cs +++ b/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; } @@ -192,19 +191,11 @@ public static int Main(string[] args) } #region ReversePinvoke - bool inOutByValueThrows = false; - try - { - DelMarshal_InOut d1 = new DelMarshal_InOut(Call_DelMarshal_InOut); - PInvokeDef.RPinvoke_DelMarshal_InOut(d1, "ň"); - } - catch (MarshalDirectiveException) - { - inOutByValueThrows = true; - } - if (!inOutByValueThrows) + DelMarshal_InOut d1 = new DelMarshal_InOut(Call_DelMarshal_InOut); + + if (!PInvokeDef.RPinvoke_DelMarshal_InOut(d1, "ň")) { - ReportFailure("Method RPinvoke_DelMarshal_InOut didn't throw a MarshalDirectiveException."); + ReportFailure("Method RPinvoke_DelMarshal_InOut[Managed Side],Return value is false"); } DelMarshalPointer_Out d2 = new DelMarshalPointer_Out(Call_DelMarshalPointer_Out); From 51da4a17666ad44a850016ec6ac9304af135dea7 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 14 Dec 2018 14:32:52 -0800 Subject: [PATCH 12/13] Add comment and explicitly load in a null value (instead of leaving it uninitialized). --- src/vm/ilmarshalers.cpp | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/vm/ilmarshalers.cpp b/src/vm/ilmarshalers.cpp index 6f2981735fad..40c8609e49c7 100644 --- a/src/vm/ilmarshalers.cpp +++ b/src/vm/ilmarshalers.cpp @@ -281,17 +281,21 @@ void ILWSTRMarshaler::EmitConvertContentsCLRToNative(ILCodeStream* pslILEmit) void ILWSTRMarshaler::EmitConvertSpaceNativeToCLR(ILCodeStream* pslILEmit) { - LIMITED_METHOD_CONTRACT; + 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) { STANDARD_VM_CONTRACT; - ILCodeLabel* pIsNullLabelByref = pslILEmit->NewCodeLabel(); + ILCodeLabel* pIsNullLabel = pslILEmit->NewCodeLabel(); EmitLoadNativeValue(pslILEmit); - pslILEmit->EmitBRFALSE(pIsNullLabelByref); + pslILEmit->EmitBRFALSE(pIsNullLabel); EmitLoadNativeValue(pslILEmit); pslILEmit->EmitDUP(); @@ -301,7 +305,7 @@ void ILWSTRMarshaler::EmitConvertContentsNativeToCLR(ILCodeStream* pslILEmit) pslILEmit->EmitNEWOBJ(METHOD__STRING__CTOR_CHARPTR, 1); EmitStoreManagedValue(pslILEmit); - pslILEmit->EmitLabel(pIsNullLabelByref); + pslILEmit->EmitLabel(pIsNullLabel); } bool ILWSTRMarshaler::NeedsClearNative() From 7b7a496a8aa5f28b0ae43c61e9ef5d680963952f Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Wed, 6 Feb 2019 09:46:16 -0800 Subject: [PATCH 13/13] Apply suggestions from code review Co-Authored-By: jkoritzinsky --- .../src/Interop/StringMarshalling/LPTSTR/LPTSTRTestNative.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestNative.cpp b/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestNative.cpp index a8381f853f5f..4d60040687ea 100644 --- a/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestNative.cpp +++ b/tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTestNative.cpp @@ -36,9 +36,9 @@ extern "C" LPWSTR ReturnErrString() extern "C" DLL_EXPORT LPWSTR Marshal_In(/*[In]*/LPWSTR s) { //Check the Input - size_t len = wcslen(s); + size_t len = TP_slen(s); - if((len != lenstrManaged)||(wmemcmp(s,(WCHAR*)strManaged,len)!=0)) + if((len != lenstrManaged)||(TP_wmemcmp(s,(WCHAR*)strManaged,len)!=0)) { printf("Error in Function Marshal_In(Native Client)\n"); return ReturnErrString();