Fix possible deadlocks on PosixSignalRegistration disposal on Windows

src/libraries/Microsoft.Extensions.Hosting/src/Internal/ConsoleLifetime.netcoreapp.cs

@@ -38,11 +38,7 @@ private void HandleWindowsShutdown(PosixSignalContext context)
             // for SIGTERM on Windows we must block this thread until the application is finished
             // otherwise the process will be killed immediately on return from this handler
 
-            // don't allow Dispose to unregister handlers, since Windows has a lock that prevents the unregistration while this handler is running
-            // just leak these, since the process is exiting
-            _sigIntRegistration = null;
-            _sigQuitRegistration = null;
-            _sigTermRegistration = null;
+            UnregisterShutdownHandlers();
 
             ApplicationLifetime.StopApplication();
 

src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs

@@ -81,6 +81,31 @@ private void AssertNonZeroAllZeroDarwin(long value)
             }
         }
 
+        private static void AssertRemoteProcessStandardOutputLine(RemoteInvokeHandle remoteHandle, string expectedMessage, int timeout)
+        {
+            using CancellationTokenSource cts = new(timeout);
+
+            Task<string?> readTask = remoteHandle.Process.StandardOutput.ReadLineAsync(cts.Token).AsTask();
+            string? line;
+            try
+            {
+                line = readTask.ConfigureAwait(false).GetAwaiter().GetResult();
+            }
+            catch (OperationCanceledException)
+            {
+                throw new XunitException($"Expected message '{expectedMessage}' was not observed on remote process within specified time.");
+            }
+
+            if (line != null)
+            {
+                Assert.Equal(expectedMessage, line);
+            }
+            else
+            {
+                throw new XunitException($"StandardOutput was closed before observing expected message '{expectedMessage}'");
+            }
+        }
+
         public static IEnumerable<object[]> SignalTestData()
         {
             if (OperatingSystem.IsWindows())
@@ -141,10 +166,7 @@ public void TestCreateNewProcessGroup_HandlerReceivesExpectedSignal(PosixSignal
                 arg: $"{signal}",
                 remoteInvokeOptions);
 
-            while (!remoteHandle.Process.StandardOutput.ReadLine().EndsWith(PosixSignalRegistrationCreatedMessage))
-            {
-                Thread.Sleep(20);
-            }
+            AssertRemoteProcessStandardOutputLine(remoteHandle, PosixSignalRegistrationCreatedMessage, WaitInMS);
 
             try
             {
@@ -161,6 +183,100 @@ public void TestCreateNewProcessGroup_HandlerReceivesExpectedSignal(PosixSignal
             }
         }
 
+        [ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
+        // Limited only to signals which terminates process by default, and are supported on all platforms by SendSignal
+        [InlineData(PosixSignal.SIGINT)]
+        [InlineData(PosixSignal.SIGQUIT)]
+        public void SignalHandler_CanDisposeInHandler(PosixSignal signal)
+        {
+            const string PosixSignalRegistrationCreatedMessage = "PosixSignalRegistration created.";
+            const string PosixSignalHandlerStartedMessage = "PosixSignalRegistration handler started.";
+            const string PosixSignalHandlerDisposedMessage = "PosixSignalRegistration disposed.";
+            const int UnterminatedExitCode = -1;
+
+            var remoteInvokeOptions = new RemoteInvokeOptions { CheckExitCode = false };
+            remoteInvokeOptions.StartInfo.RedirectStandardOutput = true;
+            if (OperatingSystem.IsWindows())
+            {
+                remoteInvokeOptions.StartInfo.CreateNewProcessGroup = true;
+            }
+
+            using RemoteInvokeHandle remoteHandle = RemoteExecutor.Invoke(
+                (signalStr) =>
+                {
+                    PosixSignal expectedSignal = Enum.Parse<PosixSignal>(signalStr);
+                    using ManualResetEvent receivedSignalEvent = new ManualResetEvent(false);
+                    ReEnableCtrlCHandlerIfNeeded(expectedSignal);
+
+                    PosixSignalRegistration? p = null;
+                    p = PosixSignalRegistration.Create(expectedSignal, (ctx) =>
+                    {
+                        Console.WriteLine(PosixSignalHandlerStartedMessage);
+                        Assert.Equal(expectedSignal, ctx.Signal);
+
+                        Assert.NotNull(p);
+                        p?.Dispose();
+
+                        // Used for checking that Dispose returned and did not get stuck
+                        Console.WriteLine(PosixSignalHandlerDisposedMessage);
+
+                        receivedSignalEvent.Set();
+                        ctx.Cancel = true;
+                    });
+
+                    Console.WriteLine(PosixSignalRegistrationCreatedMessage);
+
+                    // Wait for signal which unregisters itself
+                    Assert.True(receivedSignalEvent.WaitOne(WaitInMS));
+
+                    // Wait for second signal which should terminate process by default system handler
+                    Thread.Sleep(WaitInMS);
+
+                    // If we did not terminated yet, return failure exit code
+                    return UnterminatedExitCode;
+                },
+                arg: $"{signal}",
+                remoteInvokeOptions);
+
+
+            try
+            {
+                AssertRemoteProcessStandardOutputLine(remoteHandle, PosixSignalRegistrationCreatedMessage, WaitInMS);
+
+                SendSignal(signal, remoteHandle.Process.Id);
+
+                AssertRemoteProcessStandardOutputLine(remoteHandle, PosixSignalHandlerStartedMessage, WaitInMS);
+                AssertRemoteProcessStandardOutputLine(remoteHandle, PosixSignalHandlerDisposedMessage, WaitInMS);
+
+                SendSignal(signal, remoteHandle.Process.Id);
+
+                // https://github.com/dotnet/runtime/issues/125733
+                if (PlatformDetection.IsMonoRuntime && signal == PosixSignal.SIGQUIT && !PlatformDetection.IsWindows)
+                {
+                    SendSignal(PosixSignal.SIGTERM, remoteHandle.Process.Id);
+                }
+
+                Assert.True(remoteHandle.Process.WaitForExit(WaitInMS));
+                Assert.True(remoteHandle.Process.StandardOutput.EndOfStream);
+                if (OperatingSystem.IsWindows())
+                {
+                    Assert.Equal(unchecked((int)0xC000013A), remoteHandle.Process.ExitCode); // STATUS_CONTROL_C_EXIT
+                }
+                else
+                {
+                    // Signal numbers are platform dependent, so we can't check exact exit code
+                    Assert.NotEqual(0, remoteHandle.Process.ExitCode);
+                    Assert.NotEqual(UnterminatedExitCode, remoteHandle.Process.ExitCode);
+                }
+            }
+            finally
+            {
+                // If sending the signal fails or process did not exit on its own, we want to kill the process ASAP
+                // to prevent RemoteExecutor's timeout from hiding it.
+                remoteHandle.Process.Kill();
+            }
+        }
+
         [ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
         [InlineData(-2)]
         [InlineData((long)int.MaxValue + 1)]

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Windows.cs

@@ -10,6 +10,18 @@ public sealed partial class PosixSignalRegistration
     {
         private static readonly Dictionary<int, List<Token>> s_registrations = new();
 
+        /// <summary>
+        /// Serializes concurrent <see cref="Register"/> calls to make the emptiness check and
+        /// the subsequent token insertion atomic.
+        /// </summary>
+        private static readonly object s_registerLock = new();
+
+        /// <summary>
+        /// Runtime can generate multiple addresses to the same function. To ensure that registering and
+        /// unregistering always use the same instance, we capture it in this static field.
+        /// </summary>
+        private static readonly unsafe delegate* unmanaged<int, Interop.BOOL> s_handlerRoutineAddr = &HandlerRoutine;
+
         private static unsafe PosixSignalRegistration Register(PosixSignal signal, Action<PosixSignalContext> handler)
         {
             int signo = signal switch
@@ -24,26 +36,56 @@ private static unsafe PosixSignalRegistration Register(PosixSignal signal, Actio
             var token = new Token(signal, signo, handler);
             var registration = new PosixSignalRegistration(token);
 
-            lock (s_registrations)
+            lock (s_registerLock)
             {
-                if (s_registrations.Count == 0 &&
-                    !Interop.Kernel32.SetConsoleCtrlHandler(&HandlerRoutine, Add: true))
+                bool registerCtrlHandler = false;
+                lock (s_registrations)
                 {
-                    throw Win32Marshal.GetExceptionForLastWin32Error();
+                    if (s_registrations.Count == 0)
+                    {
+                        registerCtrlHandler = true;
+                    }
                 }
 
-                if (!s_registrations.TryGetValue(signo, out List<Token>? tokens))
+                // All SetConsoleCtrlHandler calls must happen outside s_registrations locked section
+                // otherwise we risk AB/BA deadlock between it and internal critical section in OS.
+
+                if (registerCtrlHandler)
                 {
-                    s_registrations[signo] = tokens = new List<Token>();
+                    // User may reset registrations externally by direct calls of Free/Attach/AllocConsole.
+                    // We do not know if it is currently registered or not. To prevent duplicate
+                    // registration, we try unregister existing one first.
+                    if (!Interop.Kernel32.SetConsoleCtrlHandler(s_handlerRoutineAddr, Add: false))
+                    {
+                        // Returns ERROR_INVALID_PARAMETER if it was not registered. Throw for everything else.
+                        int error = Marshal.GetLastPInvokeError();
+                        if (error != Interop.Errors.ERROR_INVALID_PARAMETER)
+                        {
+                            throw Win32Marshal.GetExceptionForWin32Error(error);
+                        }
+                    }
+
+                    if (!Interop.Kernel32.SetConsoleCtrlHandler(s_handlerRoutineAddr, Add: true))
+                    {
+                        throw Win32Marshal.GetExceptionForLastWin32Error();
+                    }
                 }
 
-                tokens.Add(token);
+                lock (s_registrations)
+                {
+                    if (!s_registrations.TryGetValue(signo, out List<Token>? tokens))
+                    {
+                        s_registrations[signo] = tokens = new List<Token>();
+                    }
+
+                    tokens.Add(token);
+                }
             }
 
             return registration;
         }
 
-        private unsafe void Unregister()
+        private void Unregister()
         {
             lock (s_registrations)
             {
@@ -58,19 +100,6 @@ private unsafe void Unregister()
                         {
                             s_registrations.Remove(token.SigNo);
                         }
-
-                        if (s_registrations.Count == 0 &&
-                            !Interop.Kernel32.SetConsoleCtrlHandler(&HandlerRoutine, Add: false))
-                        {
-                            // Ignore errors due to the handler no longer being registered; this can happen, for example, with
-                            // direct use of Alloc/Attach/FreeConsole which result in the table of control handlers being reset.
-                            // Throw for everything else.
-                            int error = Marshal.GetLastPInvokeError();
-                            if (error != Interop.Errors.ERROR_INVALID_PARAMETER)
-                            {
-                                throw Win32Marshal.GetExceptionForWin32Error(error);
-                            }
-                        }
                     }
                 }
             }