Add portable wrapper for POSIX kill(2) by kinkie · Pull Request #2331 · squid-cache/squid

kinkie · 2025-12-26T15:21:25Z

Windows does not really offer kill() and signal(), only limited wrappers.
Refactor emulation code, rename kill() to xkill() to highlight it's a wrapper

yadij · 2025-12-26T19:44:18Z

compat/signal.h

+#if !(_SQUID_WINDOWS_ || _SQUID_MINGW_)
+
+inline int xkill(pid_t pid, int sig)
+{
+    return kill(pid, sig);
+}
+
+inline bool
+IsPidValid(pid_t pid)
+{
+    return kill(pid, 0) == 0;
+}
+
+#endif /* !(_SQUID_WINDOWS_ || _SQUID_MINGW_) */


Suggested change

#if !(_SQUID_WINDOWS_ || _SQUID_MINGW_)

inline int xkill(pid_t pid, int sig)

{

return kill(pid, sig);

}

inline bool

IsPidValid(pid_t pid)

{

return kill(pid, 0) == 0;

}

#endif /* !(_SQUID_WINDOWS_ || _SQUID_MINGW_) */

/// POSIX kill(2) equivalent

int xkill(pid_t, int);

#if _SQUID_WINDOWS_ || _SQUID_MINGW_

inline int xkill(pid_t pid, int sig) { return kill(pid, sig); }

#endif

yadij · 2025-12-26T20:11:03Z

compat/signal.h

+/// true if pid can be sent a signal (no signal is sent)
+inline bool
+IsPidValid(pid_t pid);
+


This is not part of the POSIX API. We should be using that API instead of local wrappers.

Suggested change

/// true if pid can be sent a signal (no signal is sent)

inline bool

IsPidValid(pid_t pid);

I agree that compat/signal.h should not provide IsPidValid() API.

Instead, xkill() implementation for Windows may (and probably should!) use a static helper function, hopefully with a more revealing name, to implement xkill(signal=0) support).

Old higher-level code should continue to use ProcessIsRunning() and raw xkill(signal=0) calls.

yadij · 2025-12-26T20:12:46Z

src/Instance.cc

 static bool
 ProcessIsRunning(const pid_t pid)
 {
-    const auto result = kill(pid, 0);


Please continue to use the POSIX API equivalent call.

const auto result = xkill(pid, 0);

yadij · 2025-12-26T20:29:35Z

compat/signal.cc

+}
+
+/// true if the given pid is the current process, false otherwise
+bool


When the caller is restored to using xkill(pid, 0) to check for validity this can be made static

Suggested change

bool

static bool

yadij · 2025-12-26T20:30:10Z

compat/signal.cc

+xkill(pid_t pid, int sig)
+{
+    auto hProcess = OpenProcess(PROCESS_QUERY_INFORMATION | PROCESS_VM_READ, FALSE, pid);
+


Suggested change

yadij · 2025-12-26T20:40:37Z

compat/signal.cc

+    char MyProcessName[MAX_PATH];
+    GetProcessName(getpid(), MyProcessName);
+    char ProcessNameToCheck[MAX_PATH];


MAX_PATH is not always a small value. Please allocate these dynamically.

yadij · 2025-12-26T20:47:02Z

compat/signal.cc

+    switch (sig) {
+// Windows does not support sending generic POSIX signals.
+// TODO: implement a signal sending and handling mechanism
+// using Windows messages
+    case SIGKILL:
+        if (TerminateProcess(hProcess, 0)) {
+            CloseHandle(hProcess);
+            return 0;
+        }
+        break;
+    case 0:
+        CloseHandle(hProcess);
+        if (IsPidValid(pid))
+            return 0;
+        return -1;
+        break;
+    default:
+        // Unsupported signal
+        CloseHandle(hProcess);
+    }
+
+    return -1;


The break; case for SIGKILL handling leaves the process handle hanging.

This whole section would be better implemented such that the handle is always closed at a single point of return.

Suggested change

switch (sig) {

// Windows does not support sending generic POSIX signals.

// TODO: implement a signal sending and handling mechanism

// using Windows messages

case SIGKILL:

if (TerminateProcess(hProcess, 0)) {

CloseHandle(hProcess);

return 0;

}

break;

case 0:

CloseHandle(hProcess);

if (IsPidValid(pid))

return 0;

return -1;

break;

default:

// Unsupported signal

CloseHandle(hProcess);

}

return -1;

// Windows does not support sending generic POSIX signals.

// TODO: implement a signal sending and handling mechanism

// using Windows messages

bool result = false;

switch (sig) {

case SIGKILL:

result = TerminateProcess(hProcess, 0);

break;

case 0:

result = IsPidValid(pid);

break;

default:

// Unsupported signal

}

CloseHandle(hProcess);

return (result ? 0 : -1);

yadij · 2025-12-26T20:53:02Z

compat/signal.h

+#ifndef SQUID_COMPAT_SIGNAL_H
+#define SQUID_COMPAT_SIGNAL_H
+
+#if !defined(SIGHUP)


Please add a reference for where these values come from.

Suggested change

#if !defined(SIGHUP)

// Signal definitions from POSIX API. see https://www.man7.org/linux/man-pages/man7/signal.7.html

// TODO: use x86 or arch-specific defines instead of these SPARC CPU values.

#if !defined(SIGHUP)

yadij · 2025-12-26T21:00:24Z

compat/signal.h

+#if !defined(WIFEXITED)
+inline int
+WIFEXITED(int status) {
+    return (status & 0x7f) == 0;
+}
+#endif
+
+#if !defined(WEXITSTATUS)
+inline int
+WEXITSTATUS(int status) {
+    return (status & 0xff00) >> 8;
+}
+#endif
+
+#if !defined(WIFSIGNALED)
+inline int
+WIFSIGNALED(int status) {
+    return (status & 0x7f) != 0;
+}
+#endif
+
+#if !defined(WTERMSIG)
+inline int
+WTERMSIG(int status) {
+    return (status & 0x7f);
+}
+#endif


All these W...() definitions are part of the POSIX sys/wait.h header. Not part of signal.h.
Please remove.

compat/signal.cc

yadij · 2025-12-26T21:18:16Z

FYI, reference fro what symbols should be in each POSIX header file can be found at https://pubs.opengroup.org/onlinepubs/9799919799/

remove redundant code Co-authored-by: Amos Jeffries <yadij@users.noreply.github.com>

rousskov

This is not a comprehensive review, but it may help you remove quite a bit of noise/distractions from this draft PR.

rousskov · 2025-12-28T02:14:41Z

compat/signal.h

+/// true if pid can be sent a signal (no signal is sent)
+inline bool
+IsPidValid(pid_t pid);
+


I agree that compat/signal.h should not provide IsPidValid() API.

Instead, xkill() implementation for Windows may (and probably should!) use a static helper function, hopefully with a more revealing name, to implement xkill(signal=0) support).

Old higher-level code should continue to use ProcessIsRunning() and raw xkill(signal=0) calls.

rousskov · 2025-12-28T02:21:41Z

compat/signal.cc

+    GetProcessName(getpid(), MyProcessName);
+    char ProcessNameToCheck[MAX_PATH];
+    GetProcessName(pid, ProcessNameToCheck);
+    if (strcmp(MyProcessName, ProcessNameToCheck) == 0)


It is not clear to me why IsPidValid(getpid()) should return false. It is usually possible to send a signal to oneself, right? Please either refactor or add a C++ comment to explain why we need to compare getpid() process name with ours.

N.B. I am aware that existing Windows code has similar logic.

P.S. I am writing this change request while assuming that this or similar function is going to be preserved despite the requested removal of "public" IsPidValid() API.

rousskov · 2025-12-28T02:23:51Z

compat/signal.cc

+static void
+GetProcessName(pid_t pid, char *ProcessName)
+{
+    strcpy(ProcessName, "unknown");


If this code survives, please return a nil name instead of using a special value to mean "we do not know what the process name is [because some system calls have failed]" (that callers do not know about!).

N.B. I am aware that official code uses this hack.

rousskov · 2025-12-28T02:37:14Z

I adjusted PR title to highlight that PR changes are going to affect all kill(2) callers, not just MinGW code.

Please note that PR description talks about signal() as well. I suggest removing that text from PR description. If we need a portable signal(3) alternative, we should probably use std::signal(), but we can make those decisions in a dedicated PR, when needed.

kinkie added 4 commits August 10, 2025 22:10

implement xkill

671698f

adopt xkill and implement auxiliary functions

e4c2e85

kill(pid, 0) -> IsPidValid(pid)

d382fea

Merge branch 'master' into update-mingw-17

6304674

kinkie marked this pull request as draft December 26, 2025 15:21

Source maintenance

2739f6c

rousskov self-requested a review December 26, 2025 19:39

yadij requested changes Dec 26, 2025

View reviewed changes

yadij reviewed Dec 26, 2025

View reviewed changes

compat/signal.cc Outdated Show resolved Hide resolved

Apply suggestion from @yadij

bea387c

remove redundant code Co-authored-by: Amos Jeffries <yadij@users.noreply.github.com>

rousskov requested changes Dec 28, 2025

View reviewed changes

rousskov changed the title ~~MinGW: refactor signal code~~ Add portable wrapper for POSIX kill(2) Dec 28, 2025

rousskov added the S-waiting-for-author author action is expected (and usually required) label Dec 28, 2025

	/// true if pid can be sent a signal (no signal is sent)
	inline bool
	IsPidValid(pid_t pid);

Conversation

kinkie commented Dec 26, 2025

Uh oh!

yadij Dec 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yadij commented Dec 26, 2025

Uh oh!

rousskov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rousskov commented Dec 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yadij Dec 26, 2025 •

edited

Loading