posix: implement pid == -1 and pid == 0 cases for kill#808
Conversation
Send signal to all processes in the caller's process group when pid == 0, per POSIX.1-2017.
Send signal to all processes the caller has permission to signal when pid == -1, per POSIX.1-2017. Note: matches existing posix_killGroup behavior by not performing additional permission checks, since the kernel does not currently track per-process permissions for signaling. Fixes: phoenix-rtos/phoenix-rtos-project#1692
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements handling for pid == 0 and pid == -1 in posix_tkill, introducing a new helper function posix_killAll to broadcast signals to all processes. Feedback on this change highlights a critical issue in posix_killAll: it should exclude PID 1 (init) and the calling process itself from the signal broadcast to prevent system crashes and premature termination of the caller.
| static int posix_killAll(int sig) | ||
| { | ||
| process_info_t *pinfo; | ||
| rbnode_t *node; | ||
|
|
||
| (void)proc_lockSet(&posix_common.lock); | ||
| for (node = lib_rbMinimum(posix_common.pid.root); node != NULL; node = lib_rbNext(node)) { | ||
| pinfo = lib_treeof(process_info_t, linkage, node); | ||
| (void)proc_sigpost(pinfo->process, sig); | ||
| } | ||
| (void)proc_lockClear(&posix_common.lock); | ||
|
|
||
| return EOK; | ||
| } |
There was a problem hiding this comment.
According to POSIX.1-2017, when pid == -1, the signal should be sent to all processes for which the calling process has permission, excluding system processes. In standard implementations (like Linux/BSD), this excludes init (PID 1) and the calling process itself. Sending a terminating signal (like SIGKILL or SIGTERM) to init will crash the operating system, and sending it to the calling process itself early in the loop will terminate it before it can finish broadcasting the signal to other processes. We should exclude PID 1 and the calling process from the signal broadcast.
static int posix_killAll(int sig)
{
process_info_t *pinfo;
rbnode_t *node;
pid_t self_pid = process_getPid(proc_current()->process);
(void)proc_lockSet(&posix_common.lock);
for (node = lib_rbMinimum(posix_common.pid.root); node != NULL; node = lib_rbNext(node)) {
pinfo = lib_treeof(process_info_t, linkage, node);
if (pinfo->process != 1 && pinfo->process != self_pid) {
(void)proc_sigpost(pinfo->process, sig);
}
}
(void)proc_lockClear(&posix_common.lock);
return EOK;
}There was a problem hiding this comment.
Good catch, thanks — fixed in the latest commit. Excluded both PID 1 and the caller's own pid from the broadcast.
|
@julianuziemblo This is ready for review. Since I don't have permission to assign, tagging you here per your earlier note on #1692. |
Per review feedback, exclude PID 1 (init) and the calling process from the pid == -1 broadcast in posix_killAll. Signaling init could destabilize the system, and signaling the caller early in the loop could terminate it before the remaining processes are signaled.
| /* Handle pid == 0: send signal to all processes in caller's process group */ | ||
| if (pid == 0) { | ||
| return -ENOSYS; | ||
| process_info_t *pinfo; |
There was a problem hiding this comment.
As you can see in the rest of this submodule, our guidelines recommend defining variables at the beginning of the functions.
| { | ||
| process_info_t *pinfo; | ||
| rbnode_t *node; | ||
| pid_t self_pid = process_getPid(proc_current()->process); |
There was a problem hiding this comment.
| return EOK; | ||
| } | ||
|
|
||
| static int posix_killAll(int sig) |
There was a problem hiding this comment.
I would include the fact that it leaves current process alive in the name or at least in the comment. You could also specify that this is a posix requirement
| @@ -2533,6 +2533,23 @@ static int posix_killGroup(pid_t pgid, int sig) | |||
| return EOK; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
We do leave 2 empty lines between functions, please keep that style.
etiaro
left a comment
etiaro
left a comment
There was a problem hiding this comment.
The logic looks fine, I have a few minor comments to the style.
It would also be nice if you squashed your commits into one (we do not squash your commits on merge).
Also, we have a rule about comments saying "why" not "what", in this case you can change them to say that this is how POSIX specifies kill functions, or simply remove them.
| (void)proc_lockClear(&posix_common.lock); | ||
|
|
||
| return EOK; | ||
| } |
There was a problem hiding this comment.
Those 2 functions (posix_killAll and posix_killGroup) are essentially the same, the only difference seems to be the condition on which we sigpost. Consider merging them into one with e.g. an enum value (tkill_group, tkill_all) or other similar solution.
3 participants
Description
Implements the
pid == -1andpid == 0cases forkill/tkillper POSIX.1-2017:pid == 0: sends the signal to all processes sharing the caller's process group (reuses existingposix_killGroup).pid == -1: sends the signal to all processes (newposix_killAllfunction, following the same loop pattern asposix_killGroup).Note: neither case performs additional permission checks beyond what
posix_killGroupalready does, since the kernel doesn't currently track per-process signal permissions. Happy to adjust if you'd like something different here.Motivation and Context
Previously,
kill(0, sig)returned-ENOSYSandkill(-1, sig)returned-ESRCH, both were unimplemented (marked with a TODO in the code) rather than following POSIX-defined behavior.Fixes #1692
Types of changes
How Has This Been Tested?
Checklist:
Special treatment