Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[iOS][Android] Fix crash in Exception.CaptureDispatchState #70970

Merged
merged 3 commits into from
Jun 22, 2022
Merged

[iOS][Android] Fix crash in Exception.CaptureDispatchState #70970

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
890eb82
[iOS][Android] Fix crash in Exception.CaptureDispatchState
Jun 20, 2022
5fe7d36
Lock all of CaptureDispatchState
Jun 21, 2022
5521f8b
Marek's feedback
Jun 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 26 additions & 6 deletions src/mono/System.Private.CoreLib/src/System/Exception.Mono.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ public DispatchState(MonoStackFrame[]? stackFrames)

private bool HasBeenThrown => _traceIPs != null;

private readonly object frameLock = new object();
Copy link
Contributor

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


public MethodBase? TargetSite
{
[RequiresUnreferencedCode("Metadata for the method might be incomplete or removed")]
Expand All @@ -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;
}
Expand All @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, AFAICT StackTrace.get_trace always returns an array or throws an exception. It does not return null

Copy link
Member

@stephentoub stephentoub Jun 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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).
Expand Down