From de9a903d3ba0707cf2decb8878f1860e7f6d84fa Mon Sep 17 00:00:00 2001 From: Marek Habersack Date: Thu, 7 Mar 2024 11:29:30 +0100 Subject: [PATCH 1/4] Prevent NREX when instantiating Java.Lang.StackTraceElement Context: https://github.com/xamarin/xamarin-android/issues/8788#issuecomment-1980765624 Context: https://developer.android.com/reference/java/lang/StackTraceElement?hl=en#StackTraceElement(java.lang.String,%20java.lang.String,%20java.lang.String,%20int) `Java.Lang.StackTraceElement` constructor requires the `declaringClass` and `methodName` parameters to never be `null`. Pass the string `Unknown` whenever any of this information is missing from the managed stack frame. Additionally, the `lineNumber` parameter is now set to `-2` if we think a stack frame points to native code. --- .../Android.Runtime/JavaProxyThrowable.cs | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/src/Mono.Android/Android.Runtime/JavaProxyThrowable.cs b/src/Mono.Android/Android.Runtime/JavaProxyThrowable.cs index 5755f705f30..31118ec193a 100644 --- a/src/Mono.Android/Android.Runtime/JavaProxyThrowable.cs +++ b/src/Mono.Android/Android.Runtime/JavaProxyThrowable.cs @@ -66,15 +66,28 @@ void TranslateStackTrace () int nElements = frames.Length + (javaTrace?.Length ?? 0); StackTraceElement[] elements = new StackTraceElement[nElements]; + const string Unknown = "Unknown"; for (int i = 0; i < frames.Length; i++) { StackFrame managedFrame = frames[i]; MethodBase? managedMethod = StackFrameGetMethod (managedFrame); + // https://developer.android.com/reference/java/lang/StackTraceElement?hl=en#StackTraceElement(java.lang.String,%20java.lang.String,%20java.lang.String,%20int) + int lineNumber; + if (managedFrame != null) { + lineNumber = managedFrame.GetFileLineNumber (); + if (lineNumber == 0) { + // -2 means it's a native frame + lineNumber = managedFrame.HasNativeImage () ? -2 : -1; + } + } else { + lineNumber = -1; + } + var throwableFrame = new StackTraceElement ( - declaringClass: managedMethod?.DeclaringType?.FullName, - methodName: managedMethod?.Name, + declaringClass: managedMethod?.DeclaringType?.FullName ?? Unknown, + methodName: managedMethod?.Name ?? Unknown, fileName: managedFrame?.GetFileName (), - lineNumber: managedFrame?.GetFileLineNumber () ?? -1 + lineNumber: lineNumber ); elements[i] = throwableFrame; From 695304c0cc4e4f587df8a3ca62c5622d861ef35f Mon Sep 17 00:00:00 2001 From: Marek Habersack Date: Fri, 8 Mar 2024 12:10:20 +0100 Subject: [PATCH 2/4] Update unit test --- .../System/ExceptionTest.cs | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/tests/Mono.Android-Tests/System/ExceptionTest.cs b/tests/Mono.Android-Tests/System/ExceptionTest.cs index 0c2c1fd847b..0a551e8b5c7 100644 --- a/tests/Mono.Android-Tests/System/ExceptionTest.cs +++ b/tests/Mono.Android-Tests/System/ExceptionTest.cs @@ -39,13 +39,13 @@ public void InnerExceptionIsSet () ex = e; } - using (Java.Lang.Throwable proxy = CreateJavaProxyThrowable (ex)) - using (var source = new Java.Lang.Throwable ("detailMessage", proxy)) - using (var alias = new Java.Lang.Throwable (source.Handle, JniHandleOwnership.DoNotTransfer)) { - CompareStackTraces (ex, proxy); - Assert.AreEqual ("detailMessage", alias.Message); - Assert.AreSame (ex, alias.InnerException); - } + using Java.Lang.Throwable proxy = CreateJavaProxyThrowable (ex); + using var source = new Java.Lang.Throwable ("detailMessage", proxy); + using var alias = new Java.Lang.Throwable (source.Handle, JniHandleOwnership.DoNotTransfer); + + CompareStackTraces (ex, proxy); + Assert.AreEqual ("detailMessage", alias.Message); + Assert.AreSame (ex, alias.InnerException); } void CompareStackTraces (Exception ex, Java.Lang.Throwable throwable) @@ -61,10 +61,16 @@ void CompareStackTraces (Exception ex, Java.Lang.Throwable throwable) var mf = managedFrames[i]; var jf = javaFrames[i]; + // Unknown line locations are -1 on the Java side + int managedLine = mf.GetFileLineNumber (); + if (managedLine == 0) { + managedLine = -1; + } + Assert.AreEqual (mf.GetMethod ()?.Name, jf.MethodName, $"Frame {i}: method names differ"); Assert.AreEqual (mf.GetMethod ()?.DeclaringType.FullName, jf.ClassName, $"Frame {i}: class names differ"); Assert.AreEqual (mf.GetFileName (), jf.FileName, $"Frame {i}: file names differ"); - Assert.AreEqual (mf.GetFileLineNumber (), jf.LineNumber, $"Frame {i}: line numbers differ"); + Assert.AreEqual (managedLine, jf.LineNumber, $"Frame {i}: line numbers differ"); } } } From 705a6ec74f724fca7d313ea0e87e05bd9f16b0a0 Mon Sep 17 00:00:00 2001 From: Marek Habersack Date: Wed, 13 Mar 2024 11:59:28 +0100 Subject: [PATCH 3/4] More comprehensive frame reporting --- .../Android.Runtime/JavaProxyThrowable.cs | 94 ++++++++++++++++--- 1 file changed, 80 insertions(+), 14 deletions(-) diff --git a/src/Mono.Android/Android.Runtime/JavaProxyThrowable.cs b/src/Mono.Android/Android.Runtime/JavaProxyThrowable.cs index 31118ec193a..00b2944d022 100644 --- a/src/Mono.Android/Android.Runtime/JavaProxyThrowable.cs +++ b/src/Mono.Android/Android.Runtime/JavaProxyThrowable.cs @@ -1,7 +1,9 @@ using System; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; +using System.Globalization; using System.Reflection; +using System.Text; using StackTraceElement = Java.Lang.StackTraceElement; @@ -37,6 +39,81 @@ public static JavaProxyThrowable Create (Exception innerException) return proxy; } + (int lineNumber, string? methodName, string? className) GetFrameInfo (StackFrame? managedFrame, MethodBase? managedMethod) + { + string? methodName = null; + string? className = null; + + if (managedFrame == null) { + if (managedMethod != null) { + methodName = managedMethod.Name; + className = managedMethod.DeclaringType?.FullName; + } + + return (-1, methodName, className); + } + + int lineNumber = -1; + lineNumber = managedFrame.GetFileLineNumber (); + if (lineNumber == 0) { + // -2 means it's a native frame + lineNumber = managedFrame.HasNativeImage () ? -2 : -1; + } + + if (managedMethod != null) { + // If we have no line number information and if it's a managed frame, add the + // IL offset. + if (lineNumber == -1 && managedFrame.HasILOffset ()) { + methodName = $"{managedMethod.Name} + 0x{managedFrame.HasILOffset:x}"; + } else { + methodName = managedMethod.Name; + } + + return (lineNumber, methodName, managedMethod.DeclaringType?.FullName); + } + + string frameString = managedFrame.ToString (); + var sb = new StringBuilder (); + + // We take the part of the returned string that stretches from the beginning to the first space character + // and treat it as the method name. + // https://github.com/dotnet/runtime/blob/18c3ad05c3fc127c3b7f37c49bc350bf7f8264a0/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/DeveloperExperience/DeveloperExperience.cs#L15-L55 + int pos = frameString.IndexOf (' '); + string? fullName = null; + if (pos > 1) { + fullName = frameString.Substring (0, pos); + } + + if (!String.IsNullOrEmpty (fullName) && (pos = fullName.LastIndexOf ('.')) >= 1) { + className = pos + 1 < fullName.Length ? fullName.Substring (pos + 1) : null; + fullName = fullName.Substring (0, pos); + } + + if (!String.IsNullOrEmpty (fullName)) { + sb.Append (fullName); + } else if (managedFrame.HasNativeImage ()) { + // We have no name, so we'll put the native IP + nint nativeIP = managedFrame.GetNativeIP (); + sb.Append (CultureInfo.InvariantCulture, $"Native 0x{nativeIP:x}"); + } + + if (sb.Length > 0) { + // We will also append information native offset information, if available and only if we + // have recorded any previous information, since the offset without context is useless. + int nativeOffset = managedFrame.GetNativeOffset (); + if (nativeOffset != StackFrame.OFFSET_UNKNOWN) { + sb.Append (" + "); + sb.Append (CultureInfo.InvariantCulture, $"0x{nativeOffset:x}"); + } + } + + if (sb.Length > 0) { + methodName = sb.ToString (); + } + + return (lineNumber, methodName, className); + } + void TranslateStackTrace () { // FIXME: https://github.com/xamarin/xamarin-android/issues/8724 @@ -61,7 +138,6 @@ void TranslateStackTrace () // ..but ignore } - StackFrame[] frames = trace.GetFrames (); int nElements = frames.Length + (javaTrace?.Length ?? 0); StackTraceElement[] elements = new StackTraceElement[nElements]; @@ -72,20 +148,10 @@ void TranslateStackTrace () MethodBase? managedMethod = StackFrameGetMethod (managedFrame); // https://developer.android.com/reference/java/lang/StackTraceElement?hl=en#StackTraceElement(java.lang.String,%20java.lang.String,%20java.lang.String,%20int) - int lineNumber; - if (managedFrame != null) { - lineNumber = managedFrame.GetFileLineNumber (); - if (lineNumber == 0) { - // -2 means it's a native frame - lineNumber = managedFrame.HasNativeImage () ? -2 : -1; - } - } else { - lineNumber = -1; - } - + (int lineNumber, string? methodName, string? declaringClass) = GetFrameInfo (managedFrame, managedMethod); var throwableFrame = new StackTraceElement ( - declaringClass: managedMethod?.DeclaringType?.FullName ?? Unknown, - methodName: managedMethod?.Name ?? Unknown, + declaringClass: declaringClass ?? Unknown, + methodName: methodName ?? Unknown, fileName: managedFrame?.GetFileName (), lineNumber: lineNumber ); From e13935de1ed5567b30c83a888f4fd108e817f7e7 Mon Sep 17 00:00:00 2001 From: Marek Habersack Date: Fri, 15 Mar 2024 12:02:25 +0100 Subject: [PATCH 4/4] Fix oops(1) and oops(2) --- .../Android.Runtime/JavaProxyThrowable.cs | 2 +- tests/Mono.Android-Tests/System/ExceptionTest.cs | 11 ++++++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/Mono.Android/Android.Runtime/JavaProxyThrowable.cs b/src/Mono.Android/Android.Runtime/JavaProxyThrowable.cs index 00b2944d022..d221bf48054 100644 --- a/src/Mono.Android/Android.Runtime/JavaProxyThrowable.cs +++ b/src/Mono.Android/Android.Runtime/JavaProxyThrowable.cs @@ -64,7 +64,7 @@ public static JavaProxyThrowable Create (Exception innerException) // If we have no line number information and if it's a managed frame, add the // IL offset. if (lineNumber == -1 && managedFrame.HasILOffset ()) { - methodName = $"{managedMethod.Name} + 0x{managedFrame.HasILOffset:x}"; + methodName = $"{managedMethod.Name} + 0x{managedFrame.GetILOffset():x}"; } else { methodName = managedMethod.Name; } diff --git a/tests/Mono.Android-Tests/System/ExceptionTest.cs b/tests/Mono.Android-Tests/System/ExceptionTest.cs index 0a551e8b5c7..19b1098d89c 100644 --- a/tests/Mono.Android-Tests/System/ExceptionTest.cs +++ b/tests/Mono.Android-Tests/System/ExceptionTest.cs @@ -61,13 +61,18 @@ void CompareStackTraces (Exception ex, Java.Lang.Throwable throwable) var mf = managedFrames[i]; var jf = javaFrames[i]; - // Unknown line locations are -1 on the Java side + // Unknown line locations are -1 on the Java side if they're managed, -2 if they're native int managedLine = mf.GetFileLineNumber (); if (managedLine == 0) { - managedLine = -1; + managedLine = mf.HasNativeImage () ? -2 : -1; } - Assert.AreEqual (mf.GetMethod ()?.Name, jf.MethodName, $"Frame {i}: method names differ"); + if (managedLine > 0) { + Assert.AreEqual (mf.GetMethod ()?.Name, jf.MethodName, $"Frame {i}: method names differ"); + } else { + string managedMethodName = mf.GetMethod ()?.Name ?? String.Empty; + Assert.IsTrue (jf.MethodName.StartsWith ($"{managedMethodName} + 0x"), $"Frame {i}: method name should start with: '{managedMethodName} + 0x'"); + } Assert.AreEqual (mf.GetMethod ()?.DeclaringType.FullName, jf.ClassName, $"Frame {i}: class names differ"); Assert.AreEqual (mf.GetFileName (), jf.FileName, $"Frame {i}: file names differ"); Assert.AreEqual (managedLine, jf.LineNumber, $"Frame {i}: line numbers differ");