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

Attach unattached threads before querying for the current domain #6223

Merged
merged 1 commit into from
Aug 26, 2021

Conversation

grendello
Copy link
Contributor

Fixes: #6211

Mono VM will return a valid AppDomain pointer (both in "legacy" and NET6
cases) only if the current thread is attached to some domain. It is
possible that when managed code is called from an unattached Java
thread, mono_domain_get() will return a NULL pointer instead of a
valid one. If we pass this pointer along to Mono, a segfault may occur
if Mono fails to validate the passed pointer. Sample code which may
trigger the problem:

public override void OnCreate()
{
   AppDomain.CurrentDomain.UnhandledException += (sender, e) =>
   {
       Console.WriteLine("!!! UnhandledException: " + e.ExceptionObject.GetType().FullName);
   };
   base.OnCreate();
}

protected override void OnStart()
{
    base.OnStart();
    Java.Util.Concurrent.Executors.NewSingleThreadExecutor()
        .Execute(new Runnable(() =>
        {
            throw new Exception("Exception from Java Executor!");
        }));
}

Possible crashes caused by the above code:

08-20 15:00:20.619 19214 19241 F libc    : Fatal signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0xb8 in tid 19241 (pool-1-thread-1), pid 19214 (ndroidcrashtest)
08-20 15:00:20.786 19245 19245 F DEBUG   : backtrace:
08-20 15:00:20.786  1648  1648 I hwservicemanager: getTransport: Cannot find entry android.hardware.graphics.mapper@3.0::IMapper/default in either framework or device manifest.
08-20 15:00:20.786 19245 19245 F DEBUG   :       #00 pc 0011b72f  /apex/com.android.runtime/lib/bionic/libc.so (pthread_mutex_lock+31) (BuildId: 471745f0fbbcedb3db1553d5bd6fcd8b)
08-20 15:00:20.786 19245 19245 F DEBUG   :       #01 pc 0015240d  /data/app/com.companyname.androidcrashtest-sqgk-Mnu8GZM5hKPu1yWEQ==/lib/x86/libmonosgen-2.0.so (mono_domain_assembly_search+61)

08-20 15:22:04.462 19737 19763 F libc    : Fatal signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0xb8 in tid 19763 (pool-1-thread-1), pid 19737 (ndroidcrashtest)
08-20 15:22:04.599 19767 19767 F DEBUG   : backtrace:
08-20 15:22:04.599 19767 19767 F DEBUG   :       #00 pc 0011b72f  /apex/com.android.runtime/lib/bionic/libc.so (pthread_mutex_lock+31) (BuildId: 471745f0fbbcedb3db1553d5bd6fcd8b)
08-20 15:22:04.599 19767 19767 F DEBUG   :       #01 pc 0026b91a  /data/app/com.companyname.androidcrashtest-JQoYc3YFwZtEoSJlTqX_BA==/lib/x86/libmonosgen-2.0.so (mono_os_mutex_lock+42)

To avoid the above situation, wrap the mono_domain_get() call in a
method which checks the return value and, if it's nullptr, it calls
mono_get_root_domain() and attaches the current thread to that domain.

Fixes: dotnet#6211

Mono VM will return a valid AppDomain pointer (both in "legacy" and NET6
cases) only if the current thread is attached to some domain.  It is
possible that when managed code is called from an unattached Java
thread, `mono_domain_get()` will return a `NULL` pointer instead of a
valid one.  If we pass this pointer along to Mono, a segfault may occur
if Mono fails to validate the passed pointer.  Sample code which may
trigger the problem:

```csharp
public override void OnCreate()
{
   AppDomain.CurrentDomain.UnhandledException += (sender, e) =>
   {
       Console.WriteLine("!!! UnhandledException: " + e.ExceptionObject.GetType().FullName);
   };
   base.OnCreate();
}

protected override void OnStart()
{
    base.OnStart();
    Java.Util.Concurrent.Executors.NewSingleThreadExecutor()
        .Execute(new Runnable(() =>
        {
            throw new Exception("Exception from Java Executor!");
        }));
}
```

Possible crashes caused by the above code:

    08-20 15:00:20.619 19214 19241 F libc    : Fatal signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0xb8 in tid 19241 (pool-1-thread-1), pid 19214 (ndroidcrashtest)
    08-20 15:00:20.786 19245 19245 F DEBUG   : backtrace:
    08-20 15:00:20.786  1648  1648 I hwservicemanager: getTransport: Cannot find entry android.hardware.graphics.mapper@3.0::IMapper/default in either framework or device manifest.
    08-20 15:00:20.786 19245 19245 F DEBUG   :       #00 pc 0011b72f  /apex/com.android.runtime/lib/bionic/libc.so (pthread_mutex_lock+31) (BuildId: 471745f0fbbcedb3db1553d5bd6fcd8b)
    08-20 15:00:20.786 19245 19245 F DEBUG   :       #1 pc 0015240d  /data/app/com.companyname.androidcrashtest-sqgk-Mnu8GZM5hKPu1yWEQ==/lib/x86/libmonosgen-2.0.so (mono_domain_assembly_search+61)

    08-20 15:22:04.462 19737 19763 F libc    : Fatal signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0xb8 in tid 19763 (pool-1-thread-1), pid 19737 (ndroidcrashtest)
    08-20 15:22:04.599 19767 19767 F DEBUG   : backtrace:
    08-20 15:22:04.599 19767 19767 F DEBUG   :       #00 pc 0011b72f  /apex/com.android.runtime/lib/bionic/libc.so (pthread_mutex_lock+31) (BuildId: 471745f0fbbcedb3db1553d5bd6fcd8b)
    08-20 15:22:04.599 19767 19767 F DEBUG   :       #1 pc 0026b91a  /data/app/com.companyname.androidcrashtest-JQoYc3YFwZtEoSJlTqX_BA==/lib/x86/libmonosgen-2.0.so (mono_os_mutex_lock+42)

To avoid the above situation, wrap the `mono_domain_get()` call in a
method which checks the return value and, if it's `nullptr`, it calls
`mono_get_root_domain()` and attaches the current thread to that domain.
@grendello grendello requested a review from jonpryor as a code owner August 23, 2021 15:19
@@ -2327,7 +2329,7 @@ MonodroidRuntime::Java_mono_android_Runtime_register (JNIEnv *env, jstring manag

MonoMethod *register_jni_natives = registerType;
#if !defined (NET6)
MonoDomain *domain = mono_domain_get ();
MonoDomain *domain = utils.get_current_domain (/* attach_thread_if_needed */ false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does mono_thread_attach() differ from mono_jit_thread_attach()? Why use utils.get_current_domain (/* attach_thread_if_needed */ false) instead of using utils.get_current_domain () and removing the mono_jit_thread_attach() invocation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mono_jit_thread_attach will call mono_get_root_domain() when the domain passed to it is NULL, basically what our utils.get_current_domain() does and it also switches to the new domain if it's different than the current one (something mono_thread_attach doesn't do). The reason I left it around and added the call to utils.get_current_domain is because I usually try not to make as few changes to existing code as possible, using utils.get_current_domain is consistent throughout the code and there's the subtle side effect of mono_jit_thread_attach switching to the passed domain.

@@ -1041,7 +1041,7 @@ OSBridge::ensure_jnienv (void)
JNIEnv *env;
jvm->GetEnv ((void**)&env, JNI_VERSION_1_6);
if (env == nullptr) {
mono_thread_attach (mono_domain_get ());
mono_thread_attach (utils.get_current_domain (/* attach_thread_if_needed */ false));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If utils.get_current_domain() calls mono_thread_attach() if the domain isn't set -- which presumably it won't be if this thread hasn't been seen before (?) -- then do we need this separate mono_thread_attach() call?

Why not just call utils.get_current_domain() and let the side-effect attach?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't do that. If mono_domain_get does not return NULL then we wouldn't call mono_thread_attach at all. And it's most likely that mono_domain_get will not return NULL.

@jonpryor jonpryor merged commit bfa8eab into dotnet:main Aug 26, 2021
@grendello grendello deleted the issue-6211 branch August 26, 2021 16:20
@github-actions github-actions bot locked and limited conversation to collaborators Jan 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't handle exception by AppDomain.UnhandledException when thrown in Executor thread
2 participants