Skip to content

Revert "Support marshalling of null SafeHandle." #48193

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 12, 2021
Merged

Revert "Support marshalling of null SafeHandle." #48193

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/coreclr/System.Private.CoreLib/src/System/StubHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1241,8 +1241,7 @@ internal static IntPtr SafeHandleAddRef(SafeHandle pHandle, ref bool success)
{
if (pHandle == null)
{
success = false;
return IntPtr.Zero;
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.pHandle, ExceptionResource.ArgumentNull_SafeHandle);
}

pHandle.DangerousAddRef(ref success);
Expand All @@ -1252,7 +1251,11 @@ internal static IntPtr SafeHandleAddRef(SafeHandle pHandle, ref bool success)
// Releases the SH (to be called from finally block).
internal static void SafeHandleRelease(SafeHandle pHandle)
{
Debug.Assert(pHandle != null);
if (pHandle == null)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.pHandle, ExceptionResource.ArgumentNull_SafeHandle);
}

pHandle.DangerousRelease();
}

Expand Down
13 changes: 0 additions & 13 deletions src/coreclr/tools/Common/TypeSystem/Interop/IL/Marshaller.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1742,19 +1742,7 @@ protected override void EmitMarshalArgumentManagedToNative()
if (IsManagedByRef)
PropagateFromByRefArg(marshallingCodeStream, _managedHome);

// Boolean by-ref for DangerousAddRef().
var vAddRefed = emitter.NewLocal(Context.GetWellKnownType(WellKnownType.Boolean));
marshallingCodeStream.EmitLdc(0);
marshallingCodeStream.EmitStLoc(vAddRefed);

// Initialize native value to null.
marshallingCodeStream.EmitLdc(0);
marshallingCodeStream.Emit(ILOpcode.conv_i);
StoreNativeValue(marshallingCodeStream);

ILCodeLabel lHandleIsNull = emitter.NewCodeLabel();
LoadManagedValue(marshallingCodeStream);
marshallingCodeStream.Emit(ILOpcode.brfalse, lHandleIsNull);
LoadManagedValue(marshallingCodeStream);
marshallingCodeStream.EmitLdLoca(vAddRefed);
marshallingCodeStream.Emit(ILOpcode.call, emitter.NewToken(
Expand All @@ -1767,7 +1755,6 @@ protected override void EmitMarshalArgumentManagedToNative()
safeHandleType.GetKnownMethod("DangerousGetHandle",
new MethodSignature(0, 0, Context.GetWellKnownType(WellKnownType.IntPtr), TypeDesc.EmptyTypes))));
StoreNativeValue(marshallingCodeStream);
marshallingCodeStream.EmitLabel(lHandleIsNull);

ILCodeLabel lNotAddrefed = emitter.NewCodeLabel();
cleanupCodeStream.EmitLdLoc(vAddRefed);
Expand Down
13 changes: 9 additions & 4 deletions ...mon/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.SecKeyRef.Export.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,13 @@ internal static partial class Interop
{
internal static partial class AppleCrypto
{
private static readonly SafeCreateHandle s_nullExportString = new SafeCreateHandle();

[DllImport(Libraries.AppleCryptoNative)]
private static extern int AppleCryptoNative_SecKeyExport(
SafeSecKeyRefHandle? key,
int exportPrivate,
SafeCreateHandle? cfExportPassphrase,
SafeCreateHandle cfExportPassphrase,
out SafeCFDataHandle cfDataOut,
out int pOSStatus);

Expand All @@ -25,9 +27,9 @@ internal static SafeCFDataHandle SecKeyExportData(
bool exportPrivate,
ReadOnlySpan<char> password)
{
SafeCreateHandle? exportPassword = exportPrivate
SafeCreateHandle exportPassword = exportPrivate
? CoreFoundation.CFStringCreateFromSpan(password)
: null;
: s_nullExportString;

int ret;
SafeCFDataHandle cfData;
Expand All @@ -44,7 +46,10 @@ internal static SafeCFDataHandle SecKeyExportData(
}
finally
{
exportPassword?.Dispose();
if (exportPassword != s_nullExportString)
{
exportPassword.Dispose();
}
}

if (ret == 1)
Expand Down
23 changes: 14 additions & 9 deletions ...ibraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.X509.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ internal static partial class AppleCrypto
private static int AppleCryptoNative_X509ImportCertificate(
ReadOnlySpan<byte> keyBlob,
X509ContentType contentType,
SafeCreateHandle? cfPfxPassphrase,
SafeCreateHandle cfPfxPassphrase,
SafeKeychainHandle tmpKeychain,
int exportable,
out SafeSecCertificateHandle pCertOut,
Expand All @@ -44,7 +44,7 @@ private static extern int AppleCryptoNative_X509ImportCertificate(
ref byte pbKeyBlob,
int cbKeyBlob,
X509ContentType contentType,
SafeCreateHandle? cfPfxPassphrase,
SafeCreateHandle cfPfxPassphrase,
SafeKeychainHandle tmpKeychain,
int exportable,
out SafeSecCertificateHandle pCertOut,
Expand All @@ -56,7 +56,7 @@ private static extern int AppleCryptoNative_X509ImportCollection(
ref byte pbKeyBlob,
int cbKeyBlob,
X509ContentType contentType,
SafeCreateHandle? cfPfxPassphrase,
SafeCreateHandle cfPfxPassphrase,
SafeKeychainHandle tmpKeychain,
int exportable,
out SafeCFArrayHandle pCollectionOut,
Expand Down Expand Up @@ -97,7 +97,7 @@ private static extern int AppleCryptoNative_X509DemuxAndRetainHandle(
private static extern int AppleCryptoNative_X509ExportData(
SafeCreateHandle data,
X509ContentType type,
SafeCreateHandle? cfExportPassphrase,
SafeCreateHandle cfExportPassphrase,
out SafeCFDataHandle pExportOut,
out int pOSStatus);

Expand Down Expand Up @@ -190,10 +190,12 @@ private static SafeSecCertificateHandle X509ImportCertificate(
SafeSecCertificateHandle certHandle;
int osStatus;

SafeCreateHandle cfPassphrase = importPassword ?? s_nullExportString;

int ret = AppleCryptoNative_X509ImportCertificate(
bytes,
contentType,
importPassword,
cfPassphrase,
keychain,
exportable ? 1 : 0,
out certHandle,
Expand Down Expand Up @@ -235,7 +237,7 @@ internal static SafeCFArrayHandle X509ImportCollection(
SafeKeychainHandle keychain,
bool exportable)
{
SafeCreateHandle? cfPassphrase = null;
SafeCreateHandle cfPassphrase = s_nullExportString;
bool releasePassword = false;

int ret;
Expand Down Expand Up @@ -277,7 +279,10 @@ ref MemoryMarshal.GetReference(bytes),
importPassword.DangerousRelease();
}

cfPassphrase?.Dispose();
if (cfPassphrase != s_nullExportString)
{
cfPassphrase.Dispose();
}
}

collectionHandle.Dispose();
Expand Down Expand Up @@ -471,7 +476,7 @@ internal static SafeSecIdentityHandle X509CopyWithPrivateKey(
return null;
}

private static byte[] X509Export(X509ContentType contentType, SafeCreateHandle? cfPassphrase, IntPtr[] certHandles)
private static byte[] X509Export(X509ContentType contentType, SafeCreateHandle cfPassphrase, IntPtr[] certHandles)
{
Debug.Assert(contentType == X509ContentType.Pkcs7 || contentType == X509ContentType.Pkcs12);

Expand Down Expand Up @@ -508,7 +513,7 @@ private static byte[] X509Export(X509ContentType contentType, SafeCreateHandle?

internal static byte[] X509ExportPkcs7(IntPtr[] certHandles)
{
return X509Export(X509ContentType.Pkcs7, null, certHandles);
return X509Export(X509ContentType.Pkcs7, s_nullExportString, certHandles);
}

internal static byte[] X509ExportPfx(IntPtr[] certHandles, SafePasswordHandle exportPassword)
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/Common/src/Interop/Windows/WinSock/Interop.TransmitFile.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ internal static partial class Mswsock
[DllImport(Interop.Libraries.Mswsock, SetLastError = true)]
internal static extern unsafe bool TransmitFile(
SafeHandle socket,
SafeHandle? fileHandle,
IntPtr fileHandle,
int numberOfBytesToWrite,
int numberOfBytesPerSend,
NativeOverlapped* overlapped,
Expand Down
24 changes: 21 additions & 3 deletions src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1052,9 +1052,27 @@ private static unsafe bool TransmitFileHelper(
transmitFileBuffers.TailLength = postBufferLength;
}

return Interop.Mswsock.TransmitFile(
socket, fileHandle, 0, 0, overlapped,
needTransmitFileBuffers ? &transmitFileBuffers : null, flags);
bool releaseRef = false;
IntPtr fileHandlePtr = IntPtr.Zero;
try
{
if (fileHandle != null)
{
fileHandle.DangerousAddRef(ref releaseRef);
fileHandlePtr = fileHandle.DangerousGetHandle();
}

return Interop.Mswsock.TransmitFile(
socket, fileHandlePtr, 0, 0, overlapped,
needTransmitFileBuffers ? &transmitFileBuffers : null, flags);
}
finally
{
if (releaseRef)
{
fileHandle!.DangerousRelease();
}
}
}

public static unsafe SocketError SendFileAsync(SafeSocketHandle handle, FileStream? fileStream, byte[]? preBuffer, byte[]? postBuffer, TransmitFileOptions flags, TransmitFileAsyncResult asyncResult)
Expand Down
3 changes: 3 additions & 0 deletions src/libraries/System.Private.CoreLib/src/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1585,6 +1585,9 @@
<data name="ArgumentNull_Path" xml:space="preserve">
<value>Path cannot be null.</value>
</data>
<data name="ArgumentNull_SafeHandle" xml:space="preserve">
<value>SafeHandle cannot be null.</value>
</data>
<data name="ArgumentNull_Stream" xml:space="preserve">
<value>Stream cannot be null.</value>
</data>
Expand Down
3 changes: 3 additions & 0 deletions src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -831,6 +831,8 @@ private static string GetResourceString(ExceptionResource resource)
return SR.ArgumentException_OtherNotArrayOfCorrectLength;
case ExceptionResource.ArgumentNull_Array:
return SR.ArgumentNull_Array;
case ExceptionResource.ArgumentNull_SafeHandle:
return SR.ArgumentNull_SafeHandle;
case ExceptionResource.ArgumentOutOfRange_EndIndexStartIndex:
return SR.ArgumentOutOfRange_EndIndexStartIndex;
case ExceptionResource.ArgumentOutOfRange_Enum:
Expand Down Expand Up @@ -1027,6 +1029,7 @@ internal enum ExceptionResource
Task_WaitMulti_NullTask,
ArgumentException_OtherNotArrayOfCorrectLength,
ArgumentNull_Array,
ArgumentNull_SafeHandle,
ArgumentOutOfRange_EndIndexStartIndex,
ArgumentOutOfRange_Enum,
ArgumentOutOfRange_HugeArrayNotSupported,
Expand Down
35 changes: 13 additions & 22 deletions src/mono/mono/metadata/marshal-ilgen.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5343,35 +5343,34 @@ emit_marshal_safehandle_ilgen (EmitMarshalContext *m, int argnum, MonoType *t,

switch (action){
case MARSHAL_ACTION_CONV_IN: {
int dar_release_slot, pos_done;
int dar_release_slot, pos;

conv_arg = mono_mb_add_local (mb, int_type);
*conv_arg_type = int_type;

if (!sh_dangerous_add_ref)
init_safe_handle ();

mono_mb_emit_ldarg (mb, argnum);
pos = mono_mb_emit_branch (mb, CEE_BRTRUE);
mono_mb_emit_exception (mb, "ArgumentNullException", NULL);

mono_mb_patch_branch (mb, pos);

/* Create local to hold the ref parameter to DangerousAddRef */
dar_release_slot = mono_mb_add_local (mb, boolean_type);

/* set release = false; */
mono_mb_emit_icon (mb, 0);
mono_mb_emit_stloc (mb, dar_release_slot);

/* set conv = IntPtr.Zero; */
mono_mb_emit_icon (mb, 0);
mono_mb_emit_byte (mb, CEE_CONV_I);
mono_mb_emit_stloc (mb, conv_arg);

if (t->byref) {
int old_handle_value_slot = mono_mb_add_local (mb, int_type);

if (is_in (t)) {
/* Load and check the byref SafeHandle */
mono_mb_emit_ldarg(mb, argnum);
mono_mb_emit_byte(mb, CEE_LDIND_REF);
pos_done = mono_mb_emit_branch(mb, CEE_BRFALSE);

if (!is_in (t)) {
mono_mb_emit_icon (mb, 0);
mono_mb_emit_stloc (mb, conv_arg);
} else {
/* safehandle.DangerousAddRef (ref release) */
mono_mb_emit_ldarg (mb, argnum);
mono_mb_emit_byte (mb, CEE_LDIND_REF);
Expand All @@ -5386,14 +5385,8 @@ emit_marshal_safehandle_ilgen (EmitMarshalContext *m, int argnum, MonoType *t,
mono_mb_emit_byte (mb, CEE_DUP);
mono_mb_emit_stloc (mb, conv_arg);
mono_mb_emit_stloc (mb, old_handle_value_slot);

mono_mb_patch_branch(mb, pos_done);
}
} else {
/* Load and check the SafeHandle */
mono_mb_emit_ldarg(mb, argnum);
pos_done = mono_mb_emit_branch(mb, CEE_BRFALSE);

/* safehandle.DangerousAddRef (ref release) */
mono_mb_emit_ldarg (mb, argnum);
mono_mb_emit_ldloc_addr (mb, dar_release_slot);
Expand All @@ -5404,8 +5397,6 @@ emit_marshal_safehandle_ilgen (EmitMarshalContext *m, int argnum, MonoType *t,
mono_mb_emit_ldflda (mb, MONO_STRUCT_OFFSET (MonoSafeHandle, handle));
mono_mb_emit_byte (mb, CEE_LDIND_I);
mono_mb_emit_stloc (mb, conv_arg);

mono_mb_patch_branch(mb, pos_done);
}

break;
Expand Down Expand Up @@ -5491,7 +5482,7 @@ emit_marshal_safehandle_ilgen (EmitMarshalContext *m, int argnum, MonoType *t,
}
break;
}

case MARSHAL_ACTION_CONV_RESULT: {
ERROR_DECL (error);
MonoMethod *ctor = NULL;
Expand Down Expand Up @@ -5525,7 +5516,7 @@ emit_marshal_safehandle_ilgen (EmitMarshalContext *m, int argnum, MonoType *t,
mono_mb_emit_byte (mb, CEE_STIND_I);
break;
}

case MARSHAL_ACTION_MANAGED_CONV_IN:
fprintf (stderr, "mono/marshal: SafeHandles missing MANAGED_CONV_IN\n");
break;
Expand Down
9 changes: 0 additions & 9 deletions src/tests/Interop/PInvoke/SafeHandles/SafeHandleTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,23 +16,14 @@ public static void RunTest()
{
var testHandle = new TestSafeHandle(initialValue);
Assert.IsTrue(SafeHandleNative.SafeHandleByValue(testHandle, initialValue));
Assert.IsTrue(SafeHandleNative.SafeHandleByValue(null, IntPtr.Zero));

Assert.IsTrue(SafeHandleNative.SafeHandleByRef(ref testHandle, initialValue, newValue));
Assert.AreEqual(newValue, testHandle.DangerousGetHandle());

testHandle = null;
Assert.IsTrue(SafeHandleNative.SafeHandleByRef(ref testHandle, IntPtr.Zero, newValue));
Assert.AreEqual(newValue, testHandle.DangerousGetHandle());

AbstractDerivedSafeHandle abstrHandle = new AbstractDerivedSafeHandleImplementation(initialValue);
Assert.IsTrue(SafeHandleNative.SafeHandleInByRef(abstrHandle, initialValue));
Assert.Throws<MarshalDirectiveException>(() => SafeHandleNative.SafeHandleByRef(ref abstrHandle, initialValue, newValue));

abstrHandle = null;
Assert.IsTrue(SafeHandleNative.SafeHandleInByRef(abstrHandle, IntPtr.Zero));
Assert.AreEqual(null, abstrHandle);

NoDefaultConstructorSafeHandle noDefaultCtorHandle = new NoDefaultConstructorSafeHandle(initialValue);
Assert.Throws<MissingMethodException>(() => SafeHandleNative.SafeHandleByRef(ref noDefaultCtorHandle, initialValue, newValue));

Expand Down