| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
We observed several crash reports in dotnet Android coming from google play store, like this one: #109921 (comment) 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 e74da7c 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.
There was a problem hiding this comment.
lgtm. please get a code review. we will take for consideration in 8.0.x
Sorry, something went wrong.
|
/ba-g BA was showing all known test errors. |
Sorry, something went wrong.
|
@leecow this change didn't get merged into 8.0.14 apparently due to the timing. Please mark it as 8.0.15 when you create the milestone. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Backport of #112282 to release/8.0
/cc @steveisok @lateralusX
Customer Impact
As reported in #109921 (comment), customers experienced a crash when trying to open a stream due to the runtime accessing a NULL class pointer. The reason for the NULL class pointer was due to a race where one thread could return NULL even if another thread successfully loaded it.
The fix uses acquire/release semantics on the static variable guarding the init of the cached class and makes sure memory order is preserved on both read and write.
Regression
This problem existed long ago, but a change we made back in .NET 7 made this more likely to happen.
Testing
Manual testing
Risk
Low
IMPORTANT: If this backport is for a servicing release, please verify that:
Package authoring no longer needed in .NET 9
IMPORTANT: Starting with .NET 9, you no longer need to edit a NuGet package's csproj to enable building and bump the version.
Keep in mind that we still need package authoring in .NET 8 and older versions.