From 440260bd40b3134d3e8e9dc922609f98c73189da Mon Sep 17 00:00:00 2001 From: lateralusX Date: Fri, 7 Feb 2025 18:39:29 +0100 Subject: [PATCH] Fix init race in mono_class_try_get_[shortname]_class. We observed several crash reports in dotnet Android coming from google play store, like this one: https://github.com/dotnet/runtime/issues/109921#issuecomment-2640412870 and it happens at the following callstack: mono_class_setup_vtable_full at /__w/1/s/src/mono/mono/metadata/class-setup-vtable.c:900 init_io_stream_slots at /__w/1/s/src/mono/mono/metadata/icall.c:3378 ves_icall_System_IO_Stream_HasOverriddenBeginEndRead at /__w/1/s/src/mono/mono/metadata/icall.c:3437 (inlined by) ves_icall_System_IO_Stream_HasOverriddenBeginEndRead_raw at /__w/1/s/src/mono/mono/metadata/../metadata/icall-def.h:228 Looking a little deeper at that that code path expects call to mono_class_try_get_stream_class to succeed since it calls mono_class_setup_vtable on returned class pointer. There are other places where code expectes mono_class_try_get_[shortname]_class to always succeed or it will assert. Other locations handles the case where it returns NULL meaning that the class has been linked out. After looking into the macro implementation generating the mono_class_try_get_[shortname]_class functions it appears that it has a race where one thread could return NULL even if the class successfully gets loaded by another racing thread. Initially that might have been intentionally and callers would need to redo the call, but then there is no way for callers to distinct between hitting the race and class not available. Adding to the fact that code would also expect that some critical classes never fail loading, like what init_io_stream_slots does, this race ended up in race conditions. In the past, this particular race in init_io_stream_slots was hidden due to multiple calls to mono_class_try_get_stream_class minimzing the risk of a race to cause a crash. That implementation was however changed by https://github.com/dotnet/runtime/commit/e74da7c72c81787f8339df93cb6f13ac40b1e39d ending up in just one call to mono_class_try_get_stream_class in the critical paths. Since code relies on mono_class_try_get_[shortname]_class to succeed for some critical classes, code asserts if it returns NULL or crashes. The fix will use acquire/release semantics on the static variable guarding the cached type and make sure memory order is preserved on both read and write. Previous implementation adopted a similar approach but it took a copy of the static tmp_class into a local variable meaning that memory order would not be guaranteed, even if it applied memory barriers and volatile keywords on the static variables. Fix also adds an assert into init_io_stream_slots since it expects failures in calls to mono_class_try_get_stream_class as fatal. --- src/mono/mono/metadata/class-internals.h | 23 +++++++++-------------- src/mono/mono/metadata/icall.c | 2 ++ 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/src/mono/mono/metadata/class-internals.h b/src/mono/mono/metadata/class-internals.h index 5d804d1634b7bb..5cd05e50edf801 100644 --- a/src/mono/mono/metadata/class-internals.h +++ b/src/mono/mono/metadata/class-internals.h @@ -952,25 +952,20 @@ mono_class_get_##shortname##_class (void) \ // GENERATE_TRY_GET_CLASS_WITH_CACHE attempts mono_class_load_from_name approximately // only once. i.e. if it fails, it will return null and not retry. -// In a race it might try a few times, but not indefinitely. -// -// FIXME This maybe has excessive volatile/barriers. -// #define GENERATE_TRY_GET_CLASS_WITH_CACHE(shortname,name_space,name) \ MonoClass* \ mono_class_try_get_##shortname##_class (void) \ { \ - static volatile MonoClass *tmp_class; \ - static volatile gboolean inited; \ - MonoClass *klass = (MonoClass *)tmp_class; \ - mono_memory_barrier (); \ - if (!inited) { \ - klass = mono_class_try_load_from_name (mono_class_generate_get_corlib_impl (), name_space, name); \ - tmp_class = klass; \ - mono_memory_barrier (); \ - inited = TRUE; \ + static MonoClass *cached_class; \ + static gboolean cached_class_inited; \ + gboolean tmp_inited; \ + mono_atomic_load_acquire(tmp_inited, gboolean, &cached_class_inited); \ + if (G_LIKELY(tmp_inited)) { \ + return (MonoClass*)cached_class; \ } \ - return klass; \ + cached_class = mono_class_try_load_from_name (mono_class_generate_get_corlib_impl (), name_space, name); \ + mono_atomic_store_release(&cached_class_inited, TRUE); \ + return (MonoClass*)cached_class; \ } GENERATE_TRY_GET_CLASS_WITH_CACHE_DECL (safehandle) diff --git a/src/mono/mono/metadata/icall.c b/src/mono/mono/metadata/icall.c index 139a2d02dd2012..a5ba24a6883114 100644 --- a/src/mono/mono/metadata/icall.c +++ b/src/mono/mono/metadata/icall.c @@ -3425,6 +3425,8 @@ static void init_io_stream_slots (void) { MonoClass* klass = mono_class_try_get_stream_class (); + g_assert(klass); + mono_class_setup_vtable (klass); MonoMethod **klass_methods = m_class_get_methods (klass); if (!klass_methods) {