-
Notifications
You must be signed in to change notification settings - Fork 2
Description
The problem
This is a public API in ipc::util:
bool process_running(process_id_t process_id)
{
using boost::system::system_category;
using ::kill;
// using ::errno; // It's a macro apparently.
// The below, down to the impl, is described in the doc header. Note these are POSIX semantics.
#ifndef FLOW_OS_LINUX
static_assert(false, "Using POSIX ::kill(pid, 0) semantics to check whether process running. "
"Have not coded Windows equivalent yet. Should work in non-Linux POSIX OS but "
"must be checked/tested.");
#endif
if (kill(process_id, 0) == 0)
{
return true;
}
// else
assert((Error_code{errno, system_category()} == boost::system::errc::no_such_process)
&& "The fake signal zero is valid and is not actually sent so no permission problem possible; hence "
"ESRCH or equivalent must be the only possible error.");
return false;
} // process_running()
The assert() that explodes if errno is not ESRCH -- effectively, in modern Linux on EPERM -- is wrong. I must have gotten some faulty info somewhere and failed to carefully read Linux man 2 kill (mea culpa... sloppy work, hopefully not typical of me). So:
- If target PID "exists" but has dif UID from us, and we are not privileged, then it'll explode; or if asserts are off return false (does-not-exist, the opposite of true).
How to fix
This stuff if surprisingly subtle... but ignoring various exoticisms, both 0 and -1/EPERM should be treated as return true, and -1/ESRCH should be treated as return false. So remove the assert() and return true at the end, essentially. (Or equivalent. assert() on non-ESRCH/EPERM still = fine.)
Impact
There are 2 potential consumers of this API. One is inside Flow-IPC; it's SHM-related cleanup code. Fortunately in that case it is invoked in a particular context where the PID in-question would have been from the same application as the caller. So, at it stands, no impact.
The other is, hypothetically, a Flow-IPC user could use this API just because. It's public after all, and why shouldn't it be? So, some impact, but the chances as of this writing of someone relying on this aren't that high. Not zero though.
In the future, depending on who uses it and how, it could be quite problematic.
Priority
So, should fix ASAP. Actually, I was working on something that would get broken by this bug.