From 890eb826f7099d843fbef7e4a35e9a395ee2555b Mon Sep 17 00:00:00 2001 From: Steve Pfister Date: Sun, 19 Jun 2022 21:50:10 -0400 Subject: [PATCH 1/3] [iOS][Android] Fix crash in Exception.CaptureDispatchState There is a crash in `Exception.CaptureDispatchState` when called from one thread at the same time another calls into `Exception.RestoreDispatchState`. The reason for the crash is due to the way we do not update `foreignExceptionFrames` in a thread-safe way. `foreignExceptionFrames` is used in both methods and can crash when the size changes before the array is copied. The fix is to lock when reading from and writing to `foreignExceptionFrames`. Fixes https://github.com/dotnet/runtime/issues/70081 --- .../src/System/Exception.Mono.cs | 32 +++++++++++++++---- 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Exception.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Exception.Mono.cs index 25eb031ddaa2b0..ad8150670d06ba 100644 --- a/src/mono/System.Private.CoreLib/src/System/Exception.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Exception.Mono.cs @@ -49,6 +49,8 @@ public DispatchState(MonoStackFrame[]? stackFrames) private bool HasBeenThrown => _traceIPs != null; + private readonly object frameLock = new object(); + public MethodBase? TargetSite { [RequiresUnreferencedCode("Metadata for the method might be incomplete or removed")] @@ -74,9 +76,22 @@ internal DispatchState CaptureDispatchState() if (foreignExceptionsFrames != null) { - var combinedStackFrames = new MonoStackFrame[stackFrames.Length + foreignExceptionsFrames.Length]; - Array.Copy(foreignExceptionsFrames, 0, combinedStackFrames, 0, foreignExceptionsFrames.Length); - Array.Copy(stackFrames, 0, combinedStackFrames, foreignExceptionsFrames.Length, stackFrames.Length); + MonoStackFrame[] combinedStackFrames; + int fefLength; + + // Make sure foreignExceptionFrames does not change at this point. + // Otherwise, the Array.Copy into combinedStackFrames can fail due to size differences + // + // See https://github.com/dotnet/runtime/issues/70081 + lock(frameLock) + { + fefLength = foreignExceptionsFrames.Length; + + combinedStackFrames = new MonoStackFrame[stackFrames.Length + fefLength]; + Array.Copy(foreignExceptionsFrames, 0, combinedStackFrames, 0, fefLength); + } + + Array.Copy(stackFrames, 0, combinedStackFrames, fefLength, stackFrames.Length); stackFrames = combinedStackFrames; } @@ -91,9 +106,14 @@ internal DispatchState CaptureDispatchState() internal void RestoreDispatchState(in DispatchState state) { - foreignExceptionsFrames = state.StackFrames; - - _stackTraceString = null; + // Isolate so we can safely update foreignExceptionFrames and CaptureDispatchState can read the correct values + // + // See https://github.com/dotnet/runtime/issues/70081 + lock(frameLock) + { + foreignExceptionsFrames = state.StackFrames; + _stackTraceString = null; + } } // Returns true if setting the _remoteStackTraceString field is legal, false if not (immutable exception). From 5fe7d36573257cb9cde5beea60602610c8361be2 Mon Sep 17 00:00:00 2001 From: Steve Pfister Date: Tue, 21 Jun 2022 13:33:16 -0400 Subject: [PATCH 2/3] Lock all of CaptureDispatchState --- .../src/System/Exception.Mono.cs | 44 ++++++++----------- 1 file changed, 19 insertions(+), 25 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Exception.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Exception.Mono.cs index ad8150670d06ba..8dd9c3b65e2526 100644 --- a/src/mono/System.Private.CoreLib/src/System/Exception.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Exception.Mono.cs @@ -68,37 +68,31 @@ internal DispatchState CaptureDispatchState() { MonoStackFrame[]? stackFrames; - if (_traceIPs != null) + // Make sure foreignExceptionFrames does not change at this point. + // Otherwise, the Array.Copy into combinedStackFrames can fail due to size differences + // + // See https://github.com/dotnet/runtime/issues/70081 + lock(frameLock) { - stackFrames = Diagnostics.StackTrace.get_trace(this, 0, true); - if (stackFrames.Length > 0) - stackFrames[stackFrames.Length - 1].isLastFrameFromForeignException = true; - - if (foreignExceptionsFrames != null) + if (_traceIPs != null) { - MonoStackFrame[] combinedStackFrames; - int fefLength; - - // Make sure foreignExceptionFrames does not change at this point. - // Otherwise, the Array.Copy into combinedStackFrames can fail due to size differences - // - // See https://github.com/dotnet/runtime/issues/70081 - lock(frameLock) + stackFrames = Diagnostics.StackTrace.get_trace(this, 0, true); + if (stackFrames.Length > 0) + stackFrames[stackFrames.Length - 1].isLastFrameFromForeignException = true; + + if (foreignExceptionsFrames != null) { - fefLength = foreignExceptionsFrames.Length; + var combinedStackFrames = new MonoStackFrame[stackFrames.Length + foreignExceptionsFrames.Length]; + Array.Copy(foreignExceptionsFrames, 0, combinedStackFrames, 0, foreignExceptionsFrames.Length); + Array.Copy(stackFrames, 0, combinedStackFrames, foreignExceptionsFrames.Length, stackFrames.Length); - combinedStackFrames = new MonoStackFrame[stackFrames.Length + fefLength]; - Array.Copy(foreignExceptionsFrames, 0, combinedStackFrames, 0, fefLength); + stackFrames = combinedStackFrames; } - - Array.Copy(stackFrames, 0, combinedStackFrames, fefLength, stackFrames.Length); - - stackFrames = combinedStackFrames; } - } - else - { - stackFrames = foreignExceptionsFrames; + else + { + stackFrames = foreignExceptionsFrames; + } } return new DispatchState(stackFrames); From 5521f8befe36605ddc379299ba3cd4fcc303da98 Mon Sep 17 00:00:00 2001 From: Steve Pfister Date: Wed, 22 Jun 2022 10:29:36 -0400 Subject: [PATCH 3/3] Marek's feedback --- .../src/System/Exception.Mono.cs | 53 ++++++++----------- 1 file changed, 23 insertions(+), 30 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Exception.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Exception.Mono.cs index 8dd9c3b65e2526..eea91b4a0be6ea 100644 --- a/src/mono/System.Private.CoreLib/src/System/Exception.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Exception.Mono.cs @@ -68,46 +68,39 @@ internal DispatchState CaptureDispatchState() { MonoStackFrame[]? stackFrames; - // Make sure foreignExceptionFrames does not change at this point. - // Otherwise, the Array.Copy into combinedStackFrames can fail due to size differences - // - // See https://github.com/dotnet/runtime/issues/70081 - lock(frameLock) + if (_traceIPs != null) { - if (_traceIPs != null) - { - stackFrames = Diagnostics.StackTrace.get_trace(this, 0, true); - if (stackFrames.Length > 0) - stackFrames[stackFrames.Length - 1].isLastFrameFromForeignException = true; - - if (foreignExceptionsFrames != null) - { - var combinedStackFrames = new MonoStackFrame[stackFrames.Length + foreignExceptionsFrames.Length]; - Array.Copy(foreignExceptionsFrames, 0, combinedStackFrames, 0, foreignExceptionsFrames.Length); - Array.Copy(stackFrames, 0, combinedStackFrames, foreignExceptionsFrames.Length, stackFrames.Length); - - stackFrames = combinedStackFrames; - } - } - else + stackFrames = Diagnostics.StackTrace.get_trace(this, 0, true); + if (stackFrames.Length > 0) + stackFrames[stackFrames.Length - 1].isLastFrameFromForeignException = true; + + // Make sure foreignExceptionsFrames does not change at this point. + // Otherwise, the Array.Copy into combinedStackFrames can fail due to size differences + // + // See https://github.com/dotnet/runtime/issues/70081 + MonoStackFrame[]? feFrames = foreignExceptionsFrames; + + if (feFrames != null) { - stackFrames = foreignExceptionsFrames; + var combinedStackFrames = new MonoStackFrame[stackFrames.Length + feFrames.Length]; + Array.Copy(feFrames, 0, combinedStackFrames, 0, feFrames.Length); + Array.Copy(stackFrames, 0, combinedStackFrames, feFrames.Length, stackFrames.Length); + + stackFrames = combinedStackFrames; } } + else + { + stackFrames = foreignExceptionsFrames; + } return new DispatchState(stackFrames); } internal void RestoreDispatchState(in DispatchState state) { - // Isolate so we can safely update foreignExceptionFrames and CaptureDispatchState can read the correct values - // - // See https://github.com/dotnet/runtime/issues/70081 - lock(frameLock) - { - foreignExceptionsFrames = state.StackFrames; - _stackTraceString = null; - } + foreignExceptionsFrames = state.StackFrames; + _stackTraceString = null; } // Returns true if setting the _remoteStackTraceString field is legal, false if not (immutable exception).