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

Latent type confusion bugs in .NET8/9 Mono #112395

Open
kg opened this issue Feb 11, 2025 · 0 comments
Open

Latent type confusion bugs in .NET8/9 Mono #112395

kg opened this issue Feb 11, 2025 · 0 comments

Comments

@kg
Copy link
Member

kg commented Feb 11, 2025

During the development of #111645 I found various type confusion issues in our existing Mono source code. Backporting the fixes for these issues is not necessarily worth it since the resulting behavior change could expose new bugs or change application behavior, but it's worth tracking all of the latent issues here in case we need to fix them later to address customer-facing issues.

There are probably more errors of this type that I didn't find.


if ((m_class_get_element_class (type->data.klass) == mono_defaults.char_class) && !unicode)

type is either ARRAY or SZARRAY; the two types have different internal representations so the access to klass here is incorrect.


mono_unichar2 *utf16_array = g_utf8_to_utf16 ((const char *)ptr, arr->max_length, NULL, NULL, NULL);

utf16_array can be NULL and that scenario is not handled. The current implementation could potentially erroneously mess with the heap in the zero page on WASM; on other targets it should fail-fast.


/* Fall through */

/* Fall through */

/* fall through */

The fallthrough from GENERICINST to VALUETYPE results in various accesses to klass being incorrect. Note that there are many functions with the same error.


if (m_class_is_valuetype (t->data.klass)) {

The use of klass on a GENERICINST is incorrect.


(sig->ret->type == MONO_TYPE_CLASS && !strcmp (m_class_get_name (sig->ret->data.generic_class->container_class), "Task")) ||

This copy-paste error treats as CLASS as a GENERICINST and as a result the comparison against Task will probably never succeed and could crash instead.


MonoType blob_type;

MonoType blob_type;

These stack-allocated MonoTypes are not fully initialized which means the additional data in the type is random garbage from the stack.


blob_type.data.klass = mono_class_from_mono_type_internal (&blob_type);

This call to mono_class_from_mono_type_internal is being passed &blob_type which does not match the pattern from icall.c or from its neighboring calls, so it's possibly (but not certainly) an error.



Fallthrough for ARRAY and SZARRAY is incorrect since they have different internal representations.

@kg kg added this to the 10.0.0 milestone Feb 11, 2025
kg added a commit that referenced this issue Feb 11, 2025
* Guard members of MonoType union behind type check helpers
* Add _unchecked to call sites that are obviously guarded correctly
* Fix type confusion in bulk_type_log_single_type
* Fix genericinst fallthrough treating a MonoGenericClass ptr as a MonoClass ptr
* Fix type confusion in amd64 mini
* Fix type confusion in aot-runtime-wasm
* Prune a dead goto to make unchecked safe
* Fix two cases where we were partially initializing a stack-allocated MonoType instead of fully initializing it
* See #112395 for detailed list of bugs fixed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant