-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[iOS][Android] Fix crash in Exception.CaptureDispatchState #70970
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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")] | ||
|
@@ -66,34 +68,46 @@ 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) | ||
steveisok marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
stackFrames = Diagnostics.StackTrace.get_trace(this, 0, true); | ||
if (stackFrames.Length > 0) | ||
stackFrames[stackFrames.Length - 1].isLastFrameFromForeignException = true; | ||
|
||
if (foreignExceptionsFrames != null) | ||
if (_traceIPs != 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; | ||
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 = foreignExceptionsFrames; | ||
} | ||
} | ||
else | ||
{ | ||
stackFrames = foreignExceptionsFrames; | ||
} | ||
|
||
return new DispatchState(stackFrames); | ||
} | ||
|
||
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From what I can see, I don't believe @lambdageek - can you please help confirm? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, AFAICT There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. get_trace is only called if _traceIPs is not null. In what situation is _traceIPs null? Seems like it could be null if the exception wasn't thrown? So what happens if a new Exception is created, the dispatch info is captured before it's thrown, and then this race manifests when restoring that empty dispatch info concurrently with another capture attempt? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like there is some magic integration between I believe I guess what could happen is the stack frame could be flagged with @lambdageek @marek-safar your thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, Capture/Restore are pretty rare, right? Can we just lock everything in them? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not opposed. |
||
_stackTraceString = null; | ||
} | ||
} | ||
|
||
// Returns true if setting the _remoteStackTraceString field is legal, false if not (immutable exception). | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this field is not needed