Skip to content

Commit

Permalink
Don't block non-blittable pointer types in P/Invokes. (dotnet#1866)
Browse files Browse the repository at this point in the history
* Don't block non-blittable pointer types in P/Invokes.

Don't block non-blittable pointer types in P/Invokes. If you're using pointers in interop you're already using unsafe code so we're going to assume you know what you're doing. C# won't let you form a pointer to a reference, so there's no risk of passing an object reference directly to native unless you're writing IL directly.

Fixes dotnet/coreclr#27800

* Delete unused resources.

* Apply fix to managed type system as well.

* Add positive test for nonblittable pointers. Remove negative tests that were in GenericTest.
  • Loading branch information
jkoritzinsky authored Jan 23, 2020
1 parent a680821 commit 53e8fea
Show file tree
Hide file tree
Showing 24 changed files with 50 additions and 404 deletions.
2 changes: 0 additions & 2 deletions src/coreclr/src/dlls/mscorrc/mscorrc.rc
Original file line number Diff line number Diff line change
Expand Up @@ -608,8 +608,6 @@ BEGIN
IDS_EE_BADMARSHAL_SAFEHANDLEARRAY "Invalid managed/unmanaged type combination (Arrays of SafeHandles are not supported)."
IDS_EE_BADMARSHAL_CRITICALHANDLEARRAY "Invalid managed/unmanaged type combination (Arrays of CriticalHandles are not supported)."
IDS_EE_BADMARSHAL_SYSARRAY "Invalid managed/unmanaged type combination (System.Array must be paired with SafeArray or Interface)."
IDS_EE_BADMARSHAL_PTRSUBTYPE "Pointers cannot reference managed objects. Use ByRef instead."
IDS_EE_BADMARSHAL_PTRNONBLITTABLE "Pointers cannot reference marshaled structures. Use ByRef instead."
IDS_EE_BADMARSHAL_RESTRICTION "This type can only be marshaled in restricted ways."
IDS_EE_BADMARSHAL_ASANYRESTRICTION "AsAny cannot be used on return types, ByRef parameters, ArrayWithOffset, or parameters passed from unmanaged to managed."
IDS_EE_BADMARSHAL_VBBYVALSTRRESTRICTION "VBByRefStr can only be used in combination with in/out, ByRef managed-to-unmanaged strings."
Expand Down
2 changes: 0 additions & 2 deletions src/coreclr/src/dlls/mscorrc/resource.h
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,6 @@
#define IDS_EE_BADMARSHAL_BADMANAGED 0x1756
#define IDS_EE_SRC_OBJ_NOT_COMOBJECT 0x1757
#define IDS_EE_CANNOT_COERCE_COMOBJECT 0x1759
#define IDS_EE_BADMARSHAL_PTRSUBTYPE 0x175b
#define IDS_EE_BADMARSHAL_PTRNONBLITTABLE 0x175c

#define IDS_EE_BADMARSHAL_RESTRICTION 0x175d
#define IDS_EE_BADMARSHAL_ASANYRESTRICTION 0x175f
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ namespace Internal.TypeSystem.Interop
{
public static partial class MarshalHelpers
{
internal static TypeDesc GetNativeTypeFromMarshallerKind(TypeDesc type,
MarshallerKind kind,
internal static TypeDesc GetNativeTypeFromMarshallerKind(TypeDesc type,
MarshallerKind kind,
MarshallerKind elementMarshallerKind,
#if !READYTORUN
InteropStateManager interopStateManager,
Expand Down Expand Up @@ -121,7 +121,7 @@ internal static TypeDesc GetNativeTypeFromMarshallerKind(TypeDesc type,
#if !READYTORUN
interopStateManager,
#endif
marshalAs,
marshalAs,
isArrayElement: true);

return elementNativeType.MakePointerType();
Expand Down Expand Up @@ -438,22 +438,6 @@ internal static MarshallerKind GetMarshallerKind(
}
else if (type.IsPointer)
{
#if READYTORUN
TypeDesc parameterType = ((PointerType)type).ParameterType;

if ((!parameterType.IsEnum
&& !parameterType.IsPrimitive
&& !MarshalUtils.IsBlittableType(parameterType))
|| parameterType.IsGCPointer)
{
// Pointers cannot reference marshaled structures. Use ByRef instead.
return MarshallerKind.Invalid;
}
#else
// Do not bother enforcing the above artificial restriction
// See https://github.com/dotnet/coreclr/issues/27800
#endif

if (nativeType == NativeTypeKind.Default)
return MarshallerKind.BlittableValue;
else
Expand Down
18 changes: 0 additions & 18 deletions src/coreclr/src/vm/mlinfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1612,13 +1612,6 @@ MarshalInfo::MarshalInfo(Module* pModule,
// plus they are not marked as blittable.
if (!th.IsEnum())
{
// It should be blittable
if (!th.IsBlittable())
{
m_resID = IDS_EE_BADMARSHAL_PTRNONBLITTABLE;
IfFailGoto(E_FAIL, lFail);
}

// Check for Copy Constructor Modifier
if (sigtmp.HasCustomModifier(pModule, "Microsoft.VisualC.NeedsCopyConstructorModifier", ELEMENT_TYPE_CMOD_REQD) ||
sigtmp.HasCustomModifier(pModule, "System.Runtime.CompilerServices.IsCopyConstructed", ELEMENT_TYPE_CMOD_REQD) )
Expand All @@ -1634,17 +1627,6 @@ MarshalInfo::MarshalInfo(Module* pModule,
}
}
}
else
{
if (!(mtype2 != ELEMENT_TYPE_CLASS &&
mtype2 != ELEMENT_TYPE_STRING &&
mtype2 != ELEMENT_TYPE_OBJECT &&
mtype2 != ELEMENT_TYPE_SZARRAY))
{
m_resID = IDS_EE_BADMARSHAL_PTRSUBTYPE;
IfFailGoto(E_FAIL, lFail);
}
}
}
}

Expand Down
1 change: 1 addition & 0 deletions src/coreclr/tests/src/Interop/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ add_subdirectory(PInvoke/BestFitMapping/LPStr)
add_subdirectory(PInvoke/Delegate)
add_subdirectory(PInvoke/Primitives/Int)
add_subdirectory(PInvoke/Primitives/RuntimeHandles)
add_subdirectory(PInvoke/Primitives/Pointer)
add_subdirectory(PInvoke/SizeParamIndex/PInvoke/PassingByOut)
add_subdirectory(PInvoke/SizeParamIndex/PInvoke/PassingByRef)
add_subdirectory(PInvoke/SizeParamIndex/ReversePInvoke/PassingByOut)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,24 +11,15 @@ unsafe partial class GenericsNative
[DllImport(nameof(GenericsNative))]
public static extern bool? GetNullableB(bool hasValue, bool value);

[DllImport(nameof(GenericsNative))]
public static extern void GetNullableBOut(bool hasValue, bool value, bool?* pValue);

[DllImport(nameof(GenericsNative))]
public static extern void GetNullableBOut(bool hasValue, bool value, out bool? pValue);

[DllImport(nameof(GenericsNative))]
public static extern bool?* GetNullableBPtr(bool hasValue, bool value);

[DllImport(nameof(GenericsNative), EntryPoint = "GetNullableBPtr")]
public static extern ref readonly bool? GetNullableBRef(bool hasValue, bool value);

[DllImport(nameof(GenericsNative))]
public static extern bool? AddNullableB(bool? lhs, bool? rhs);

[DllImport(nameof(GenericsNative))]
public static extern bool? AddNullableBs(bool?* pValues, int count);

[DllImport(nameof(GenericsNative))]
public static extern bool? AddNullableBs([MarshalAs(UnmanagedType.LPArray, SizeParamIndex = 1)] bool?[] pValues, int count);

Expand All @@ -42,17 +33,8 @@ private static void TestNullableB()
{
Assert.Throws<MarshalDirectiveException>(() => GenericsNative.GetNullableB(true, false));

Assert.Throws<MarshalDirectiveException>(() => {
bool? value2;
GenericsNative.GetNullableBOut(true, false, &value2);
});

Assert.Throws<MarshalDirectiveException>(() => GenericsNative.GetNullableBOut(true, false, out bool? value3));

Assert.Throws<MarshalDirectiveException>(() => GenericsNative.GetNullableBPtr(true, false));

Assert.Throws<MarshalDirectiveException>(() => GenericsNative.GetNullableBRef(true, false));

Assert.Throws<MarshalDirectiveException>(() => GenericsNative.AddNullableB(default, default));

bool?[] values = new bool?[] {
Expand All @@ -63,13 +45,6 @@ private static void TestNullableB()
default
};

Assert.Throws<MarshalDirectiveException>(() => {
fixed (bool?* pValues = &values[0])
{
GenericsNative.AddNullableBs(pValues, values.Length);
}
});

Assert.Throws<MarshalDirectiveException>(() => GenericsNative.AddNullableBs(values, values.Length));

Assert.Throws<MarshalDirectiveException>(() => GenericsNative.AddNullableBs(in values[0], values.Length));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,24 +11,15 @@ unsafe partial class GenericsNative
[DllImport(nameof(GenericsNative))]
public static extern char? GetNullableC(bool hasValue, char value);

[DllImport(nameof(GenericsNative))]
public static extern void GetNullableCOut(bool hasValue, char value, char?* pValue);

[DllImport(nameof(GenericsNative))]
public static extern void GetNullableCOut(bool hasValue, char value, out char? pValue);

[DllImport(nameof(GenericsNative))]
public static extern char?* GetNullableCPtr(bool hasValue, char value);

[DllImport(nameof(GenericsNative), EntryPoint = "GetNullableCPtr")]
public static extern ref readonly char? GetNullableCRef(bool hasValue, char value);

[DllImport(nameof(GenericsNative))]
public static extern char? AddNullableC(char? lhs, char? rhs);

[DllImport(nameof(GenericsNative))]
public static extern char? AddNullableCs(char?* pValues, int count);

[DllImport(nameof(GenericsNative))]
public static extern char? AddNullableCs([MarshalAs(UnmanagedType.LPArray, SizeParamIndex = 1)] char?[] pValues, int count);

Expand All @@ -42,17 +33,8 @@ private static void TestNullableC()
{
Assert.Throws<MarshalDirectiveException>(() => GenericsNative.GetNullableC(true, '1'));

Assert.Throws<MarshalDirectiveException>(() => {
char? value2;
GenericsNative.GetNullableCOut(true, '1', &value2);
});

Assert.Throws<MarshalDirectiveException>(() => GenericsNative.GetNullableCOut(true, '1', out char? value3));

Assert.Throws<MarshalDirectiveException>(() => GenericsNative.GetNullableCPtr(true, '1'));

Assert.Throws<MarshalDirectiveException>(() => GenericsNative.GetNullableCRef(true, '1'));

Assert.Throws<MarshalDirectiveException>(() => GenericsNative.AddNullableC(default, default));

char?[] values = new char?[] {
Expand All @@ -63,13 +45,6 @@ private static void TestNullableC()
default
};

Assert.Throws<MarshalDirectiveException>(() => {
fixed (char?* pValues = &values[0])
{
GenericsNative.AddNullableCs(pValues, values.Length);
}
});

Assert.Throws<MarshalDirectiveException>(() => GenericsNative.AddNullableCs(values, values.Length));

Assert.Throws<MarshalDirectiveException>(() => GenericsNative.AddNullableCs(in values[0], values.Length));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,24 +11,15 @@ unsafe partial class GenericsNative
[DllImport(nameof(GenericsNative))]
public static extern double? GetNullableD(bool hasValue, double value);

[DllImport(nameof(GenericsNative))]
public static extern void GetNullableDOut(bool hasValue, double value, double?* pValue);

[DllImport(nameof(GenericsNative))]
public static extern void GetNullableDOut(bool hasValue, double value, out double? pValue);

[DllImport(nameof(GenericsNative))]
public static extern double?* GetNullableDPtr(bool hasValue, double value);

[DllImport(nameof(GenericsNative), EntryPoint = "GetNullableDPtr")]
public static extern ref readonly double? GetNullableDRef(bool hasValue, double value);

[DllImport(nameof(GenericsNative))]
public static extern double? AddNullableD(double? lhs, double? rhs);

[DllImport(nameof(GenericsNative))]
public static extern double? AddNullableDs(double?* pValues, int count);

[DllImport(nameof(GenericsNative))]
public static extern double? AddNullableDs([MarshalAs(UnmanagedType.LPArray, SizeParamIndex = 1)] double?[] pValues, int count);

Expand All @@ -42,17 +33,8 @@ private static void TestNullableD()
{
Assert.Throws<MarshalDirectiveException>(() => GenericsNative.GetNullableD(true, 1.0));

Assert.Throws<MarshalDirectiveException>(() => {
double? value2;
GenericsNative.GetNullableDOut(true, 1.0, &value2);
});

Assert.Throws<MarshalDirectiveException>(() => GenericsNative.GetNullableDOut(true, 1.0, out double? value3));

Assert.Throws<MarshalDirectiveException>(() => GenericsNative.GetNullableDPtr(true, 1.0));

Assert.Throws<MarshalDirectiveException>(() => GenericsNative.GetNullableDRef(true, 1.0));

Assert.Throws<MarshalDirectiveException>(() => GenericsNative.AddNullableD(default, default));

double?[] values = new double?[] {
Expand All @@ -63,13 +45,6 @@ private static void TestNullableD()
default
};

Assert.Throws<MarshalDirectiveException>(() => {
fixed (double?* pValues = &values[0])
{
GenericsNative.AddNullableDs(pValues, values.Length);
}
});

Assert.Throws<MarshalDirectiveException>(() => GenericsNative.AddNullableDs(values, values.Length));

Assert.Throws<MarshalDirectiveException>(() => GenericsNative.AddNullableDs(in values[0], values.Length));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,25 +10,15 @@ unsafe partial class GenericsNative
{
[DllImport(nameof(GenericsNative))]
public static extern float? GetNullableF(bool hasValue, float value);

[DllImport(nameof(GenericsNative))]
public static extern void GetNullableFOut(bool hasValue, float value, float?* pValue);

[DllImport(nameof(GenericsNative))]
public static extern void GetNullableFOut(bool hasValue, float value, out float? pValue);

[DllImport(nameof(GenericsNative))]
public static extern float?* GetNullableFPtr(bool hasValue, float value);

[DllImport(nameof(GenericsNative), EntryPoint = "GetNullableFPtr")]
public static extern ref readonly float? GetNullableFRef(bool hasValue, float value);

[DllImport(nameof(GenericsNative))]
public static extern float? AddNullableF(float? lhs, float? rhs);

[DllImport(nameof(GenericsNative))]
public static extern float? AddNullableFs(float?* pValues, int count);

[DllImport(nameof(GenericsNative))]
public static extern float? AddNullableFs([MarshalAs(UnmanagedType.LPArray, SizeParamIndex = 1)] float?[] pValues, int count);

Expand All @@ -42,17 +32,8 @@ private static void TestNullableF()
{
Assert.Throws<MarshalDirectiveException>(() => GenericsNative.GetNullableF(true, 1.0f));

Assert.Throws<MarshalDirectiveException>(() => {
float? value2;
GenericsNative.GetNullableFOut(true, 1.0f, &value2);
});

Assert.Throws<MarshalDirectiveException>(() => GenericsNative.GetNullableFOut(true, 1.0f, out float? value3));

Assert.Throws<MarshalDirectiveException>(() => GenericsNative.GetNullableFPtr(true, 1.0f));

Assert.Throws<MarshalDirectiveException>(() => GenericsNative.GetNullableFRef(true, 1.0f));

Assert.Throws<MarshalDirectiveException>(() => GenericsNative.AddNullableF(default, default));

float?[] values = new float?[] {
Expand All @@ -63,13 +44,6 @@ private static void TestNullableF()
default
};

Assert.Throws<MarshalDirectiveException>(() => {
fixed (float?* pValues = &values[0])
{
GenericsNative.AddNullableFs(pValues, values.Length);
}
});

Assert.Throws<MarshalDirectiveException>(() => GenericsNative.AddNullableFs(values, values.Length));

Assert.Throws<MarshalDirectiveException>(() => GenericsNative.AddNullableFs(in values[0], values.Length));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,24 +11,15 @@ unsafe partial class GenericsNative
[DllImport(nameof(GenericsNative))]
public static extern long? GetNullableL(bool hasValue, long value);

[DllImport(nameof(GenericsNative))]
public static extern void GetNullableLOut(bool hasValue, long value, long?* pValue);

[DllImport(nameof(GenericsNative))]
public static extern void GetNullableLOut(bool hasValue, long value, out long? pValue);

[DllImport(nameof(GenericsNative))]
public static extern long?* GetNullableLPtr(bool hasValue, long value);

[DllImport(nameof(GenericsNative), EntryPoint = "GetNullableLPtr")]
public static extern ref readonly long? GetNullableLRef(bool hasValue, long value);

[DllImport(nameof(GenericsNative))]
public static extern long? AddNullableL(long? lhs, long? rhs);

[DllImport(nameof(GenericsNative))]
public static extern long? AddNullableLs(long?* pValues, int count);

[DllImport(nameof(GenericsNative))]
public static extern long? AddNullableLs([MarshalAs(UnmanagedType.LPArray, SizeParamIndex = 1)] long?[] pValues, int count);

Expand All @@ -42,17 +33,8 @@ private static void TestNullableL()
{
Assert.Throws<MarshalDirectiveException>(() => GenericsNative.GetNullableL(true, 1L));

Assert.Throws<MarshalDirectiveException>(() => {
long? value2;
GenericsNative.GetNullableLOut(true, 1L, &value2);
});

Assert.Throws<MarshalDirectiveException>(() => GenericsNative.GetNullableLOut(true, 1L, out long? value3));

Assert.Throws<MarshalDirectiveException>(() => GenericsNative.GetNullableLPtr(true, 1L));

Assert.Throws<MarshalDirectiveException>(() => GenericsNative.GetNullableLRef(true, 1L));

Assert.Throws<MarshalDirectiveException>(() => GenericsNative.AddNullableL(default, default));

long?[] values = new long?[] {
Expand All @@ -63,13 +45,6 @@ private static void TestNullableL()
default
};

Assert.Throws<MarshalDirectiveException>(() => {
fixed (long?* pValues = &values[0])
{
GenericsNative.AddNullableLs(pValues, values.Length);
}
});

Assert.Throws<MarshalDirectiveException>(() => GenericsNative.AddNullableLs(values, values.Length));

Assert.Throws<MarshalDirectiveException>(() => GenericsNative.AddNullableLs(in values[0], values.Length));
Expand Down
Loading

0 comments on commit 53e8fea

Please sign in to comment.