[iOS][Android] Fix crash in Exception.CaptureDispatchState #70970
Conversation
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 dotnet#70081
steveisok
requested review from
akoeplinger,
simonrozsival,
mdh1418 and
mkhamoyan
| // See https://github.com/dotnet/runtime/issues/70081 | ||
| lock(frameLock) | ||
| { | ||
| foreignExceptionsFrames = state.StackFrames; |
There was a problem hiding this comment.
Can state.StackFrames ever be null? If yes, this could still cause a null ref exception in CaptureDispatchState, if foregnExceptionFrames is set to null after the null check that's outside of the lock and before the .Length check inside of the lock. Do you need to move the null check to be inside the lock in CaptureDispatchState?
There was a problem hiding this comment.
From what I can see, I don't believe state.StackFrames can be null. Diagnostics.StackTrace.get_trace(this, 0, true) is an icall to a native function that appears to always return an array.
@lambdageek - can you please help confirm?
There was a problem hiding this comment.
yes, AFAICT StackTrace.get_trace always returns an array or throws an exception. It does not return null
There was a problem hiding this comment.
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.
It looks like there is some magic integration between Exception.Mono.cs and native code. It's likely there are gaps in my understanding, but I'll try my best to answer your question.
I believe _traceIPs becomes not null only after an exception instance is thrown. In your example, the dispatch info and foreignExceptionFrames would continue to be null until the exception is thrown. If there was a concurrent capture and restore, I think foreignExceptionFrames would continue to be null no matter what until the first get_trace read and subsequent restore.
I guess what could happen is the stack frame could be flagged with isLastFrameFromForeignException == true and foreignExceptionFrames could still be null if a concurrent Capture / Restore happens and there is no follow up Restore. It's unclear if this matters at all.
@lambdageek @marek-safar your thoughts?
There was a problem hiding this comment.
Yea, _traceIPs is set from native code in mono-exceptions.c after unwinding.
Capture/Restore are pretty rare, right? Can we just lock everything in them?
|
/backport to release/6.0 |
|
Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/2545516804 |
| @@ -49,6 +49,8 @@ public DispatchState(MonoStackFrame[]? stackFrames) | |||
|
|
|||
| private bool HasBeenThrown => _traceIPs != null; | |||
|
|
|||
| private readonly object frameLock = new object(); | |||
There was a problem hiding this comment.
this field is not needed
dotnet#70970 was merged with a field that was unused. This PR removes it.
#70970 was merged with a field that was unused. This PR removes it. Co-authored-by: Steve Pfister <[email protected]>
ghost
locked as resolved and limited conversation to collaborators
There is a crash in
Exception.CaptureDispatchStatewhen called from one thread at the same time another calls intoException.RestoreDispatchState. The reason for the crash is due to the way we do not updateforeignExceptionFramesin a thread-safe way.foreignExceptionFramesis used in both methods and can crash when the size changes before the array is copied.Fixes #70081