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

@@ -161,6 +161,98 @@ public void TestCreateNewProcessGroup_HandlerReceivesExpectedSignal(PosixSignal
             }
         }
 
+        [ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
+        [MemberData(nameof(SignalTestData))]
+        public void SignalHandler_CanDisposeInHandler(PosixSignal signal)
+        {
+            const string PosixSignalRegistrationCreatedMessage = "PosixSignalRegistration created...";
+            const string PosixSignalHandlerStartedMessage = "PosixSignalHandlerStartedMessage created...";
+            const string PosixSignalHandlerDisposedMessage = "PosixSignalHandlerDisposedMessage created...";
+
+            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(1000);
+
+                    // If we did not terminated yet, return failure exit code
+                    return -1;
+                },
+                arg: $"{signal}",
+                remoteInvokeOptions);
+
+            while (!remoteHandle.Process.StandardOutput.ReadLine().EndsWith(PosixSignalRegistrationCreatedMessage))
+            {
+                Thread.Sleep(20);
+            }
+
+            try
+            {
+                SendSignal(signal, remoteHandle.Process.Id);
+
+                while (!remoteHandle.Process.StandardOutput.ReadLine().EndsWith(PosixSignalHandlerStartedMessage))
+                {
+                    Thread.Sleep(20);
+                }
+
+                while (!remoteHandle.Process.StandardOutput.ReadLine().EndsWith(PosixSignalHandlerDisposedMessage))
+                {
+                    Thread.Sleep(20);
+                }
+
+                SendSignal(signal, 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
+                {
+                    Assert.Equal(0, remoteHandle.Process.ExitCode);
+                }
+            }
+            finally
+            {
+                // If sending the signal fails, 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,17 @@ public sealed partial class PosixSignalRegistration
     {
         private static readonly Dictionary<int, List<Token>> s_registrations = new();
 
+        /// <summary>
+        /// Lock object used to serialize registration changes that affect calls to SetConsoleCtrlHandler.
+        /// </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 +35,53 @@ 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))
+                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 +96,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);
-                            }
-                        }
                     }
                 }
             }