Skip to content

Commit

Permalink
[Mono]: Fix static closed delegate fnptr crash. (#68701)
Browse files Browse the repository at this point in the history
* Fix static closed delegate fnptr crash.

When accessing a function pointer to a static closed delegate like
done in added test:

GetFunctionPointerForDelegate_MarshalledClosedStaticDelegate

it will trigger a read outside of the allocated mspecs buffer since
invoke_sig and method signature arguments wont match.

There was already logic to adjust this in emit_managed_wrapper_ilgen,
but done after call to emit_managed_wrapper_validate_signature
that will touch memory and depending on its content, trigger a crash.

Fix make sure we do signature adjustments first and then validate
the signature. Fix also adjust a couple of mspecs allocations to use
g_new0 as all others to make sure we get NULL pointers in the mspecs
array.

Since this scenario was not covered on CI, commit also adds a new test
in GetFunctionPointerForDelegateTests.cs covering this scenario.
  • Loading branch information
lateralusX committed May 3, 2022
1 parent c9d8f80 commit 288de11
Show file tree
Hide file tree
Showing 4 changed files with 39 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,29 @@ public void GetFunctionPointer_GenericDelegate_ThrowsArgumentException()
AssertExtensions.Throws<ArgumentException>("delegate", () => Marshal.GetFunctionPointerForDelegate(d));
}

[Fact]
[SkipOnPlatform(TestPlatforms.Browser, "Not supported on Browser.")]
public void GetFunctionPointerForDelegate_MarshalledOpenStaticDelegate()
{
MethodInfo targetMethod = typeof(GetFunctionPointerForDelegateTests).GetMethod(nameof(Method), BindingFlags.NonPublic | BindingFlags.Static);
Delegate original = targetMethod.CreateDelegate(typeof(NonGenericDelegate), null);
IntPtr ptr = Marshal.GetFunctionPointerForDelegate(original);
Assert.NotEqual(IntPtr.Zero, ptr);
}

[Fact]
[SkipOnPlatform(TestPlatforms.Browser, "Not supported on Browser.")]
public void GetFunctionPointerForDelegate_MarshalledClosedStaticDelegate()
{
MethodInfo targetMethod = typeof(GetFunctionPointerForDelegateTests).GetMethod(nameof(Method), BindingFlags.NonPublic | BindingFlags.Static);
Delegate original = targetMethod.CreateDelegate(typeof(NoArgsDelegate), "value");
IntPtr ptr = Marshal.GetFunctionPointerForDelegate(original);
Assert.NotEqual(IntPtr.Zero, ptr);
}

public delegate void GenericDelegate<T>(T t);
public delegate void NonGenericDelegate(string t);
public delegate void NoArgsDelegate();

private static void Method(string s) { }
}
Expand Down
2 changes: 1 addition & 1 deletion src/mono/mono/metadata/cominterop.c
Original file line number Diff line number Diff line change
Expand Up @@ -2089,7 +2089,7 @@ cominterop_get_ccw_method (MonoClass *iface, MonoMethod *method, MonoError *erro
adjust_method = cominterop_get_managed_wrapper_adjusted (method);
sig_adjusted = mono_method_signature_internal (adjust_method);

mspecs = g_new (MonoMarshalSpec*, sig_adjusted->param_count + 1);
mspecs = g_new0 (MonoMarshalSpec*, sig_adjusted->param_count + 1);
mono_method_get_marshal_info (method, mspecs);

/* move managed args up one */
Expand Down
29 changes: 16 additions & 13 deletions src/mono/mono/metadata/marshal-ilgen.c
Original file line number Diff line number Diff line change
Expand Up @@ -743,7 +743,7 @@ emit_native_wrapper_validate_signature (MonoMethodBuilder *mb, MonoMethodSignatu
if (mspecs) {
for (int i = 0; i < sig->param_count; i ++) {
if (mspecs [i + 1] && mspecs [i + 1]->native == MONO_NATIVE_CUSTOM) {
if (!mspecs [i + 1]->data.custom_data.custom_name || strlen (mspecs [i + 1]->data.custom_data.custom_name) == 0) {
if (!mspecs [i + 1]->data.custom_data.custom_name || *mspecs [i + 1]->data.custom_data.custom_name == '\0') {
mono_mb_emit_exception_full (mb, "System", "TypeLoadException", g_strdup ("Missing ICustomMarshaler type"));
return FALSE;
}
Expand Down Expand Up @@ -5023,7 +5023,7 @@ emit_managed_wrapper_validate_signature (MonoMethodSignature* sig, MonoMarshalSp
if (mspecs) {
for (int i = 0; i < sig->param_count; i ++) {
if (mspecs [i + 1] && mspecs [i + 1]->native == MONO_NATIVE_CUSTOM) {
if (!mspecs [i + 1]->data.custom_data.custom_name || strlen (mspecs [i + 1]->data.custom_data.custom_name) == 0) {
if (!mspecs [i + 1]->data.custom_data.custom_name || *mspecs [i + 1]->data.custom_data.custom_name == '\0') {
mono_error_set_generic_error (error, "System", "TypeLoadException", "Missing ICustomMarshaler type");
return FALSE;
}
Expand Down Expand Up @@ -5067,8 +5067,21 @@ emit_managed_wrapper_ilgen (MonoMethodBuilder *mb, MonoMethodSignature *invoke_s
sig = m->sig;
csig = m->csig;

if (!emit_managed_wrapper_validate_signature (sig, mspecs, error))
if (!sig->hasthis && sig->param_count != invoke_sig->param_count) {
/* Closed delegate */
g_assert (sig->param_count == invoke_sig->param_count + 1);
closed = TRUE;
/* Use a new signature without the first argument */
sig = mono_metadata_signature_dup (sig);
memmove (&sig->params [0], &sig->params [1], (sig->param_count - 1) * sizeof (MonoType*));
sig->param_count --;
}

if (!emit_managed_wrapper_validate_signature (sig, mspecs, error)) {
if (closed)
g_free (sig);
return;
}

MonoType *int_type = mono_get_int_type ();
MonoType *boolean_type = m_class_get_byval_arg (mono_defaults.boolean_class);
Expand All @@ -5079,16 +5092,6 @@ emit_managed_wrapper_ilgen (MonoMethodBuilder *mb, MonoMethodSignature *invoke_s
/* allocate local 2 (boolean) delete_old */
mono_mb_add_local (mb, boolean_type);

if (!sig->hasthis && sig->param_count != invoke_sig->param_count) {
/* Closed delegate */
g_assert (sig->param_count == invoke_sig->param_count + 1);
closed = TRUE;
/* Use a new signature without the first argument */
sig = mono_metadata_signature_dup (sig);
memmove (&sig->params [0], &sig->params [1], (sig->param_count - 1) * sizeof (MonoType*));
sig->param_count --;
}

if (!MONO_TYPE_IS_VOID(sig->ret)) {
/* allocate local 3 to store the return value */
mono_mb_add_local (mb, sig->ret);
Expand Down
2 changes: 1 addition & 1 deletion src/mono/mono/metadata/marshal.c
Original file line number Diff line number Diff line change
Expand Up @@ -3561,7 +3561,7 @@ mono_marshal_get_native_wrapper (MonoMethod *method, gboolean check_exceptions,
csig = mono_metadata_signature_dup_full (get_method_image (method), sig);
mono_marshal_set_callconv_from_modopt (method, csig, FALSE);

mspecs = g_new (MonoMarshalSpec*, sig->param_count + 1);
mspecs = g_new0 (MonoMarshalSpec*, sig->param_count + 1);
mono_method_get_marshal_info (method, mspecs);

if (mono_class_try_get_suppress_gc_transition_attribute_class ()) {
Expand Down

0 comments on commit 288de11

Please sign in to comment.