Skip to content

Prevent assert if stack trace has function pointer #63787

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

Merged
merged 9 commits into from
Jan 17, 2022
Merged

Prevent assert if stack trace has function pointer #63787

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6b7c32a
Prevent crash if stack trace has function pointer
kant2002 Jan 14, 2022
301bce4
Fix wording
kant2002 Jan 14, 2022
d19c78f
Match CoreCLR behaviour
kant2002 Jan 14, 2022
97d04b9
Remove no longer neede parameter
kant2002 Jan 14, 2022
723a237
Add test for function pointer signature in stack trace
kant2002 Jan 15, 2022
0ea16ce
Fix typo
kant2002 Jan 15, 2022
25e4b89
Update src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTe…
kant2002 Jan 15, 2022
ee5c9e4
Change validation of stack trace on Mono
kant2002 Jan 15, 2022
eb91287
Apply issue to test
kant2002 Jan 16, 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
14 changes: 13 additions & 1 deletion .../System.Private.StackTraceMetadata/src/Internal/StackTraceMetadata/MethodNameFormatter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,8 +255,12 @@ private void EmitTypeName(Handle typeHandle, bool namespaceQualified)
EmitString(typeHandle.ToGenericParameterHandle(_metadataReader).GetGenericParameter(_metadataReader).Name);
break;

case HandleType.FunctionPointerSignature:
EmitFunctionPointerTypeName();
break;

default:
Debug.Assert(false);
Debug.Assert(false, $"Type handle {typeHandle.HandleType} was not handled");
_outputBuilder.Append("???");
break;
}
Expand Down Expand Up @@ -407,6 +411,14 @@ private void EmitPointerTypeName(PointerSignatureHandle pointerSigHandle)
_outputBuilder.Append('*');
}

/// <summary>
/// Emit function pointer type.
/// </summary>
private void EmitFunctionPointerTypeName()
{
_outputBuilder.Append("IntPtr");
}

/// <summary>
/// Emit by-reference type.
/// </summary>
Expand Down
18 changes: 18 additions & 0 deletions src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,21 @@ public void ToString_NullFrame_ThrowsNullReferenceException()
Assert.Equal(Environment.NewLine, stackTrace.ToString());
}

[Fact]
public unsafe void ToString_FunctionPointerSignature()
{
// This is sepate from ToString_Invoke_ReturnsExpected since unsafe cannot be used for iterators
var stackTrace = FunctionPointerParameter(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Usage of explicit type (for reference: Coding Guidelines))

Suggested change
var stackTrace = FunctionPointerParameter(null);
StackTrace stackTrace = FunctionPointerParameter(null);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see var style used a lot in this file. I would say most of the time.

if (PlatformDetection.IsMonoRuntime)
{
Assert.Contains("System.Diagnostics.Tests.StackTraceTests.FunctionPointerParameter(MonoFNPtrFakeClass x)", stackTrace.ToString());
Copy link
Member

Choose a reason for hiding this comment

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

The Mono behavior is a bug that we'll want to fix eventually. Could you instead add [ActiveIssue("https://github.com/dotnet/runtime/issues/11354", TestRuntimes.Mono)] to this test?

(It doesn't make sense to cut a new bug for it; we have plenty of Mono bugs - Mono will either be fixed when 11354 is fixed, or there will be a new Mono-specific bug that we'll need to open as part of fixing 11354 to block all the failing tests added at that point.)

}
else
{
Assert.Contains("System.Diagnostics.Tests.StackTraceTests.FunctionPointerParameter(IntPtr x)", stackTrace.ToString());
}
}

[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
public void ToString_ShowILOffset()
{
Expand Down Expand Up @@ -384,6 +399,9 @@ public void ToString_ShowILOffset()
[MethodImpl(MethodImplOptions.NoOptimization | MethodImplOptions.NoInlining)]
private static StackTrace TwoParameters(int x, string y) => new StackTrace();

[MethodImpl(MethodImplOptions.NoOptimization | MethodImplOptions.NoInlining)]
private unsafe static StackTrace FunctionPointerParameter(delegate*<void> x) => new StackTrace();

[MethodImpl(MethodImplOptions.NoOptimization | MethodImplOptions.NoInlining)]
private static StackTrace Generic<T>() => new StackTrace();
[MethodImpl(MethodImplOptions.NoOptimization | MethodImplOptions.NoInlining)]
Expand Down
1 change: 1 addition & 0 deletions src/libraries/System.Diagnostics.StackTrace/tests/System.Diagnostics.StackTrace.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
<TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks>
<TestRuntime>true</TestRuntime>
<IncludeRemoteExecutor>true</IncludeRemoteExecutor>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>

<!-- these tests depend on the pdb files -->
<DebuggerSupport Condition="'$(DebuggerSupport)' == '' and '$(TargetOS)' == 'Browser'">true</DebuggerSupport>
Expand Down