kill: killing a zombie process should fail

A process which has exited may still have its process handle
held open by its children.  Such a process doesn't appear in
the process table.  It is thus similar to a zombie process in
UNIX.  Using kill(1) to interact with such a process was seen
to succeed, contrary to expectation.

The code for "ordinary" signals in kill(2) did check if the
process was still active but didn't treat an attempt to kill
an inactive process as an error.  Furthermore, sending SIGKILL
or the fake signal 0 to a process didn't even check if the
process was still active.

Rearrange the implementation of kill(2) so that an attempt to
signal an inactive process is treated as an error.  This also
consolidates handling of SIGKILL and signal 0 with "ordinary"
signals.

Saves 96 bytes.

(GitHub issue #416)

3 files changed, 101 insertions, 114 deletions

diff --git a/coreutils/timeout.c b/coreutils/timeout.c
index ff58a753a..802ddfc07 100644
--- a/coreutils/timeout.c
+++ b/coreutils/timeout.c
@@ -54,7 +54,9 @@ static HANDLE child = INVALID_HANDLE_VALUE;
 static void kill_child(void)
 {
         if (child != INVALID_HANDLE_VALUE) {
-                kill_signal_by_handle(child, SIGTERM);
+                pid_t pid = (pid_t)GetProcessId(child);
+                if (pid)
+                        kill(pid, SIGTERM);
         }
 }
 
@@ -206,13 +208,15 @@ int timeout_main(int argc UNUSED_PARAM, char **argv)
         status = signo == SIGKILL ? 137 : 124;
 
         pid = (pid_t)GetProcessId(child);
-        kill(pid, signo);
+        if (pid) {
+                kill(pid, signo);
 
-        if (kill_timeout > 0) {
-                if (timeout_wait(kill_timeout, child, &status))
-                        goto finish;
-                kill(pid, SIGKILL);
-                status = 137;
+                if (kill_timeout > 0) {
+                        if (timeout_wait(kill_timeout, child, &status))
+                                goto finish;
+                        kill(pid, SIGKILL);
+                        status = 137;
+                }
         }
  finish:
         CloseHandle(child);
diff --git a/include/mingw.h b/include/mingw.h
index 41d4cd940..56a7285e5 100644
--- a/include/mingw.h
+++ b/include/mingw.h
@@ -569,7 +569,6 @@ int mingw_execve(const char *cmd, char *const *argv, char *const *envp);
 #define has_dos_drive_prefix(path) (isalpha(*(path)) && (path)[1] == ':')
 
 BOOL WINAPI kill_child_ctrl_handler(DWORD dwCtrlType);
-int kill_signal_by_handle(HANDLE process, int sig);
 int FAST_FUNC is_valid_signal(int number);
 int exit_code_to_wait_status(DWORD win_exit_code);
 int exit_code_to_posix(DWORD win_exit_code);
diff --git a/win32/process.c b/win32/process.c
index 5be07e54a..b993bb2be 100644
--- a/win32/process.c
+++ b/win32/process.c
@@ -685,14 +685,98 @@ void FAST_FUNC read_cmdline(char *buf, int col, unsigned pid, const char *comm)
 }
 
 /**
+ * Determine whether a process runs in the same architecture as the current
+ * one. That test is required before we assume that GetProcAddress() returns
+ * a valid address *for the target process*.
+ */
+static inline int process_architecture_matches_current(HANDLE process)
+{
+        static BOOL current_is_wow = -1;
+        BOOL is_wow;
+
+        if (current_is_wow == -1 &&
+            !IsWow64Process (GetCurrentProcess(), &current_is_wow))
+                current_is_wow = -2;
+        if (current_is_wow == -2)
+                return 0; /* could not determine current process' WoW-ness */
+        if (!IsWow64Process (process, &is_wow))
+                return 0; /* cannot determine */
+        return is_wow == current_is_wow;
+}
+
+/**
+ * This function tries to terminate a Win32 process, as gently as possible,
+ * by injecting a thread that calls ExitProcess().
+ *
+ * Note: as kernel32.dll is loaded before any process, the other process and
+ * this process will have ExitProcess() at the same address.
+ *
+ * The idea comes from the Dr Dobb's article "A Safer Alternative to
+ * TerminateProcess()" by Andrew Tucker (July 1, 1999),
+ * http://www.drdobbs.com/a-safer-alternative-to-terminateprocess/184416547
+ *
+ */
+static int kill_signal_by_handle(HANDLE process, int sig)
+{
+        DECLARE_PROC_ADDR(DWORD, ExitProcess, LPVOID);
+        PVOID arg = (PVOID)(intptr_t)(sig << 24);
+        DWORD thread_id;
+        HANDLE thread;
+
+        if (!INIT_PROC_ADDR(kernel32, ExitProcess) ||
+                        !process_architecture_matches_current(process)) {
+                SetLastError(ERROR_ACCESS_DENIED);
+                return -1;
+        }
+
+        if (sig != 0 && (thread = CreateRemoteThread(process, NULL, 0,
+                                           ExitProcess, arg, 0, &thread_id))) {
+                CloseHandle(thread);
+        }
+        return 0;
+}
+
+static int kill_signal(pid_t pid, int sig)
+{
+        HANDLE process;
+        int ret = 0;
+        DWORD code;
+
+        if (sig == SIGKILL)
+                process = OpenProcess(PROCESS_TERMINATE, FALSE, pid);
+        else
+                process = OpenProcess(SYNCHRONIZE | PROCESS_CREATE_THREAD |
+                        PROCESS_QUERY_INFORMATION |
+                        PROCESS_VM_OPERATION | PROCESS_VM_WRITE |
+                        PROCESS_VM_READ, FALSE, pid);
+
+        if (!process)
+                return -1;
+
+        if (!GetExitCodeProcess(process, &code) || code != STILL_ACTIVE) {
+                SetLastError(ERROR_INVALID_PARAMETER);
+                ret = -1;
+        } else if (sig == SIGKILL) {
+                /* This way of terminating processes is not gentle: they get no
+                 * chance to clean up after themselves (closing file handles,
+                 * removing .lock files, terminating spawned processes (if any),
+                 * etc). */
+                ret = !TerminateProcess(process, SIGKILL << 24);
+        } else {
+                ret = kill_signal_by_handle(process, sig);
+        }
+        CloseHandle(process);
+
+        return ret;
+}
+
+/**
  * If the process ID is positive invoke the callback for that process
  * only.  If negative or zero invoke the callback for all descendants
  * of the indicated process.  Zero indicates the current process; negative
  * indicates the process with process ID -pid.
  */
-typedef int (*kill_callback)(pid_t pid, int sig);
-
-static int kill_pids(pid_t pid, int sig, kill_callback killer)
+static int kill_pids(pid_t pid, int sig)
 {
         DWORD pids[16384];
         int max_len = sizeof(pids) / sizeof(*pids), i, len, ret = 0;
@@ -750,7 +834,7 @@ static int kill_pids(pid_t pid, int sig, kill_callback killer)
 
         for (i = len - 1; i >= 0; i--) {
                 SetLastError(0);
-                if (killer(pids[i], sig)) {
+                if (kill_signal(pids[i], sig)) {
                         errno = err_win_to_posix();
                         ret = -1;
                 }
@@ -759,104 +843,6 @@ static int kill_pids(pid_t pid, int sig, kill_callback killer)
         return ret;
 }
 
-/**
- * Determine whether a process runs in the same architecture as the current
- * one. That test is required before we assume that GetProcAddress() returns
- * a valid address *for the target process*.
- */
-static inline int process_architecture_matches_current(HANDLE process)
-{
-        static BOOL current_is_wow = -1;
-        BOOL is_wow;
-
-        if (current_is_wow == -1 &&
-            !IsWow64Process (GetCurrentProcess(), &current_is_wow))
-                current_is_wow = -2;
-        if (current_is_wow == -2)
-                return 0; /* could not determine current process' WoW-ness */
-        if (!IsWow64Process (process, &is_wow))
-                return 0; /* cannot determine */
-        return is_wow == current_is_wow;
-}
-
-/**
- * This function tries to terminate a Win32 process, as gently as possible,
- * by injecting a thread that calls ExitProcess().
- *
- * Note: as kernel32.dll is loaded before any process, the other process and
- * this process will have ExitProcess() at the same address.
- *
- * The idea comes from the Dr Dobb's article "A Safer Alternative to
- * TerminateProcess()" by Andrew Tucker (July 1, 1999),
- * http://www.drdobbs.com/a-safer-alternative-to-terminateprocess/184416547
- *
- */
-int kill_signal_by_handle(HANDLE process, int sig)
-{
-        DWORD code;
-        int ret = 0;
-
-        if (GetExitCodeProcess(process, &code) && code == STILL_ACTIVE) {
-                DECLARE_PROC_ADDR(DWORD, ExitProcess, LPVOID);
-                PVOID arg = (PVOID)(intptr_t)(sig << 24);
-                DWORD thread_id;
-                HANDLE thread;
-
-                if (!INIT_PROC_ADDR(kernel32, ExitProcess) ||
-                                !process_architecture_matches_current(process)) {
-                        SetLastError(ERROR_ACCESS_DENIED);
-                        ret = -1;
-                        goto finish;
-                }
-
-                if ((thread = CreateRemoteThread(process, NULL, 0,
-                                            ExitProcess, arg, 0, &thread_id))) {
-                        CloseHandle(thread);
-                }
-        }
-
- finish:
-        CloseHandle(process);
-        return ret;
-}
-
-static int kill_signal(pid_t pid, int sig)
-{
-        HANDLE process;
-
-        if (!(process = OpenProcess(SYNCHRONIZE | PROCESS_CREATE_THREAD |
-                        PROCESS_QUERY_INFORMATION |
-                        PROCESS_VM_OPERATION | PROCESS_VM_WRITE |
-                        PROCESS_VM_READ, FALSE, pid))) {
-                return -1;
-        }
-
-        return kill_signal_by_handle(process, sig);
-}
-
-/*
- * This way of terminating processes is not gentle: they get no chance to
- * clean up after themselves (closing file handles, removing .lock files,
- * terminating spawned processes (if any), etc).
- *
- * If the signal isn't SIGKILL just check if the target process exists.
- */
-static int kill_SIGKILL(pid_t pid, int sig)
-{
-        HANDLE process;
-        int ret = 0;
-
-        if (!(process=OpenProcess(PROCESS_TERMINATE, FALSE, pid))) {
-                return -1;
-        }
-
-        if (sig == SIGKILL)
-                ret = !TerminateProcess(process, SIGKILL << 24);
-        CloseHandle(process);
-
-        return ret;
-}
-
 int FAST_FUNC is_valid_signal(int number)
 {
         return isalpha(*get_signame(number));
@@ -897,10 +883,8 @@ int exit_code_to_posix(DWORD exit_code)
 
 int kill(pid_t pid, int sig)
 {
-        if (sig == SIGKILL || sig == 0)
-                return kill_pids(pid, sig, kill_SIGKILL);
-        else if (is_valid_signal(sig))
-                return kill_pids(pid, sig, kill_signal);
+        if (is_valid_signal(sig))
+                return kill_pids(pid, sig);
 
         errno = EINVAL;
         return -1;