From 8f28775964a7b678c9dd7d89be64ff4cad96e156 Mon Sep 17 00:00:00 2001 From: Marek Habersack Date: Sun, 17 Mar 2024 16:40:30 +0000 Subject: [PATCH] [Mono.Android] Prevent NullPointerException in TranslateStackTrace (#8795) Context: https://github.com/xamarin/xamarin-android/issues/8788#issuecomment-1980765624 Context: 1aa0ea7a8aef616d00e8467aa9eec83a1571cbd0 The [`java.lang.StackTraceElement(String, String, String, int)`][0] constructor requires that the `declaringClass` and `methodName` parameters never be `null`. Failure to do so results in a `NullPointerException`: I DOTNET : JavaProxyThrowable: translation threw an exception: Java.Lang.NullPointerException: Declaring class is null I DOTNET : at Java.Interop.JniEnvironment.InstanceMethods.CallNonvirtualVoidMethod(JniObjectReference instance, JniObjectReference type, JniMethodInfo method, JniArgumentValue* args) I DOTNET : at Java.Interop.JniPeerMembers.JniInstanceMethods.FinishCreateInstance(String constructorSignature, IJavaPeerable self, JniArgumentValue* parameter s) I DOTNET : at Java.Lang.StackTraceElement..ctor(String declaringClass, String methodName, String fileName, Int32 lineNumber) I DOTNET : at Android.Runtime.JavaProxyThrowable.TranslateStackTrace() I DOTNET : at Android.Runtime.JavaProxyThrowable.Create(Exception innerException) I DOTNET : --- End of managed Java.Lang.NullPointerException stack trace --- I DOTNET : java.lang.NullPointerException: Declaring class is null I DOTNET : at java.util.Objects.requireNonNull(Objects.java:228) I DOTNET : at java.lang.StackTraceElement.(StackTraceElement.java:71) I DOTNET : at crc6431345fe65afe8d98.AvaloniaMainActivity_1.n_onCreate(Native Method) Update `JavaProxyThrowable.TranslateStackTrace()` (1aa0ea7a8a) so that if `StackFrame.GetMethod()` returns `null`, we fallback to: 1. Trying to extract class name and method name from [`StackFrame.ToString()`][1]: MainActivity.OnCreate() + 0x37 at offset 55 in file:line:column :0:0 2. If (1) fails, pass `Unknown` for `declaringClass` and `methodName`. Additionally, the `lineNumber` parameter is now set to `-2` if we think a stack frame points to native code. [0]: https://developer.android.com/reference/java/lang/StackTraceElement#StackTraceElement(java.lang.String,%20java.lang.String,%20java.lang.String,%20int) [1]: https://github.com/xamarin/xamarin-android/pull/8758/files#r1504920023 --- .../Android.Runtime/JavaProxyThrowable.cs | 87 ++++++++++++++++++- .../System/ExceptionTest.cs | 29 +++++-- 2 files changed, 103 insertions(+), 13 deletions(-) diff --git a/src/Mono.Android/Android.Runtime/JavaProxyThrowable.cs b/src/Mono.Android/Android.Runtime/JavaProxyThrowable.cs index 5755f705f30..d221bf48054 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.GetILOffset():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,20 +138,22 @@ void TranslateStackTrace () // ..but ignore } - StackFrame[] frames = trace.GetFrames (); 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, string? methodName, string? declaringClass) = GetFrameInfo (managedFrame, managedMethod); var throwableFrame = new StackTraceElement ( - declaringClass: managedMethod?.DeclaringType?.FullName, - methodName: managedMethod?.Name, + declaringClass: declaringClass ?? Unknown, + methodName: methodName ?? Unknown, fileName: managedFrame?.GetFileName (), - lineNumber: managedFrame?.GetFileLineNumber () ?? -1 + lineNumber: lineNumber ); elements[i] = throwableFrame; diff --git a/tests/Mono.Android-Tests/System/ExceptionTest.cs b/tests/Mono.Android-Tests/System/ExceptionTest.cs index 0c2c1fd847b..19b1098d89c 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,21 @@ void CompareStackTraces (Exception ex, Java.Lang.Throwable throwable) var mf = managedFrames[i]; var jf = javaFrames[i]; - Assert.AreEqual (mf.GetMethod ()?.Name, jf.MethodName, $"Frame {i}: method names differ"); + // 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 = mf.HasNativeImage () ? -2 : -1; + } + + 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 (mf.GetFileLineNumber (), jf.LineNumber, $"Frame {i}: line numbers differ"); + Assert.AreEqual (managedLine, jf.LineNumber, $"Frame {i}: line numbers differ"); } } }