kill: kill process group with negative id

[tertsdiepraam]

Fixes #3497

I think this fixes the issue, but I have to admit I'm a bit out of my depth here. I refactored the code to use nix instead of libc, removing the need for unsafe.

@353fc443 It would be great if you could check whether this does what you expected!

Edit: I could also use some help with writing a test case for this.

[353fc443]

Yeah @tertsdiepraam That does what I expected.

Edit: I will try to add a test

[353fc443]

I added this code for testing

#[test]
fn test_kill_with_negative_pid_for_groups() {
    let command = Command::new("sh")
        .arg("-c")
        .arg("yes > /dev/null")
        .spawn()
        .unwrap();
    let pid = command.id() as i32 * -1;
    new_ucmd!()
        .arg("-s")
        .arg("9")
        .arg(pid.to_string())
        .succeeds();
}

but I'm getting this error

---- test_kill::test_kill_with_negative_pid_for_groups stdout ----
current_directory_resolved:
run: /Users/user/coreutils/target/debug/coreutils kill -s 9 -5404
thread 'test_kill::test_kill_with_negative_pid_for_groups' panicked at 'Command was expected to succeed.
stdout =
 stderr = kill: sending signal to -5404 failed: No such process
', tests/common/util.rs:174:9

even though the code seems to work in shell

Edit: the parent created through cargo test in above test cannot be killed even using GNU kill -9 -pid

[tertsdiepraam]

@353fc443 Thank you for checking and writing the test! The failing test probably has something to do with the fact that the command is a subprocess of the Rust process and therefore doesn't have its own process group? In that case I'm not sure we can actually test this.

[353fc443]

I think we can use setpgid syscall for setting a different process group for the process.

[tertsdiepraam]

I tried the following very naive test (using nix's setpgid):

#[test]
fn test_kill_with_negative_pid_for_groups() {
    let command = Command::new("sh")
        .arg("-c")
        .arg("yes > /dev/null")
        .spawn()
        .unwrap();
    
    let pid = command.id() as i32;
    setpgid(nix::unistd::Pid::from_raw(pid), nix::unistd::Pid::from_raw(pid)).unwrap();

    new_ucmd!()
        .arg("-s")
        .arg("9")
        .arg(format!("-{}", pid))
        .succeeds();
}

And it kinda works like half of the time, but the other half it panics with:

panicked at 'called `Result::unwrap()` on an `Err` value: EACCES'

at the unwrap call in the test. So it looks like we're on the right track :)

Any idea what might be wrong here?

[353fc443]

I was also facing the same issue

ERRORS
The setpgid() function will fail if:
[EACCES]
The value of the pid argument matches the process ID of a child process of the calling process and the child process has successfully executed one of the exec functions.

[sylvestre]

@tertsdiepraam you gave up on the test ? :)

[tertsdiepraam]

@sylvestre Yes, I really don't know how to write one that works.

[pro465]

how about trying a command that does (or atleast should) not call exec?

#[test]
fn test_kill_with_negative_pid_for_groups() {
    let command = Command::new("yes")
        .stdout(Stdio::null())
        .spawn()
        .unwrap();
    
    let pid = command.id() as i32;
    setpgid(nix::unistd::Pid::from_raw(pid), nix::unistd::Pid::from_raw(pid)).unwrap();

    new_ucmd!()
        .arg("-s")
        .arg("9")
        .arg(format!("-{}", pid))
        .succeeds();
}

[tertsdiepraam]

@pro465 I just tried it and it's the same problem sadly. Thanks for the suggestion though! @sylvestre I think we should merge this without a test.