From 6b7c32a574e690061a33ccdab280df66beff56f8 Mon Sep 17 00:00:00 2001 From: Andrii Kurdiumov Date: Fri, 14 Jan 2022 17:06:06 +0600 Subject: [PATCH 1/9] Prevent crash if stack trace has function pointer Closes #63785 --- .../StackTraceMetadata/MethodNameFormatter.cs | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/coreclr/nativeaot/System.Private.StackTraceMetadata/src/Internal/StackTraceMetadata/MethodNameFormatter.cs b/src/coreclr/nativeaot/System.Private.StackTraceMetadata/src/Internal/StackTraceMetadata/MethodNameFormatter.cs index cc5dd065dac28b..8a91d0680d3fc9 100644 --- a/src/coreclr/nativeaot/System.Private.StackTraceMetadata/src/Internal/StackTraceMetadata/MethodNameFormatter.cs +++ b/src/coreclr/nativeaot/System.Private.StackTraceMetadata/src/Internal/StackTraceMetadata/MethodNameFormatter.cs @@ -255,8 +255,12 @@ private void EmitTypeName(Handle typeHandle, bool namespaceQualified) EmitString(typeHandle.ToGenericParameterHandle(_metadataReader).GetGenericParameter(_metadataReader).Name); break; + case HandleType.FunctionPointerSignature: + EmitFunctionPointerTypeName(typeHandle.ToFunctionPointerSignatureHandle(_metadataReader)); + break; + default: - Debug.Assert(false); + Debug.Assert(false, $"Type handle {typeHandle.HandleType} does not handled"); _outputBuilder.Append("???"); break; } @@ -407,6 +411,21 @@ private void EmitPointerTypeName(PointerSignatureHandle pointerSigHandle) _outputBuilder.Append('*'); } + /// + /// Emit function pointer type. + /// + /// Function pointer type specification signature handle + private void EmitFunctionPointerTypeName(FunctionPointerSignatureHandle functionPointerSigHandle) + { + FunctionPointerSignature functionPointerSig = _metadataReader.GetFunctionPointerSignature(functionPointerSigHandle); + _outputBuilder.Append("delegate*<"); + MethodSignature methodSignature = functionPointerSig.Signature.GetMethodSignature(_metadataReader); + EmitTypeVector(methodSignature.Parameters); + _outputBuilder.Append(','); + EmitTypeName(methodSignature.ReturnType, namespaceQualified: false); + _outputBuilder.Append('>'); + } + /// /// Emit by-reference type. /// From 301bce46d7ca2eed2a855ca98e7a2ebe56a8e9ae Mon Sep 17 00:00:00 2001 From: Andrii Kurdiumov Date: Fri, 14 Jan 2022 17:34:00 +0600 Subject: [PATCH 2/9] Fix wording --- .../src/Internal/StackTraceMetadata/MethodNameFormatter.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/nativeaot/System.Private.StackTraceMetadata/src/Internal/StackTraceMetadata/MethodNameFormatter.cs b/src/coreclr/nativeaot/System.Private.StackTraceMetadata/src/Internal/StackTraceMetadata/MethodNameFormatter.cs index 8a91d0680d3fc9..80d8e4c29197a0 100644 --- a/src/coreclr/nativeaot/System.Private.StackTraceMetadata/src/Internal/StackTraceMetadata/MethodNameFormatter.cs +++ b/src/coreclr/nativeaot/System.Private.StackTraceMetadata/src/Internal/StackTraceMetadata/MethodNameFormatter.cs @@ -260,7 +260,7 @@ private void EmitTypeName(Handle typeHandle, bool namespaceQualified) break; default: - Debug.Assert(false, $"Type handle {typeHandle.HandleType} does not handled"); + Debug.Assert(false, $"Type handle {typeHandle.HandleType} was not handled"); _outputBuilder.Append("???"); break; } From d19c78fcde2a7e7b90a01b1c15f22748b68730ae Mon Sep 17 00:00:00 2001 From: Andrii Kurdiumov Date: Fri, 14 Jan 2022 21:24:58 +0600 Subject: [PATCH 3/9] Match CoreCLR behaviour --- .../Internal/StackTraceMetadata/MethodNameFormatter.cs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/coreclr/nativeaot/System.Private.StackTraceMetadata/src/Internal/StackTraceMetadata/MethodNameFormatter.cs b/src/coreclr/nativeaot/System.Private.StackTraceMetadata/src/Internal/StackTraceMetadata/MethodNameFormatter.cs index 80d8e4c29197a0..02242912c17740 100644 --- a/src/coreclr/nativeaot/System.Private.StackTraceMetadata/src/Internal/StackTraceMetadata/MethodNameFormatter.cs +++ b/src/coreclr/nativeaot/System.Private.StackTraceMetadata/src/Internal/StackTraceMetadata/MethodNameFormatter.cs @@ -417,13 +417,7 @@ private void EmitPointerTypeName(PointerSignatureHandle pointerSigHandle) /// Function pointer type specification signature handle private void EmitFunctionPointerTypeName(FunctionPointerSignatureHandle functionPointerSigHandle) { - FunctionPointerSignature functionPointerSig = _metadataReader.GetFunctionPointerSignature(functionPointerSigHandle); - _outputBuilder.Append("delegate*<"); - MethodSignature methodSignature = functionPointerSig.Signature.GetMethodSignature(_metadataReader); - EmitTypeVector(methodSignature.Parameters); - _outputBuilder.Append(','); - EmitTypeName(methodSignature.ReturnType, namespaceQualified: false); - _outputBuilder.Append('>'); + _outputBuilder.Append("IntPtr"); } /// From 97d04b93fd33fd40af9475dc6d0f84754515a3ce Mon Sep 17 00:00:00 2001 From: Andrii Kurdiumov Date: Fri, 14 Jan 2022 21:32:00 +0600 Subject: [PATCH 4/9] Remove no longer neede parameter --- .../src/Internal/StackTraceMetadata/MethodNameFormatter.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/coreclr/nativeaot/System.Private.StackTraceMetadata/src/Internal/StackTraceMetadata/MethodNameFormatter.cs b/src/coreclr/nativeaot/System.Private.StackTraceMetadata/src/Internal/StackTraceMetadata/MethodNameFormatter.cs index 02242912c17740..ebfbab7a8e2c02 100644 --- a/src/coreclr/nativeaot/System.Private.StackTraceMetadata/src/Internal/StackTraceMetadata/MethodNameFormatter.cs +++ b/src/coreclr/nativeaot/System.Private.StackTraceMetadata/src/Internal/StackTraceMetadata/MethodNameFormatter.cs @@ -256,7 +256,7 @@ private void EmitTypeName(Handle typeHandle, bool namespaceQualified) break; case HandleType.FunctionPointerSignature: - EmitFunctionPointerTypeName(typeHandle.ToFunctionPointerSignatureHandle(_metadataReader)); + EmitFunctionPointerTypeName(); break; default: @@ -414,8 +414,7 @@ private void EmitPointerTypeName(PointerSignatureHandle pointerSigHandle) /// /// Emit function pointer type. /// - /// Function pointer type specification signature handle - private void EmitFunctionPointerTypeName(FunctionPointerSignatureHandle functionPointerSigHandle) + private void EmitFunctionPointerTypeName() { _outputBuilder.Append("IntPtr"); } From 723a23792cecda9a111cf4b676ed7eb6fa6a3cd3 Mon Sep 17 00:00:00 2001 From: Andrii Kurdiumov Date: Sat, 15 Jan 2022 11:45:56 +0600 Subject: [PATCH 5/9] Add test for function pointer signature in stack trace --- .../tests/StackTraceTests.cs | 10 ++++++++++ .../tests/System.Diagnostics.StackTrace.Tests.csproj | 1 + 2 files changed, 11 insertions(+) diff --git a/src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTests.cs b/src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTests.cs index 801a328dc8b1bd..e45fe7a0e77771 100644 --- a/src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTests.cs +++ b/src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTests.cs @@ -312,6 +312,14 @@ 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); + Assert.Contains("System.Diagnostics.Tests.StackTraceTests.FunctionPointerParameter(Int32 x)", stackTrace.ToString()); + } + [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] public void ToString_ShowILOffset() { @@ -383,6 +391,8 @@ public void ToString_ShowILOffset() private static StackTrace OneParameter(int x) => new StackTrace(); [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* x) => new StackTrace(); [MethodImpl(MethodImplOptions.NoOptimization | MethodImplOptions.NoInlining)] private static StackTrace Generic() => new StackTrace(); diff --git a/src/libraries/System.Diagnostics.StackTrace/tests/System.Diagnostics.StackTrace.Tests.csproj b/src/libraries/System.Diagnostics.StackTrace/tests/System.Diagnostics.StackTrace.Tests.csproj index d6d002664cfef3..aec1d927639468 100644 --- a/src/libraries/System.Diagnostics.StackTrace/tests/System.Diagnostics.StackTrace.Tests.csproj +++ b/src/libraries/System.Diagnostics.StackTrace/tests/System.Diagnostics.StackTrace.Tests.csproj @@ -3,6 +3,7 @@ $(NetCoreAppCurrent) true true + true true From 0ea16cef140dacd48edf891e439061176634e522 Mon Sep 17 00:00:00 2001 From: Andrii Kurdiumov Date: Sat, 15 Jan 2022 14:04:11 +0600 Subject: [PATCH 6/9] Fix typo --- .../System.Diagnostics.StackTrace/tests/StackTraceTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTests.cs b/src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTests.cs index e45fe7a0e77771..a52823b721fc80 100644 --- a/src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTests.cs +++ b/src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTests.cs @@ -317,7 +317,7 @@ public unsafe void ToString_FunctionPointerSignature() { // This is sepate from ToString_Invoke_ReturnsExpected since unsafe cannot be used for iterators var stackTrace = FunctionPointerParameter(null); - Assert.Contains("System.Diagnostics.Tests.StackTraceTests.FunctionPointerParameter(Int32 x)", stackTrace.ToString()); + Assert.Contains("System.Diagnostics.Tests.StackTraceTests.FunctionPointerParameter(IntPtr x)", stackTrace.ToString()); } [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] From 25e4b89a0840b21bfcaeb132607da8e58964b0c8 Mon Sep 17 00:00:00 2001 From: Andrii Kurdiumov Date: Sat, 15 Jan 2022 18:33:16 +0600 Subject: [PATCH 7/9] Update src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTests.cs Co-authored-by: Robin Lindner --- .../System.Diagnostics.StackTrace/tests/StackTraceTests.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTests.cs b/src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTests.cs index a52823b721fc80..035c47ddb9ee2d 100644 --- a/src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTests.cs +++ b/src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTests.cs @@ -391,6 +391,7 @@ public void ToString_ShowILOffset() private static StackTrace OneParameter(int x) => new StackTrace(); [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* x) => new StackTrace(); From ee5c9e41a2e872e4a82cce3b8d9622117e7872b4 Mon Sep 17 00:00:00 2001 From: Andrii Kurdiumov Date: Sat, 15 Jan 2022 18:46:20 +0600 Subject: [PATCH 8/9] Change validation of stack trace on Mono Mono have different messages --- .../tests/StackTraceTests.cs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTests.cs b/src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTests.cs index 035c47ddb9ee2d..7bbfacf2c542b4 100644 --- a/src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTests.cs +++ b/src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTests.cs @@ -317,7 +317,14 @@ public unsafe void ToString_FunctionPointerSignature() { // This is sepate from ToString_Invoke_ReturnsExpected since unsafe cannot be used for iterators var stackTrace = FunctionPointerParameter(null); - Assert.Contains("System.Diagnostics.Tests.StackTraceTests.FunctionPointerParameter(IntPtr x)", stackTrace.ToString()); + if (PlatformDetection.IsMonoRuntime) + { + Assert.Contains("System.Diagnostics.Tests.StackTraceTests.FunctionPointerParameter(MonoFNPtrFakeClass x)", stackTrace.ToString()); + } + else + { + Assert.Contains("System.Diagnostics.Tests.StackTraceTests.FunctionPointerParameter(IntPtr x)", stackTrace.ToString()); + } } [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] From eb9128789a12ab3d7733f0cd575536e3376ffa35 Mon Sep 17 00:00:00 2001 From: Andrii Kurdiumov Date: Sun, 16 Jan 2022 11:39:20 +0600 Subject: [PATCH 9/9] Apply issue to test --- .../tests/StackTraceTests.cs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTests.cs b/src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTests.cs index 7bbfacf2c542b4..604a34dc8cd2a1 100644 --- a/src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTests.cs +++ b/src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTests.cs @@ -313,18 +313,12 @@ public void ToString_NullFrame_ThrowsNullReferenceException() } [Fact] + [ActiveIssue("https://github.com/dotnet/runtime/issues/11354", TestRuntimes.Mono)] public unsafe void ToString_FunctionPointerSignature() { // This is sepate from ToString_Invoke_ReturnsExpected since unsafe cannot be used for iterators var stackTrace = FunctionPointerParameter(null); - if (PlatformDetection.IsMonoRuntime) - { - Assert.Contains("System.Diagnostics.Tests.StackTraceTests.FunctionPointerParameter(MonoFNPtrFakeClass x)", stackTrace.ToString()); - } - else - { - Assert.Contains("System.Diagnostics.Tests.StackTraceTests.FunctionPointerParameter(IntPtr x)", stackTrace.ToString()); - } + Assert.Contains("System.Diagnostics.Tests.StackTraceTests.FunctionPointerParameter(IntPtr x)", stackTrace.ToString()); } [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]