From 308db9e36c0a97fda91ea43955d71d9429b1425a Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Tue, 1 Aug 2023 19:46:19 +0200 Subject: [PATCH 1/4] Fix stack overflow handling on Unix There was a race between cleaning up signal handlers at process abort and multiple threads failing with stack overflow at the same time. When a process exits due to a stack overflow, we should not restore the SIGSEGV handler so that all other threads failing due to the same condition just wait until the process abort completes. --- src/coreclr/pal/src/exception/seh.cpp | 2 +- src/coreclr/pal/src/exception/signal.cpp | 14 ++++++++++---- src/coreclr/pal/src/include/pal/signal.hpp | 6 ++++-- src/coreclr/pal/src/thread/process.cpp | 4 ++-- src/tests/issues.targets | 3 --- 5 files changed, 17 insertions(+), 12 deletions(-) diff --git a/src/coreclr/pal/src/exception/seh.cpp b/src/coreclr/pal/src/exception/seh.cpp index 2045084a5683e8..f359b9d2e6646d 100644 --- a/src/coreclr/pal/src/exception/seh.cpp +++ b/src/coreclr/pal/src/exception/seh.cpp @@ -109,7 +109,7 @@ SEHCleanup() { TRACE("Cleaning up SEH\n"); - SEHCleanupSignals(); + SEHCleanupSignals(false /* isChildProcess */); } /*++ diff --git a/src/coreclr/pal/src/exception/signal.cpp b/src/coreclr/pal/src/exception/signal.cpp index c3bdcc793eb95d..3ced6c04b79fd5 100644 --- a/src/coreclr/pal/src/exception/signal.cpp +++ b/src/coreclr/pal/src/exception/signal.cpp @@ -254,8 +254,7 @@ Function : Restore default signal handlers Parameters : - None - + isChildProcess - indicates that it is called from a child process fork (no return value) note : @@ -263,7 +262,7 @@ reason for this function is that during PAL_Terminate, we reach a point where SEH isn't possible anymore (handle manager is off, etc). Past that point, we can't avoid crashing on a signal. --*/ -void SEHCleanupSignals() +void SEHCleanupSignals(bool isChildProcess) { TRACE("Restoring default signal handlers\n"); @@ -276,7 +275,14 @@ void SEHCleanupSignals() restore_signal(SIGFPE, &g_previous_sigfpe); restore_signal(SIGBUS, &g_previous_sigbus); restore_signal(SIGABRT, &g_previous_sigabrt); - restore_signal(SIGSEGV, &g_previous_sigsegv); + // Do not restore the sigsegv when stack overflow was hit on a thread, + // since all other sigsegvs have to wait in the handler until the process + // dies. + // If it is called by a forked child though, restore the sigsegv unconditionally + if (isChildProcess || (g_stackOverflowHandlerStack != 0)) + { + restore_signal(SIGSEGV, &g_previous_sigsegv); + } restore_signal(SIGINT, &g_previous_sigint); restore_signal(SIGQUIT, &g_previous_sigquit); } diff --git a/src/coreclr/pal/src/include/pal/signal.hpp b/src/coreclr/pal/src/include/pal/signal.hpp index 6a3e41b4de473f..2dcad31cc44ebf 100644 --- a/src/coreclr/pal/src/include/pal/signal.hpp +++ b/src/coreclr/pal/src/include/pal/signal.hpp @@ -112,9 +112,11 @@ Function : SEHCleanupSignals Restore default signal handlers +Parameters : + isChildProcess - indicates that it is called from a child process fork - (no parameters, no return value) + (no return value) --*/ -void SEHCleanupSignals(); +void SEHCleanupSignals(bool isChildProcess); #endif /* _PAL_SIGNAL_HPP_ */ diff --git a/src/coreclr/pal/src/thread/process.cpp b/src/coreclr/pal/src/thread/process.cpp index dcfc4c936f5149..8388bef100fd4e 100644 --- a/src/coreclr/pal/src/thread/process.cpp +++ b/src/coreclr/pal/src/thread/process.cpp @@ -2229,7 +2229,7 @@ PROCCreateCrashDump( if (g_createdumpCallback != nullptr) { // Remove the signal handlers inherited from the runtime process - SEHCleanupSignals(); + SEHCleanupSignals(true /* isChildProcess */); // Call the statically linked createdump code g_createdumpCallback(argv.size(), argv.data()); @@ -2524,7 +2524,7 @@ PROCAbort(int signal, siginfo_t* siginfo) // Restore all signals; the SIGABORT handler to prevent recursion and // the others to prevent multiple core dumps from being generated. - SEHCleanupSignals(); + SEHCleanupSignals(false /* isChildProcess */); // Abort the process after waiting for the core dump to complete abort(); diff --git a/src/tests/issues.targets b/src/tests/issues.targets index 53e2109bec9192..daea1a54bf3fbd 100644 --- a/src/tests/issues.targets +++ b/src/tests/issues.targets @@ -86,9 +86,6 @@ - - https://github.com/dotnet/runtime/issues/46175 - https://github.com/dotnet/runtime/issues/80666 From b3ba84acfea2e42b5e4723fc6824dca785d9d9ef Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Tue, 1 Aug 2023 21:54:51 +0200 Subject: [PATCH 2/4] Fix macOS build break --- src/coreclr/pal/src/exception/signal.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/coreclr/pal/src/exception/signal.cpp b/src/coreclr/pal/src/exception/signal.cpp index 3ced6c04b79fd5..4019242d8c5482 100644 --- a/src/coreclr/pal/src/exception/signal.cpp +++ b/src/coreclr/pal/src/exception/signal.cpp @@ -275,11 +275,13 @@ void SEHCleanupSignals(bool isChildProcess) restore_signal(SIGFPE, &g_previous_sigfpe); restore_signal(SIGBUS, &g_previous_sigbus); restore_signal(SIGABRT, &g_previous_sigabrt); +#if !HAVE_MACH_EXCEPTIONS // Do not restore the sigsegv when stack overflow was hit on a thread, // since all other sigsegvs have to wait in the handler until the process // dies. // If it is called by a forked child though, restore the sigsegv unconditionally if (isChildProcess || (g_stackOverflowHandlerStack != 0)) +#endif { restore_signal(SIGSEGV, &g_previous_sigsegv); } From b7a77b424de08a4864d7b5946dcad3f1f0306fe1 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Wed, 2 Aug 2023 01:23:04 +0200 Subject: [PATCH 3/4] Modify the test to exclude large frame tests on arm64 Also make it resilient to extra output after the stack trace like createdump output --- .../stackoverflow/stackoverflowtester.cs | 44 +++++++++++++------ 1 file changed, 31 insertions(+), 13 deletions(-) diff --git a/src/tests/baseservices/exceptions/stackoverflow/stackoverflowtester.cs b/src/tests/baseservices/exceptions/stackoverflow/stackoverflowtester.cs index 52707b0508f5ec..92040488034497 100644 --- a/src/tests/baseservices/exceptions/stackoverflow/stackoverflowtester.cs +++ b/src/tests/baseservices/exceptions/stackoverflow/stackoverflowtester.cs @@ -4,6 +4,7 @@ using System.Collections.Generic; using System.Diagnostics; using System.IO; +using System.Runtime.InteropServices; using System.Text; namespace TestStackOverflow @@ -24,12 +25,25 @@ static bool TestStackOverflow(string testName, string testArgs, out List testProcess.StartInfo.Arguments = $"{Path.Combine(s_currentPath, "..", testName, $"{testName}.dll")} {testArgs}"; testProcess.StartInfo.UseShellExecute = false; testProcess.StartInfo.RedirectStandardError = true; + bool endOfStackTrace = false; testProcess.ErrorDataReceived += (sender, line) => { Console.WriteLine($"\"{line.Data}\""); - if (!string.IsNullOrEmpty(line.Data)) + if (!endOfStackTrace && !string.IsNullOrEmpty(line.Data)) { - lines.Add(line.Data); + // Store lines only till the end of the stack trace. + // In the CI it can also contain lines with createdump info. + if (line.Data.StartsWith("Stack overflow.") || + line.Data.StartsWith("Repeat ") || + line.Data.StartsWith("------") || + line.Data.StartsWith(" at ")) + { + lines.Add(line.Data); + } + else + { + endOfStackTrace = true; + } } }; @@ -256,24 +270,28 @@ static int Main() return 101; } - if (!TestStackOverflowLargeFrameMainThread()) - { - return 102; - } - if (!TestStackOverflowSmallFrameSecondaryThread()) { return 103; } - if (!TestStackOverflowLargeFrameSecondaryThread()) + if ((RuntimeInformation.ProcessArchitecture != Architecture.Arm64) || + ((Environment.OSVersion.Platform != PlatformID.Unix) && (Environment.OSVersion.Platform != PlatformID.MacOSX))) { - return 104; - } + if (!TestStackOverflowLargeFrameMainThread()) + { + return 102; + } - if (!TestStackOverflow3()) - { - return 105; + if (!TestStackOverflowLargeFrameSecondaryThread()) + { + return 104; + } + + if (!TestStackOverflow3()) + { + return 105; + } } return 100; From 2b6522541102ce47b7adbffe9930679556566be4 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Wed, 2 Aug 2023 23:46:42 +0000 Subject: [PATCH 4/4] Prevent generating dumps for the child processes --- .../baseservices/exceptions/stackoverflow/stackoverflowtester.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/tests/baseservices/exceptions/stackoverflow/stackoverflowtester.cs b/src/tests/baseservices/exceptions/stackoverflow/stackoverflowtester.cs index 92040488034497..f85b67aa9d910e 100644 --- a/src/tests/baseservices/exceptions/stackoverflow/stackoverflowtester.cs +++ b/src/tests/baseservices/exceptions/stackoverflow/stackoverflowtester.cs @@ -25,6 +25,7 @@ static bool TestStackOverflow(string testName, string testArgs, out List testProcess.StartInfo.Arguments = $"{Path.Combine(s_currentPath, "..", testName, $"{testName}.dll")} {testArgs}"; testProcess.StartInfo.UseShellExecute = false; testProcess.StartInfo.RedirectStandardError = true; + testProcess.StartInfo.Environment.Add("DOTNET_DbgEnableMiniDump", "0"); bool endOfStackTrace = false; testProcess.ErrorDataReceived += (sender, line) => {