bpo-41686: Refactor signal_exec() (GH-23346)

* Add signal_add_constants() function and add ADD_INT_MACRO macro.
* The Python SIGINT handler is now installed at the end of
  signal_exec().
* Use Py_NewRef().

1 files changed, 125 insertions, 158 deletions

diff --git a/Modules/signalmodule.c b/Modules/signalmodule.c
index 7cd6666..955d4a5 100644
--- a/Modules/signalmodule.c
+++ b/Modules/signalmodule.c
@@ -248,10 +248,6 @@ report_wakeup_send_error(void* data)
 static void
 trip_signal(int sig_num)
 {
-    unsigned char byte;
-    int fd;
-    Py_ssize_t rc;
-
     _Py_atomic_store_relaxed(&Handlers[sig_num].tripped, 1);
 
     /* Set is_tripped after setting .tripped, as it gets
@@ -283,6 +279,7 @@ trip_signal(int sig_num)
        See bpo-30038 for more details.
     */
 
+    int fd;
 #ifdef MS_WINDOWS
     fd = Py_SAFE_DOWNCAST(wakeup.fd, SOCKET_T, int);
 #else
@@ -290,10 +287,10 @@ trip_signal(int sig_num)
 #endif
 
     if (fd != INVALID_FD) {
-        byte = (unsigned char)sig_num;
+        unsigned char byte = (unsigned char)sig_num;
 #ifdef MS_WINDOWS
         if (wakeup.use_send) {
-            rc = send(fd, &byte, 1, 0);
+            Py_ssize_t rc = send(fd, &byte, 1, 0);
 
             if (rc < 0) {
                 int last_error = GetLastError();
@@ -313,7 +310,7 @@ trip_signal(int sig_num)
         {
             /* _Py_write_noraise() retries write() if write() is interrupted by
                a signal (fails with EINTR). */
-            rc = _Py_write_noraise(fd, &byte, 1);
+            Py_ssize_t rc = _Py_write_noraise(fd, &byte, 1);
 
             if (rc < 0) {
                 if (wakeup.warn_on_full_buffer ||
@@ -516,8 +513,7 @@ signal_signal_impl(PyObject *module, int signalnum, PyObject *handler)
     }
 
     old_handler = Handlers[signalnum].func;
-    Py_INCREF(handler);
-    Handlers[signalnum].func = handler;
+    Handlers[signalnum].func = Py_NewRef(handler);
 
     if (old_handler != NULL) {
         return old_handler;
@@ -555,8 +551,7 @@ signal_getsignal_impl(PyObject *module, int signalnum)
     }
     old_handler = Handlers[signalnum].func;
     if (old_handler != NULL) {
-        Py_INCREF(old_handler);
-        return old_handler;
+        return Py_NewRef(old_handler);
     }
     else {
         Py_RETURN_NONE;
@@ -711,7 +706,7 @@ signal_set_wakeup_fd(PyObject *self, PyObject *args, PyObject *kwds)
     if (sockfd == (SOCKET_T)(-1) && PyErr_Occurred())
         return NULL;
 #else
-    int fd, old_fd;
+    int fd;
 
     if (!PyArg_ParseTupleAndKeywords(args, kwds, "i|$p:set_wakeup_fd", kwlist,
                                      &fd, &warn_on_full_buffer))
@@ -793,7 +788,7 @@ signal_set_wakeup_fd(PyObject *self, PyObject *args, PyObject *kwds)
         }
     }
 
-    old_fd = wakeup.fd;
+    int old_fd = wakeup.fd;
     wakeup.fd = fd;
     wakeup.warn_on_full_buffer = warn_on_full_buffer;
 
@@ -814,14 +809,14 @@ The fd must be non-blocking.");
 int
 PySignal_SetWakeupFd(int fd)
 {
-    int old_fd;
-    if (fd < 0)
+    if (fd < 0) {
         fd = -1;
+    }
 
 #ifdef MS_WINDOWS
-    old_fd = Py_SAFE_DOWNCAST(wakeup.fd, SOCKET_T, int);
+    int old_fd = Py_SAFE_DOWNCAST(wakeup.fd, SOCKET_T, int);
 #else
-    old_fd = wakeup.fd;
+    int old_fd = wakeup.fd;
 #endif
     wakeup.fd = fd;
     wakeup.warn_on_full_buffer = 1;
@@ -852,7 +847,7 @@ signal_setitimer_impl(PyObject *module, int which, PyObject *seconds,
                       PyObject *interval)
 /*[clinic end generated code: output=65f9dcbddc35527b input=de43daf194e6f66f]*/
 {
-    struct itimerval new, old;
+    struct itimerval new;
 
     if (timeval_from_double(seconds, &new.it_value) < 0) {
         return NULL;
@@ -862,6 +857,7 @@ signal_setitimer_impl(PyObject *module, int which, PyObject *seconds,
     }
 
     /* Let OS check "which" value */
+    struct itimerval old;
     if (setitimer(which, &new, &old) != 0) {
         PyErr_SetFromErrno(ItimerError);
         return NULL;
@@ -1380,251 +1376,222 @@ the first is the signal number, the second is the interrupted stack frame.");
 
 
 static int
-signal_exec(PyObject *m)
+signal_add_constants(PyObject *module)
 {
-    /* add the functions */
-#if defined(HAVE_SIGWAITINFO) || defined(HAVE_SIGTIMEDWAIT)
-    if (PyModule_AddType(m, &SiginfoType) < 0) {
-        return -1;
-    }
-#endif
-
-    /* Add some symbolic constants to the module */
-    PyObject *d = PyModule_GetDict(m);
-
-    if (PyDict_SetItemString(d, "SIG_DFL", DefaultHandler) < 0) {
-        return -1;
-    }
-
-    if (PyDict_SetItemString(d, "SIG_IGN", IgnoreHandler) < 0) {
-        return -1;
+#define ADD_INT_MACRO(macro) \
+    if (PyModule_AddIntConstant(module, #macro, macro) < 0) { \
+        return -1; \
     }
 
-    if (PyModule_AddIntMacro(m, NSIG))
-        return -1;
+    ADD_INT_MACRO(NSIG);
 
+    // SIG_xxx pthread_sigmask() constants
 #ifdef SIG_BLOCK
-    if (PyModule_AddIntMacro(m, SIG_BLOCK))
-         return -1;
+    ADD_INT_MACRO(SIG_BLOCK);
 #endif
 #ifdef SIG_UNBLOCK
-    if (PyModule_AddIntMacro(m, SIG_UNBLOCK))
-         return -1;
+    ADD_INT_MACRO(SIG_UNBLOCK);
 #endif
 #ifdef SIG_SETMASK
-    if (PyModule_AddIntMacro(m, SIG_SETMASK))
-         return -1;
+    ADD_INT_MACRO(SIG_SETMASK);
 #endif
 
-    for (int i = 1; i < NSIG; i++) {
-        void (*t)(int);
-        t = PyOS_getsig(i);
-        if (t == SIG_DFL)
-            Handlers[i].func = DefaultHandler;
-        else if (t == SIG_IGN)
-            Handlers[i].func = IgnoreHandler;
-        else
-            Handlers[i].func = Py_None; /* None of our business */
-        Py_INCREF(Handlers[i].func);
-    }
-    if (Handlers[SIGINT].func == DefaultHandler) {
-        PyObject *int_handler = PyMapping_GetItemString(d, "default_int_handler");
-        if (!int_handler) {
-            return -1;
-        }
-
-        /* Install default int handler */
-        Py_SETREF(Handlers[SIGINT].func, int_handler);
-        PyOS_setsig(SIGINT, signal_handler);
-    }
-
+    // SIGxxx signal number constants
 #ifdef SIGHUP
-    if (PyModule_AddIntMacro(m, SIGHUP))
-         return -1;
+    ADD_INT_MACRO(SIGHUP);
 #endif
 #ifdef SIGINT
-    if (PyModule_AddIntMacro(m, SIGINT))
-         return -1;
+    ADD_INT_MACRO(SIGINT);
 #endif
 #ifdef SIGBREAK
-    if (PyModule_AddIntMacro(m, SIGBREAK))
-         return -1;
+    ADD_INT_MACRO(SIGBREAK);
 #endif
 #ifdef SIGQUIT
-    if (PyModule_AddIntMacro(m, SIGQUIT))
-         return -1;
+    ADD_INT_MACRO(SIGQUIT);
 #endif
 #ifdef SIGILL
-    if (PyModule_AddIntMacro(m, SIGILL))
-         return -1;
+    ADD_INT_MACRO(SIGILL);
 #endif
 #ifdef SIGTRAP
-    if (PyModule_AddIntMacro(m, SIGTRAP))
-         return -1;
+    ADD_INT_MACRO(SIGTRAP);
 #endif
 #ifdef SIGIOT
-    if (PyModule_AddIntMacro(m, SIGIOT))
-         return -1;
+    ADD_INT_MACRO(SIGIOT);
 #endif
 #ifdef SIGABRT
-    if (PyModule_AddIntMacro(m, SIGABRT))
-         return -1;
+    ADD_INT_MACRO(SIGABRT);
 #endif
 #ifdef SIGEMT
-    if (PyModule_AddIntMacro(m, SIGEMT))
-         return -1;
+    ADD_INT_MACRO(SIGEMT);
 #endif
 #ifdef SIGFPE
-    if (PyModule_AddIntMacro(m, SIGFPE))
-         return -1;
+    ADD_INT_MACRO(SIGFPE);
 #endif
 #ifdef SIGKILL
-    if (PyModule_AddIntMacro(m, SIGKILL))
-         return -1;
+    ADD_INT_MACRO(SIGKILL);
 #endif
 #ifdef SIGBUS
-    if (PyModule_AddIntMacro(m, SIGBUS))
-         return -1;
+    ADD_INT_MACRO(SIGBUS);
 #endif
 #ifdef SIGSEGV
-    if (PyModule_AddIntMacro(m, SIGSEGV))
-         return -1;
+    ADD_INT_MACRO(SIGSEGV);
 #endif
 #ifdef SIGSYS
-    if (PyModule_AddIntMacro(m, SIGSYS))
-         return -1;
+    ADD_INT_MACRO(SIGSYS);
 #endif
 #ifdef SIGPIPE
-    if (PyModule_AddIntMacro(m, SIGPIPE))
-         return -1;
+    ADD_INT_MACRO(SIGPIPE);
 #endif
 #ifdef SIGALRM
-    if (PyModule_AddIntMacro(m, SIGALRM))
-         return -1;
+    ADD_INT_MACRO(SIGALRM);
 #endif
 #ifdef SIGTERM
-    if (PyModule_AddIntMacro(m, SIGTERM))
-         return -1;
+    ADD_INT_MACRO(SIGTERM);
 #endif
 #ifdef SIGUSR1
-    if (PyModule_AddIntMacro(m, SIGUSR1))
-         return -1;
+    ADD_INT_MACRO(SIGUSR1);
 #endif
 #ifdef SIGUSR2
-    if (PyModule_AddIntMacro(m, SIGUSR2))
-         return -1;
+    ADD_INT_MACRO(SIGUSR2);
 #endif
 #ifdef SIGCLD
-    if (PyModule_AddIntMacro(m, SIGCLD))
-         return -1;
+    ADD_INT_MACRO(SIGCLD);
 #endif
 #ifdef SIGCHLD
-    if (PyModule_AddIntMacro(m, SIGCHLD))
-         return -1;
+    ADD_INT_MACRO(SIGCHLD);
 #endif
 #ifdef SIGPWR
-    if (PyModule_AddIntMacro(m, SIGPWR))
-         return -1;
+    ADD_INT_MACRO(SIGPWR);
 #endif
 #ifdef SIGIO
-    if (PyModule_AddIntMacro(m, SIGIO))
-         return -1;
+    ADD_INT_MACRO(SIGIO);
 #endif
 #ifdef SIGURG
-    if (PyModule_AddIntMacro(m, SIGURG))
-         return -1;
+    ADD_INT_MACRO(SIGURG);
 #endif
 #ifdef SIGWINCH
-    if (PyModule_AddIntMacro(m, SIGWINCH))
-         return -1;
+    ADD_INT_MACRO(SIGWINCH);
 #endif
 #ifdef SIGPOLL
-    if (PyModule_AddIntMacro(m, SIGPOLL))
-         return -1;
+    ADD_INT_MACRO(SIGPOLL);
 #endif
 #ifdef SIGSTOP
-    if (PyModule_AddIntMacro(m, SIGSTOP))
-         return -1;
+    ADD_INT_MACRO(SIGSTOP);
 #endif
 #ifdef SIGTSTP
-    if (PyModule_AddIntMacro(m, SIGTSTP))
-         return -1;
+    ADD_INT_MACRO(SIGTSTP);
 #endif
 #ifdef SIGCONT
-    if (PyModule_AddIntMacro(m, SIGCONT))
-         return -1;
+    ADD_INT_MACRO(SIGCONT);
 #endif
 #ifdef SIGTTIN
-    if (PyModule_AddIntMacro(m, SIGTTIN))
-         return -1;
+    ADD_INT_MACRO(SIGTTIN);
 #endif
 #ifdef SIGTTOU
-    if (PyModule_AddIntMacro(m, SIGTTOU))
-         return -1;
+    ADD_INT_MACRO(SIGTTOU);
 #endif
 #ifdef SIGVTALRM
-    if (PyModule_AddIntMacro(m, SIGVTALRM))
-         return -1;
+    ADD_INT_MACRO(SIGVTALRM);
 #endif
 #ifdef SIGPROF
-    if (PyModule_AddIntMacro(m, SIGPROF))
-         return -1;
+    ADD_INT_MACRO(SIGPROF);
 #endif
 #ifdef SIGXCPU
-    if (PyModule_AddIntMacro(m, SIGXCPU))
-         return -1;
+    ADD_INT_MACRO(SIGXCPU);
 #endif
 #ifdef SIGXFSZ
-    if (PyModule_AddIntMacro(m, SIGXFSZ))
-         return -1;
+    ADD_INT_MACRO(SIGXFSZ);
 #endif
 #ifdef SIGRTMIN
-    if (PyModule_AddIntMacro(m, SIGRTMIN))
-         return -1;
+    ADD_INT_MACRO(SIGRTMIN);
 #endif
 #ifdef SIGRTMAX
-    if (PyModule_AddIntMacro(m, SIGRTMAX))
-         return -1;
+    ADD_INT_MACRO(SIGRTMAX);
 #endif
 #ifdef SIGINFO
-    if (PyModule_AddIntMacro(m, SIGINFO))
-         return -1;
+    ADD_INT_MACRO(SIGINFO);
 #endif
 
+    // ITIMER_xxx constants
 #ifdef ITIMER_REAL
-    if (PyModule_AddIntMacro(m, ITIMER_REAL))
-         return -1;
+    ADD_INT_MACRO(ITIMER_REAL);
 #endif
 #ifdef ITIMER_VIRTUAL
-    if (PyModule_AddIntMacro(m, ITIMER_VIRTUAL))
-         return -1;
+    ADD_INT_MACRO(ITIMER_VIRTUAL);
 #endif
 #ifdef ITIMER_PROF
-    if (PyModule_AddIntMacro(m, ITIMER_PROF))
-         return -1;
+    ADD_INT_MACRO(ITIMER_PROF);
 #endif
 
+    // CTRL_xxx Windows signals
+#ifdef CTRL_C_EVENT
+    ADD_INT_MACRO(CTRL_C_EVENT);
+#endif
+#ifdef CTRL_BREAK_EVENT
+    ADD_INT_MACRO(CTRL_BREAK_EVENT);
+#endif
+
+    return 0;
+
+#undef ADD_INT_MACRO
+}
+
+
+static int
+signal_exec(PyObject *m)
+{
+    assert(!PyErr_Occurred());
+
+    if (signal_add_constants(m) < 0) {
+        return -1;
+    }
+
+    /* Add some symbolic constants to the module */
+    PyObject *d = PyModule_GetDict(m);
+    if (PyDict_SetItemString(d, "SIG_DFL", DefaultHandler) < 0) {
+        return -1;
+    }
+    if (PyDict_SetItemString(d, "SIG_IGN", IgnoreHandler) < 0) {
+        return -1;
+    }
 #if defined(HAVE_GETITIMER) || defined(HAVE_SETITIMER)
     if (PyDict_SetItemString(d, "ItimerError", ItimerError) < 0) {
         return -1;
     }
 #endif
-
-#ifdef CTRL_C_EVENT
-    if (PyModule_AddIntMacro(m, CTRL_C_EVENT))
-         return -1;
+#if defined(HAVE_SIGWAITINFO) || defined(HAVE_SIGTIMEDWAIT)
+    if (PyModule_AddType(m, &SiginfoType) < 0) {
+        return -1;
+    }
 #endif
 
-#ifdef CTRL_BREAK_EVENT
-    if (PyModule_AddIntMacro(m, CTRL_BREAK_EVENT))
-         return -1;
-#endif
+    // Get signal handlers
+    for (int signum = 1; signum < NSIG; signum++) {
+        void (*c_handler)(int) = PyOS_getsig(signum);
+        if (c_handler == SIG_DFL) {
+            Handlers[signum].func = Py_NewRef(DefaultHandler);
+        }
+        else if (c_handler == SIG_IGN) {
+            Handlers[signum].func = Py_NewRef(IgnoreHandler);
+        }
+        else {
+            Handlers[signum].func = Py_NewRef(Py_None); // None of our business
+        }
+    }
 
-    if (PyErr_Occurred()) {
-        return -1;
+    // Instal Python SIGINT handler which raises KeyboardInterrupt
+    if (Handlers[SIGINT].func == DefaultHandler) {
+        PyObject *int_handler = PyMapping_GetItemString(d, "default_int_handler");
+        if (!int_handler) {
+            return -1;
+        }
+
+        Py_SETREF(Handlers[SIGINT].func, int_handler);
+        PyOS_setsig(SIGINT, signal_handler);
     }
 
-  return 0;
+    assert(!PyErr_Occurred());
+    return 0;
 }