• if the intent is to make the logic match POSIX, the manpage should be updated to exclude the mention of uid matching which contradicts both POSIX and the code
• this does not fix the original misbehavior in case of case (kill(-1, 0) with uid != 0 and no other processes with that uid):
  • since no processes are signalled, sent_one would be false
  • since there almost always are processes which cannot be signalled (e.g. ones belonging to root), ret would be EPERM, which again is returned, while it should be ESRCH instead

According to POSIX, killpg1 should never return EPERM, as all three cases mention "permission to send a signal".

If pid is 0, sig shall be sent to all processes (excluding an unspecified set of system processes) whose process group ID is equal to the process group ID of the sender, and for which the process has permission to send a signal.

If pid is -1, sig shall be sent to all processes (excluding an unspecified set of system processes) for which the process has permission to send that signal.

If pid is negative, but not -1, sig shall be sent to all processes (excluding an unspecified set of system processes) whose process group ID is equal to the absolute value of pid, and for which the process has permission to send a signal.

So it should return either ESRCH, or 0 if any process could be signalled. So the change should probably be to just remove else if branches from original code.

In D22621#495058, @AMDmi3 wrote:

• if the intent is to make the logic match POSIX, the manpage should be updated to exclude the mention of uid matching which contradicts both POSIX and the code
• this does not fix the original misbehavior in case of case (kill(-1, 0) with uid != 0 and no other processes with that uid):
  • since no processes are signalled, sent_one would be false
  • since there almost always are processes which cannot be signalled (e.g. ones belonging to root), ret would be EPERM, which again is returned, while it should be ESRCH instead

According to POSIX, killpg1 should never return EPERM, as all three cases mention "permission to send a signal".

If pid is 0, sig shall be sent to all processes (excluding an unspecified set of system processes) whose process group ID is equal to the process group ID of the sender, and for which the process has permission to send a signal.

If pid is -1, sig shall be sent to all processes (excluding an unspecified set of system processes) for which the process has permission to send that signal.

If pid is negative, but not -1, sig shall be sent to all processes (excluding an unspecified set of system processes) whose process group ID is equal to the absolute value of pid, and for which the process has permission to send a signal.

So it should return either ESRCH, or 0 if any process could be signalled. So the change should probably be to just remove else if branches from original code.

According to POSIX, both kill() and killpg() shall return EPERM, exactly when there is no a process for which permissions would allow send, but there are processes which permissions to test.

And the kill(2) manual page is actually good enough. It explains typical reasons for permission checks failure, I do not see why any of them should be removed. There might be some useful addition stating that more permission checks could be done, e.g. by a MAC module. There is even a sentence at line 109 that catches the issue I fixed in the error section.

I think the part "to all processes with the same uid as the user" is rather vague now, and would probably be better written as "to all processes which the caller has permission to", given that permission checks are quite complicated nowadays.

The [ESRCH] error seems not to be excluded when pid is negative in my reading, so the code in the review seems functionally correct.

The use of an integer that is being compared to together with a boolean may not be the clearest way to write this. Perhaps it is clearer to have a boolean indicating a signal was sent, a boolean indicating that a process was found, allowed to be visible but not to be signalled (EPERM) and an integer for weird errors (not EPERM and not ESRCH).

Our exclusion of the process itself with pid == -1 is not very clearly described in POSIX but changing that may not be a good idea.

Slightly related, the part "For compatibility with System V," about pids less than -1 can perhaps be removed as that feature is accepted as a standard feature nowadays.