Stop relying on UB in 'IContextCallback' dispatch logic

[Sergio0694]

See https://github.com/dotnet/runtime/blob/main/docs/design/specs/Memory-model.md#cross-thread-access-to-local-variables. This PR fixes accessing the callback state (which is a managed object) from the native callback invoked on the target thread.

[jkotas]

Fix a GC hole

Nit: The potential undefined behavior that this is fixing does not fit the definition of GC hole (https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/clr-code-guide.md#211-how-gc-holes-are-created).

[jkotas]

Are you sure that there cannot ever be recursion that would cause multiple recursive uses of CallbackData on the same thread?

Calls between STA apartments always come with a lot of surprises and corner-case bugs...

[Sergio0694]

Applied the suggested changes and also left some notes about reentrancy. I talked with @manodasanW as well and in this scenario this should be fine. We're only ever doing this context switch to release an object reference on its original context, and nothing else. There is no way that should/could then somehow switch back to this starting thread (it wouldn't even know which one that was) to then try to schedule another context callback recursively. So this should be fine. Also in ASTA scenarios this would also just be flat out impossible by construction (because ASTA blocks reentrant calls into it, so even if hypothetically we somehow ended up there, that second context callback would just throw before being scheduled). But then again either case should just never happen. Definitely a good point though and worth writing some additional comments about this, thank you! 😄

[jkotas]

Release can run arbitrary code, is that right? I do not see what guarantees that the arbitrary code cannot switch threads at will.

Also, this seems to be used for more than just releasing the object reference:

CsWinRT/src/WinRT.Runtime/ObjectReference.cs

Lines 751 to 760 in 93482af

	Context.CallInContext(
	_contextCallbackPtr,
	_contextToken,
	#if NET && CsWinRT_LANG_11_FEATURES
	&InitAgileReference,
	#else
	InitAgileReference,
	#endif
	null,
	this);

It would be more correct by construction to implement this as usual pool - clear the cached instance when it is rented, so that there is no way for one instance to be rented multiple times at the same time.

[hamarb123]

I would have assumed that IContextCallback::ContextCallback with ICallbackWithNoReentrancyToApplicationSTA means it shouldn't be able to run any arbitrary code on this thread as a result of that call (I'm not familiar with these APIs though, so I don't actually know), and nothing else from when we set the field to when we clear it seems problematic. If that thing doesn't guarantee this however, then something should be done to allow multiple current things at once potentially existing (e.g., using a List<object> or similar (probably manually via array for resizing purposes) would do the job).

[hamarb123]

Or do you mean the callback might call back into this thread & somehow end up back here? That would make sense I suppose. Is that possible @Sergio0694? Actually, I think in this case it would still be fine, as long as the callback is called before anything else silly happens, since it immediately reads the field & thus wouldn't get the new field value (assuming appropriate barrier or whatever - for which a volatile read would certainly be enough, but probably none is "needed"); then it would just be set to null twice, but the meaningful value of the field would already be read & that version would have already been read & had the correct value.

[Sergio0694]

"as long as the callback is called before anything else silly happens, since it immediately reads the field

That's actually a good point. Even if we somehow recursively ended up here (which I don't believe is possible), each callback would've already read the target state before invoking the user-provided callback anyway.

"Release can run arbitrary code"

Not really, I mean yes from the point of view of this API, but the context callback is only ever used internally to release IObjectReference objects, which are implemented in CsWinRT only. And we're only ever using these to pass the Release pointers which simply do a release on the tracker ref and the native object on the target context:

CsWinRT/src/WinRT.Runtime/ObjectReference.cs

Lines 955 to 967 in 93482af

	static void Release(object state)
	{
	ObjectReferenceWithContext<T> @this = Unsafe.As<ObjectReferenceWithContext<T>>(state);
	@this.ReleaseFromBase();
	}
	static void ReleaseWithoutContext(object state)
	{
	ObjectReferenceWithContext<T> @this = Unsafe.As<ObjectReferenceWithContext<T>>(state);
	@this.ReleaseWithoutContext();
	}

[hamarb123]

This looks fine now (assuming we don't need volatile read in the callback), assuming it's not possible to come back into ContextCallback while another one is executing on the same thread.

[Sergio0694]

Closing this for now, since the current behavior is technically not a GC hole and we're not actually sure the new code is safe (or even saf_er_) either, since technically once we wait for the callback COM might very well start pumping arbitrary messages on our thread too, in theory. We can revisit this for 3.0 if we notice this is causing any problems or find a better way.

[hamarb123]

Yeah, it's not a gc hole, but it is UB which iirc can produce problematic codegen in real cases. Ito handling potentially pumping arbitrary messages, you could use the static field if it's null & fallback to just using a normal array otherwise & pin it for the duration of the time (i.e., take the current logic, but add extra handling for "can use static field" based on if it's null, and just allocate & use a new object[1] if it's not null instead of the static field).

[Sergio0694]

I'll port this to the 3.0 branch and first run it with some assertions to see if we ever hit that case. I could also make a build with an assertion for 2.x and use it in the Store to get a better real world idea, I wonder how common it is. As in, if it never hits in our tests nor in the Store, then perhaps the fact the fallback is a bit slower wouldn't matter.

I do have another question though: is there no way to just keep using the current code structure (i.e. with the value being in a local) and just make that safe? If the issue is just that writing that to a local doesn't trigger the GC memory barrier, is there a way to just force it? Then after that once you have a pointer, it shouldn't matter where the location is, no?

[hamarb123]

If the issue is just that writing that to a local doesn't trigger the GC memory barrier, is there a way to just force it?

The issue is that it's UB, which means today it might do one thing, and tomorrow a new optimization could mean it does something else. You could probably make it "work", just like how people can make Unsafe.As<IncorrectType>(...) "work" in some cases, but it would still be UB to rely on it doing what you think it does & could break at any point in theory (although some setups are obviously less likely to be able to break than others).

The obvious way around this is to just make it not UB & actually allow it (and get the runtime to handle it properly) for .NET 11/12+, but that's ofc not up to me lol (but I would like to see it personally).