Guard members of MonoType union & fix related bugs #111645

kg · 2025-01-20T23:47:04Z

There is a class of issue where we fail to properly check the type of a MonoType before accessing the members of its data union, which can lead to type confusion or null pointer dereference bugs.

This PR adds dedicated access helpers for every member of the union that do a type check, renames the union to make direct accesses obvious, and removes almost all direct uses of the union members.

The process of adding the access helpers and auditing related code revealed multiple bugs; I've either fixed those bugs in this PR or added FIXME comments describing the bug.

For accesses that were obviously correct (I could see a type check right above the accessor call) I used the _unchecked variant to optimize out the type check.

As a follow-up later on in the .NET 10 cycle we can remove the type check from these accessors to make them more efficient if we need to. I don't think we'll need to, though.

kg · 2025-01-21T03:18:12Z

tracing\eventpipe\gcdump\gcdump\gcdump.cmd test failure is a real bug in mono.

Cleanup

lateralusX · 2025-01-22T12:10:19Z

src/mono/mono/metadata/metadata-internals.h

@@ -1296,6 +1296,92 @@ m_type_is_byref (const MonoType *type)
 	return type->byref__;
 }

+MONO_NEVER_INLINE void


If we are going to have this in production than I believe we should adopt a similar pattern as we have when using MONO_CLASS_DEF_PRIVATE, either extend that or make a similar thing for MonoType, that way we would run all these additional checks when running ENABLE_CHECKED_BUILD_PRIVATE_TYPES (we should have CI legs doing that), but use inline wrappers with zero overhead on release builds.

I would prefer to do actual full checks in Release unless benchmarks show that it meaningfully regresses performance, at least during the preview period, in order to flush out any remaining issues of this type

I would still add the same support for types as we have for class so we could do a MONO_TYPE_DEF_PRIVATE under ENABLED_CHECKED_BUILD_PRIVATE_TYPES to catch errors bypassing the getters on CI. We could also add support for the full check vs zero overhead wrappers as part of that and then just decide to initially opt into full checks during preview (and monitor perf hit during that time) and then have the option to move back to zero wrappers closer to release, but keep the full checks on CI builds running with the ENABLED_CHECKED_BUILD_PRIVATE_TYPES.

lateralusX · 2025-01-22T12:50:15Z

src/mono/mono/eventpipe/ep-rt-mono-runtime-provider.c

 	case MONO_TYPE_SZARRAY:
+	{
+		// FIXME: Previously was handled by below ARRAY block but that is incorrect; this implementation is speculative -kg


I assume you should be able to handle both cases similar, except that you can't assume mono_type is MonoArrayType it could also be MonoClass, so you would just need to handle the case where you get the type, rank and mono_type_parameter differently.

Add comments above some apparent bugs

kg · 2025-01-22T23:53:29Z

I went through and audited all the union accesses I could find and changed the ones that were blatantly and correctly guarded to _unchecked which should remove a lot of overhead while also giving us a way to add debug/checked guards if we want them for some reason later on.

I found some more bugs (mostly around SZARRAY handling) and added FIXME comments if I couldn't trivially fix them. I will probably try to fix all the FIXMEs I've found so far.

…Class ptr

kg · 2025-01-24T17:30:27Z

The AOT compiler failure isn't generating usable stacks for some reason so I'll have to try and reproduce it locally in Debug configuration and hopefully get stacks there. I'm not sure if the aot compiler lacking stack traces when it crashes is intentional, I'm surprised by it.

EDIT: Lesson learned: If you fix a bug in mono_allocate_stack_slots, the function mono_allocate_stack_slots2 probably has the same bug and you should fix it there too

…Class ptr, again

kg · 2025-01-25T00:54:24Z

Build linux-x64 Release AllSubsets_Mono_LLVMFULLAOT_RuntimeTests llvmfullaot and its sibling look hung, but maybe they're just normally slow, or they're very slow because of the changes in this PR? I'm not familiar with these lanes and I can't tell whether the stdout on both of them stops at 1999 by coincidence or because azdo is truncating the actual output.

Prune a dead goto to make unchecked safe

kg · 2025-01-28T02:12:42Z

I took a middle ground between @lateralusX 's suggestion and my personal preference, by renaming the union but not having a configuration where direct union access is allowed. Having two modes would have complicated the code a lot so I decided not to do it yet. If we decide we need direct access for some reason in a wider set of code I can do the work to enable it, but I think it's better to enforce use of the getters and setters everywhere.

Most uses of the getters ended up being unchecked because they were obviously guarded by a switch case or an if, so I expect the performance impact to be minor in practice. If this leads to unacceptable performance regressions my plan is to just disable the checks by default in release builds as we approach the later previews.

My audit revealed a handful of places where it wasn't obvious whether the union was being used safely beforehand, but also wasn't immediately obvious that it was being used incorrectly. As a result I couldn't "fix" them easily and left them guarded, so customers may hit failures in their applications if those locations turn out to be bugs. i.e. a function with a name indicating it's used to manipulate generic parameters that isn't doing a type check before accessing the union as if it's a generic parameter.

There's one bare use of the raw union still left in the code because the union member was being passed to some sort of macro/function as an lvalue, and I couldn't come up with a straightforward fix for that.

Something I'd love to do is have even the _unchecked accessors do checks in debug builds, just in case my audits were incorrect and I applied _unchecked somewhere I shouldn't have. But I don't know if it's worth the trouble, so I didn't bother doing it. Would love to hear people's thoughts on that.

lateralusX

Gone through all changes, mainly commenting on more places that could potentially use unchecked instead of checked gets since we already have checks of type before making calls to the types get function.

At first, I was a little reluctant to split into checked/unchecked, because it could introduce bugs by using the unchecked in wrong places or refactoring that might falsify the use of unchecked in a piece of code. Just having one implementation with full checks enabled in debug/checked build running on all our internal testing and then having a zero cost implementation in release would have mitigated this issue, but if we believe we would like to keep some of the checks in release builds, then we would need to split into two functions like you have in this PR, but then I believe it will be crucial to make the unchecked version working as the checked version in debug/checked builds so we can capture errors using the unchecked version udner false assumptions.

lateralusX · 2025-01-30T13:00:22Z