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

[Mono.Android] AndroidRuntime.RemovePeer() ignores invalid refs #5087

Closed
wants to merge 1 commit into from

Conversation

jonpryor
Copy link
Member

@jonpryor jonpryor commented Sep 4, 2020

Fixes: #4989
Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1193891

Commit 62a5709 forgot to apply the Mono.Android.dll changes! (Doh!)

Property cherry-pick d4e0738.


What happens if you dispose an instance *while disposing the instance*?

	// C#
	public class MyDisposableObject : Java.Lang.Object {
	    bool _isDisposed;

	    protected override void Dispose (bool disposing)
	    {
	        if (_isDisposed) {
	            return;
	        }
	        _isDisposed = true;
	        if (this.Handle != IntPtr.Zero)
	            this.Dispose ();
	        base.Dispose (disposing);
	    }
	}

	// …
	void MyTestMethod ()
	{
	    var value = new MyDisposableObject ();
	    value.Dispose ();
	    value.Dispose ();
	}

Here, `MyDisposableObject.Dispose(bool)` calls `Object.Dispose()`
when `Object.Handle` is valid.  This "feels" admittedly unusual, but
`IDisposable.Dispose()` is *supposed to be* Idempotent: it can be
called multiple times with no ill effects.  Shouldn't this be the same?

Unfortunately, it *isn't* the same; it crashes, hard:

	  =================================================================
	        Native stacktrace:
	  =================================================================
	        0x10245bc49 - /Library/Frameworks/Mono.framework/Versions/Current/Commands/mono : mono_dump_native_crash_info
	        0x1023f3d35 - /Library/Frameworks/Mono.framework/Versions/Current/Commands/mono : mono_handle_native_crash
	        0x10245b20f - /Library/Frameworks/Mono.framework/Versions/Current/Commands/mono : sigabrt_signal_handler
	        0x7fff6983d5fd - /usr/lib/system/libsystem_platform.dylib : _sigtramp
	        0x700009b716b0 - Unknown
	        0x7fff69713808 - /usr/lib/system/libsystem_c.dylib : abort
	        0x10658377a - …/lib/server/libjvm.dylib : _ZN2os5abortEbPvPKv
	        0x10637ea5f - …/lib/server/libjvm.dylib : _ZN8jniCheck15validate_handleEP10JavaThreadP8_jobject
	        0x10637eb08 - …/lib/server/libjvm.dylib : _ZN8jniCheck15validate_objectEP10JavaThreadP8_jobject
	        0x1063818a1 - …/lib/server/libjvm.dylib : checked_jni_GetObjectClass
	        0x10594cfa8 - …/Java.Interop/bin/TestDebug/libjava-interop.dylib : java_interop_jnienv_get_object_class
	        0x107a1c636 - Unknown
	        0x107aa2c73 - Unknown

	…
	  =================================================================
	        Managed Stacktrace:
	  =================================================================
	          at <unknown> <0xffffffff>
	          at Java.Interop.NativeMethods:java_interop_jnienv_get_object_class <0x000a5>
	          at Types:GetObjectClass <0x0010a>
	          at Types:GetJniTypeNameFromInstance <0x000a2>
	          at JniValueManager:DisposePeer <0x002c2>
	          at JniValueManager:DisposePeer <0x000f2>
	          at Java.Interop.JavaObject:Dispose <0x000ea>
	          at Java.InteropTests.JavaObjectTest:NestedDisposeInvocations <0x00072>
	          at System.Object:runtime_invoke_void__this__ <0x000b0>
	          at <unknown> <0xffffffff>
	          at System.Reflection.RuntimeMethodInfo:InternalInvoke <0x000b8>
	          at System.Reflection.RuntimeMethodInfo:Invoke <0x00152>
	          at System.Reflection.MethodBase:Invoke <0x00058>

Ouch.

The cause of the crash isn't the "successive" `.Dispose()` invocations
within `MyTestMethod()`, but rather the *nested* `.Dispose()`
invocation within `MyDisposableObject.Dispose()`.

Runtime execution is thus:

 1. `JavaObject.Dispose()`
 2. `JniRuntime.JniValueManager.DisposePeer(this)`
 3. `var h = this.PeerReference`
 4. `JniRuntime.JniValueManager.DisposePeer(h, this)`
 5. `JavaObject.Disposed()`
 6. `MyDisposableObject.Dispose(disposing:true)`
 7. `JavaObject.Dispose()`              // back to (1)?
 8. `JniRuntime.JniValueManager.DisposePeer(this)`
 9. `var h = this.PeerReference`        // *second* ref to `h`
10. `JniRuntime.JniValueManager.DisposePeer(h, this)`, which passes
    `h` to e.g. `JniEnvironment.Types.GetJniTypeNameFromInstance()`,
    thus requiring that `h` be a valid JNI reference, and also calls
    `JniObjectReference.Dispose()`, invalidating `h`.
11. "Unwinding" (4), call `Types.GetJniTypeNameFromInstance()` with
    the `h` from (3).

The problem *appears to be* the recursive `Dispose()` invocation on
(7), but the *actual* problem is step (3): by holding a cached/"old"
value of `this.PeerReference` -- and then later using that *same* value
in `JniRuntime.JniValueManager.DisposePeer()`-- when the nested
`JavaObject.Dispose()` invocation continues execution,
`this.PeerReference` will be *invalidated*, but the copy of the handle
from (3) will still be used!  This causes the JVM to very loudly abort.

The fix is to defer the "caching" present in (3): instead of storing
the `PeerReference` value "immediately" -- and disposing the same
value "later" -- don't store the value until *after*
`IJavaPeerable.Disposed()` is called.  This gives the
`Dispose(disposing:true)` method a chance to execute *before*
retaining any cached references to `PeerReference` -- which may in
turn invalidate `PeerReference`! -- thus ensuring that we only attempt
to dispose *valid* JNI handles.

Bump to xamarin/Java.Interop@007b35b, which contains fixes to
`JniRuntime.JniValueManager.DisposePeer()`, and add appropriate tests.

Fixes: dotnet#4989
Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1193891

Commit 62a5709 forgot to apply the `Mono.Android.dll` changes! (Doh!)

Property cherry-pick d4e0738.

~~~

What happens if you dispose an instance *while disposing the instance*?

	// C#
	public class MyDisposableObject : Java.Lang.Object {
	    bool _isDisposed;

	    protected override void Dispose (bool disposing)
	    {
	        if (_isDisposed) {
	            return;
	        }
	        _isDisposed = true;
	        if (this.Handle != IntPtr.Zero)
	            this.Dispose ();
	        base.Dispose (disposing);
	    }
	}

	// …
	void MyTestMethod ()
	{
	    var value = new MyDisposableObject ();
	    value.Dispose ();
	    value.Dispose ();
	}

Here, `MyDisposableObject.Dispose(bool)` calls `Object.Dispose()`
when `Object.Handle` is valid.  This "feels" admittedly unusual, but
`IDisposable.Dispose()` is *supposed to be* Idempotent: it can be
called multiple times with no ill effects.  Shouldn't this be the same?

Unfortunately, it *isn't* the same; it crashes, hard:

	  =================================================================
	        Native stacktrace:
	  =================================================================
	        0x10245bc49 - /Library/Frameworks/Mono.framework/Versions/Current/Commands/mono : mono_dump_native_crash_info
	        0x1023f3d35 - /Library/Frameworks/Mono.framework/Versions/Current/Commands/mono : mono_handle_native_crash
	        0x10245b20f - /Library/Frameworks/Mono.framework/Versions/Current/Commands/mono : sigabrt_signal_handler
	        0x7fff6983d5fd - /usr/lib/system/libsystem_platform.dylib : _sigtramp
	        0x700009b716b0 - Unknown
	        0x7fff69713808 - /usr/lib/system/libsystem_c.dylib : abort
	        0x10658377a - …/lib/server/libjvm.dylib : _ZN2os5abortEbPvPKv
	        0x10637ea5f - …/lib/server/libjvm.dylib : _ZN8jniCheck15validate_handleEP10JavaThreadP8_jobject
	        0x10637eb08 - …/lib/server/libjvm.dylib : _ZN8jniCheck15validate_objectEP10JavaThreadP8_jobject
	        0x1063818a1 - …/lib/server/libjvm.dylib : checked_jni_GetObjectClass
	        0x10594cfa8 - …/Java.Interop/bin/TestDebug/libjava-interop.dylib : java_interop_jnienv_get_object_class
	        0x107a1c636 - Unknown
	        0x107aa2c73 - Unknown

	…
	  =================================================================
	        Managed Stacktrace:
	  =================================================================
	          at <unknown> <0xffffffff>
	          at Java.Interop.NativeMethods:java_interop_jnienv_get_object_class <0x000a5>
	          at Types:GetObjectClass <0x0010a>
	          at Types:GetJniTypeNameFromInstance <0x000a2>
	          at JniValueManager:DisposePeer <0x002c2>
	          at JniValueManager:DisposePeer <0x000f2>
	          at Java.Interop.JavaObject:Dispose <0x000ea>
	          at Java.InteropTests.JavaObjectTest:NestedDisposeInvocations <0x00072>
	          at System.Object:runtime_invoke_void__this__ <0x000b0>
	          at <unknown> <0xffffffff>
	          at System.Reflection.RuntimeMethodInfo:InternalInvoke <0x000b8>
	          at System.Reflection.RuntimeMethodInfo:Invoke <0x00152>
	          at System.Reflection.MethodBase:Invoke <0x00058>

Ouch.

The cause of the crash isn't the "successive" `.Dispose()` invocations
within `MyTestMethod()`, but rather the *nested* `.Dispose()`
invocation within `MyDisposableObject.Dispose()`.

Runtime execution is thus:

 1. `JavaObject.Dispose()`
 2. `JniRuntime.JniValueManager.DisposePeer(this)`
 3. `var h = this.PeerReference`
 4. `JniRuntime.JniValueManager.DisposePeer(h, this)`
 5. `JavaObject.Disposed()`
 6. `MyDisposableObject.Dispose(disposing:true)`
 7. `JavaObject.Dispose()`              // back to (1)?
 8. `JniRuntime.JniValueManager.DisposePeer(this)`
 9. `var h = this.PeerReference`        // *second* ref to `h`
10. `JniRuntime.JniValueManager.DisposePeer(h, this)`, which passes
    `h` to e.g. `JniEnvironment.Types.GetJniTypeNameFromInstance()`,
    thus requiring that `h` be a valid JNI reference, and also calls
    `JniObjectReference.Dispose()`, invalidating `h`.
11. "Unwinding" (4), call `Types.GetJniTypeNameFromInstance()` with
    the `h` from (3).

The problem *appears to be* the recursive `Dispose()` invocation on
(7), but the *actual* problem is step (3): by holding a cached/"old"
value of `this.PeerReference` -- and then later using that *same* value
in `JniRuntime.JniValueManager.DisposePeer()`-- when the nested
`JavaObject.Dispose()` invocation continues execution,
`this.PeerReference` will be *invalidated*, but the copy of the handle
from (3) will still be used!  This causes the JVM to very loudly abort.

The fix is to defer the "caching" present in (3): instead of storing
the `PeerReference` value "immediately" -- and disposing the same
value "later" -- don't store the value until *after*
`IJavaPeerable.Disposed()` is called.  This gives the
`Dispose(disposing:true)` method a chance to execute *before*
retaining any cached references to `PeerReference` -- which may in
turn invalidate `PeerReference`! -- thus ensuring that we only attempt
to dispose *valid* JNI handles.

Bump to dotnet/java-interop@007b35b, which contains fixes to
`JniRuntime.JniValueManager.DisposePeer()`, and add appropriate tests.
@jonpryor
Copy link
Member Author

jonpryor commented Sep 5, 2020

This was not the correct fix. See instead PR #5090 and dotnet/java-interop#708.

@jonpryor jonpryor closed this Sep 5, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Jan 27, 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.

1 participant