Only return EPERM from kill(-pid) when no process was signalled.

[kib]

Summary

As mandated by POSIX.

[AMDmi3]

• 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.

[kib]

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.

[jilles]

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.

sys/kern/kern_sig.c

1755–1756

POSIX is pretty clear that any successful signal sent (or signal that could be sent, if sig == 0) will cause kill() to succeed (not return an error). Given that if sent_one is true, ret can either be EPERM or 0 (under the assumption that p_cansignal() returns 0, EPERM or ESRCH), the part ret == EPERM && is only for special cases.

The question here is whether a "buggy" failure (such as because of an obscure edge case in locking which is not handled, or something) should be reported to the application, violating the letter of POSIX, or should be ignored if some other process was signalled, possibly confusing the application and making it hard to debug.

[kib]

Use bool controls both for EPERM and ESRCH.
More edits to manpage.

[jilles]

sys/kern/kern_sig.c

1706

found should only be set to true if p_cansignal() does not return ESRCH, so we don't admit a process exists when we should not due to restrictions like security.bsd.see_other_uids.

[kib]

sys/kern/kern_sig.c

1706

I was actually not quite sure does p_cansignal() and code which it calls like prison_<something> care about returning ESRCH when visibility is denied (vs. returning something else), but it seems fine.

[kib]

Do not set 'found' if p_cansignal() returned ESRCH.
Restructure code to unify signal sending and error calculation.
Reduce proc_lock scope.