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.

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

Jump to bottom

Disposing immutable objects can lead to unpredictable behavior when Objective-C runtime does object pooling #21425

Open
filipnavara opened this issue Oct 12, 2024 · 4 comments · May be fixed by #21446
Open

Disposing immutable objects can lead to unpredictable behavior when Objective-C runtime does object pooling #21425

filipnavara opened this issue Oct 12, 2024 · 4 comments · May be fixed by #21446
Labels
bug If an issue is a bug or a pull request a bug fix
Milestone
Future

Comments

@filipnavara
Copy link
Contributor

filipnavara commented Oct 12, 2024 •
edited
Loading

Consider the following code:

string notificationData =
	"""
	{
		action = "show_pushed_mail";
		aps =
		{
			alert =
			{
				body = "Compared Exchange Rates are out of tolerance in number of 115.";
				subtitle = "[email protected]";
				title = "eM Client Licensing";
			};
			category = "PUSHED_MAIL";
			"content-available" = 1;
			"mutable-content" = 1;
			sound = default;
			"thread-id" = "mail_cf73f3bd-1cf9-437c-bc70-4b6bd2b5f7e4";
		};
		"em-account" = "[email protected]";
		"em-account-id" = "cf73f3bd-1cf9-437c-bc70-4b6bd2b5f7e4";
		"em-body" = "";
		"em-date" = "2024-10-09T01:30:01.0000000Z";
		"em-from" = "eM Client Licensing";
		"em-from-address" = "[email protected]";
		"em-message-path" = "{\n \"Mailbox\": \"INBOX\",\n \"UIDVALIDITY\": \"1117940911\",\n \"UID\": \"317970\",\n \"Message-ID\": \"[email protected]\"\n}";
		"em-notification" = Mixed;
		"em-notification-id" = 5801afc0ceb977f379adc152333e0d25554a98d4;
		"em-subject" = "Compared Exchange Rates are out of tolerance in number of 115.";
		"gcm.message_id" = 1728437407395630;
		"google.c.a.e" = 1;
		"google.c.fid" = eEhP3hWX1U8DuSB9hb3siN;
		"google.c.sender.id" = 417058856903;
	}
	""";

var d = NSData.FromString(notificationData);
NSPropertyListFormat fmt = NSPropertyListFormat.OpenStep;
var userInfo = (NSMutableDictionary)NSPropertyListSerialization.PropertyListWithData(d, ref fmt, out var error);

var apsKey = new NSString("aps");
var data = userInfo
	.Where(kv =>
	{
		var r = !kv.Key.ToString().Equals("aps", StringComparison.OrdinalIgnoreCase);
		apsKey.Dispose();
		return r;
	})
	.ToDictionary(kv => kv.Key.ToString(), kv => kv.Value.ToString() ?? string.Empty);

It will reliably crash with ObjectDisposedException which may be quite unexpected. In fact, this is a very reduced example of a problem that was happening in a multi-threaded application where it was even less obvious.

Why does it crash?

  • We create the NSString object representing the string aps.
  • When Objective-C enumerates the dictionary keys it reuses the same object, ie. same handle, when enumerating the aps key, and that in turns maps to the same managed NSString object.
  • Calling Dispose on the NSString causes the managed representation to be invalid and next enumeration of the same key will crash with ObjectDisposedException.

What can we do about it?

Changing the behavior to remove disposed objects from the handle->managed object mapping seems dangerous. (would not help anyway in the original multi-threaded scenario)

Make the Dispose on immutable poolable classes like NSString a no-op? Make an analyzer that warns when someone tries to dispose a NSString instance (would flag the obvious error but not if someone disposes it as NSObject variable)?

Thoughts?
@rolfbjarne
Copy link
Member

This is a tricky problem, and we've run into different variations on it over the years.

Make the Dispose on immutable poolable classes like NSString a no-op? Make an analyzer that warns when someone tries to dispose a NSString instance (would flag the obvious error but not if someone disposes it as NSObject variable)?

The problem with this is that I can certainly envision someone wanting to dispose at least some NSString instances (they can be arbitrarily big, so you might want to dispose big ones).

One idea I just had would be to just skip disposing tagged pointers, but the main problem with this approach is that there's no reliable way to detect tagged pointers (Apple can change their implementation at any time). Just doing a string comparison on the Objective-C class (NSTaggedPointerString), but it's a bit scary to have correctness depend on this (if Apple changed this class's name, apps could randomly start throwing ObjectDisposedException...)

@rolfbjarne rolfbjarne added the bug If an issue is a bug or a pull request a bug fix label Oct 15, 2024
@rolfbjarne rolfbjarne added this to the Future milestone Oct 15, 2024
@filipnavara
Copy link
Contributor Author

filipnavara commented Oct 15, 2024 •
edited
Loading

Presumably, if this is affecting all tagged objects, we can differentiate by the lowest (x64) / highest (arm64) bit == 1 and disallow Dispose on those (disallow as in make it a no-op). I don't like the fact that it's dependency on internal implementation detail but it's probably just as reliable as checking for NSTaggedPointerString and it solves the same issue for NSNumber (and NSColor?).

I'm not sure if there are any other cases of reused objects aside from the tagged ones.

rolfbjarne added a commit that referenced this issue Oct 15, 2024
@rolfbjarne
[Foundation] Don't dispose tagged pointers. Fixes #21425.
6a51449 
TODO: better description.

Fixes #21425.
@rolfbjarne rolfbjarne linked a pull request Oct 15, 2024 that will close this issue
Draft
@rolfbjarne
Copy link
Member

Presumably, if this is affecting all tagged objects, we can differentiate by the lowest (x64) / highest (arm64) bit == 1 and disallow Dispose on those (disallow as in make it a no-op). I don't like the fact that it's dependency on internal implementation detail

That internal implementation detail changed in iOS 14: "Let's explore one more change coming this year. A change to the tagged pointer format on arm64." - https://developer.apple.com/videos/play/wwdc2020/10163/

Which means we'll have to detect (and not apply the tagged pointer logic) when running on iOS 13 or earlier (our min iOS is right now iOS 12.2 in .NET 9). Alternatively not enable it when an app's min OS version is <14.

@filipnavara
Copy link
Contributor Author

I think the part about the tagging logic that changed doesn't affect the "is tagged" bit, only the other ones which we do not rely on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug If an issue is a bug or a pull request a bug fix
Projects
None yet
Milestone
Future
Development

Successfully merging a pull request may close this issue.

[Foundation] Don't dispose tagged pointers. Fixes #21425. xamarin/xamarin-macios
2 participants
@rolfbjarne @filipnavara