Skip to content
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.

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

Incorrect handling of nil blocks in Managed Static Registrar #20920

Closed
filipnavara opened this issue Jul 18, 2024 · 5 comments · Fixed by #20999
Closed

Incorrect handling of nil blocks in Managed Static Registrar #20920

filipnavara opened this issue Jul 18, 2024 · 5 comments · Fixed by #20999
Assignees
Labels
bug If an issue is a bug or a pull request a bug fix
Milestone

Comments

@filipnavara
Copy link
Contributor

We started experimenting with MSR in release builds of our app and we can reproducibly hit this on device when going through a certain UI path:

System.ArgumentNullException: ArgumentNull_Generic Arg_ParamName_Name, delegate
  ?, in Delegate Runtime.ReleaseBlockWhenDelegateIsCollected(IntPtr, Delegate)
  ?, in void __Registrar_Callbacks__.callback_125_Microsoft_Maui_Controls_Platform_ControlsModalWrapper_DismissViewController(IntPtr pobj, IntPtr sel, byte p0, IntPtr p1, IntPtr* exception_gchandle)

The code comes from MAUI:
https://github.com/dotnet/maui/blob/515c8986efefc83a7b21b30f32a8d44dd9748959/src/Controls/src/Core/Platform/iOS/ControlsModalWrapper.cs#L88-L91

		public override void DismissViewController(bool animated, Action? completionHandler)
		{
			base.DismissViewController(animated, completionHandler);
		}

It makes me wonder what happens if DismissViewController is called with completionHandler == null and how would that get marshalled between ObjC and C#.

@filipnavara
Copy link
Contributor Author

Repro: https://github.com/filipnavara/ios-msr-menu-repro

Click anywhere on the main screen, it opens menu. Dismiss the menu by clicking outside it => crash.

@filipnavara
Copy link
Contributor Author

Affects both NativeAOT and MonoVM.

@filipnavara
Copy link
Contributor Author

Presumably the fix is just to flip the order of these two conditions:

if (@delegate is null)
ObjCRuntime.ThrowHelper.ThrowArgumentNullException (nameof (@delegate));
if (block == IntPtr.Zero)
return @delegate;

The MSR generates this code:

[UnmanagedCallersOnly(EntryPoint = "callback_0_menu_repro_AppDelegate_ViewController_DismissViewController")]
public unsafe static void callback_0_menu_repro_AppDelegate_ViewController_DismissViewController(nint pobj, nint sel, byte p0, nint p1, nint* exception_gchandle)
{
	try
	{
		ViewController nSObject = Runtime.GetNSObject<ViewController>(pobj, sel, (RuntimeMethodHandle)/*OpCode not supported: LdMemberToken*/, true);
		nint num = BlockLiteral.Copy(p1);
		nSObject.DismissViewController(p0 != 0, (Action)Runtime.ReleaseBlockWhenDelegateIsCollected(num, Trampolines.NIDAction.Create(num)));
	}
	catch (Exception ex)
	{
		*exception_gchandle = Runtime.AllocGCHandle(ex);
	}
}

The trampolines were always created with the Create method returning null output for null input:

[UnmanagedCallersOnly(EntryPoint = "callback_0_menu_repro_AppDelegate_ViewController_DismissViewController")]
public unsafe static void callback_0_menu_repro_AppDelegate_ViewController_DismissViewController(nint pobj, nint sel, byte p0, nint p1, nint* exception_gchandle)
{
	try
	{
		ViewController nSObject = Runtime.GetNSObject<ViewController>(pobj, sel, (RuntimeMethodHandle)/*OpCode not supported: LdMemberToken*/, true);
		nint num = BlockLiteral.Copy(p1);
		nSObject.DismissViewController(p0 != 0, (Action)Runtime.ReleaseBlockWhenDelegateIsCollected(num, Trampolines.NIDAction.Create(num)));
	}
	catch (Exception ex)
	{
		*exception_gchandle = Runtime.AllocGCHandle(ex);
	}
}

@filipnavara
Copy link
Contributor Author

Since this is an absolute blocker for us I created a custom ILLink step to patch the code: https://github.com/emclient/MailClient.Linker

@filipnavara filipnavara changed the title Possible issue with Managed Static Registrar Incorrect handling of nil blocks in Managed Static Registrar Jul 20, 2024
@ivanpovazan ivanpovazan added this to the .NET 9 milestone Aug 1, 2024
@ivanpovazan ivanpovazan added the bug If an issue is a bug or a pull request a bug fix label Aug 1, 2024
@ivanpovazan
Copy link
Contributor

/cc: @rolfbjarne

@rolfbjarne rolfbjarne self-assigned this Aug 8, 2024
rolfbjarne added a commit that referenced this issue Aug 8, 2024
…teIsCollected. Fixes #20920.

Allow a null delegate in Runtime.ReleaseBlockWhenDelegateIsCollected if the
corresponding native pointer is also null.

Fixes #20920.
@github-project-automation github-project-automation bot moved this to Done in .NET 9 Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug If an issue is a bug or a pull request a bug fix
Projects
Status: Done
3 participants