fix(shell): preserved negative pid operands in kill
Restricted -sigspec parsing to the option position and consumed the -- end-of-options marker, so negative PIDs (process groups) and post-marker operands are signaled rather than parsed as signals. Added a process-group regression covering kill -TERM -- -<pgid> <pid>. Fixes #6779
This commit is contained in:
roboomp
@@ -2288,6 +2288,75 @@ mod tests {
|
||||
assert_eq!(status.expect("checked timeout").expect("child wait").code(), Some(42));
|
||||
}
|
||||
|
||||
/// A negative PID after `--` targets a process group per `kill(2)` instead
|
||||
/// of being parsed as a numeric signal, and a plain PID in the same command
|
||||
/// is still signaled.
|
||||
#[cfg(unix)]
|
||||
#[tokio::test(flavor = "multi_thread")]
|
||||
async fn kill_builtin_preserves_negative_pid_process_group_operands() {
|
||||
let dir = tempfile::tempdir().expect("temp dir");
|
||||
let group_ready = dir.path().join("group-ready");
|
||||
let plain_ready = dir.path().join("plain-ready");
|
||||
let spawn_trapping = |ready: &std::path::Path, own_group: bool| {
|
||||
let mut cmd = Command::new("sh");
|
||||
cmd.args([
|
||||
"-c",
|
||||
"trap 'exit 42' TERM; : > \"$1\"; while :; do sleep 0.05; done",
|
||||
"sh",
|
||||
ready.to_str().expect("utf8 path"),
|
||||
]);
|
||||
if own_group {
|
||||
// pgid becomes this child's own pid, so `-pid` addresses the group.
|
||||
cmd.process_group(0);
|
||||
}
|
||||
cmd.spawn().expect("trapping child")
|
||||
};
|
||||
|
||||
let mut group_child = spawn_trapping(&group_ready, true);
|
||||
let mut plain_child = spawn_trapping(&plain_ready, false);
|
||||
let group_pid = group_child.id().expect("group pid");
|
||||
let plain_pid = plain_child.id().expect("plain pid");
|
||||
|
||||
let ready_result = time::timeout(Duration::from_secs(5), async {
|
||||
while !group_ready.exists() || !plain_ready.exists() {
|
||||
time::sleep(Duration::from_millis(10)).await;
|
||||
}
|
||||
})
|
||||
.await;
|
||||
if ready_result.is_err() {
|
||||
let _ = group_child.start_kill();
|
||||
let _ = plain_child.start_kill();
|
||||
let _ = group_child.wait().await;
|
||||
let _ = plain_child.wait().await;
|
||||
panic!("children did not install their SIGTERM traps");
|
||||
}
|
||||
|
||||
let (mut session, params) = kill_test_context().await;
|
||||
let source_info = SourceInfo::from("pi-natives:test");
|
||||
let result = session
|
||||
.shell
|
||||
.run_string(format!("kill -TERM -- -{group_pid} {plain_pid}"), &source_info, ¶ms)
|
||||
.await
|
||||
.expect("kill command");
|
||||
let code = exit_code(&result);
|
||||
|
||||
let statuses = time::timeout(Duration::from_secs(5), async {
|
||||
tokio::join!(group_child.wait(), plain_child.wait())
|
||||
})
|
||||
.await;
|
||||
if statuses.is_err() {
|
||||
let _ = group_child.start_kill();
|
||||
let _ = plain_child.start_kill();
|
||||
let _ = group_child.wait().await;
|
||||
let _ = plain_child.wait().await;
|
||||
panic!("negative-PID kill must signal the process group and the plain PID");
|
||||
}
|
||||
let (group_status, plain_status) = statuses.expect("checked timeout");
|
||||
assert_eq!(code, 0, "negative-PID kill should succeed");
|
||||
assert_eq!(group_status.expect("group wait").code(), Some(42));
|
||||
assert_eq!(plain_status.expect("plain wait").code(), Some(42));
|
||||
}
|
||||
|
||||
/// `cmp` remains available with no executable search path, proving the shell
|
||||
/// dispatches the in-process builtin rather than a platform binary.
|
||||
#[tokio::test(flavor = "multi_thread")]
|
||||
|
||||
+38
-14
@@ -67,31 +67,55 @@ impl builtins::Command for KillCommand {
|
||||
}
|
||||
}
|
||||
|
||||
// Look through the remaining args for process operands or a -sigspec option.
|
||||
let mut saw_pid_or_job_spec = false;
|
||||
// Parse the remaining args: an optional leading `-sigspec` in the option
|
||||
// position, an optional `--` end-of-options marker, then pid/jobspec
|
||||
// operands. A hyphen is only a sigspec while still in the option position;
|
||||
// once a sigspec or an operand is seen, later hyphen-led args are operands
|
||||
// so negative PIDs (process groups per `kill(2)`) survive — e.g.
|
||||
// `kill -TERM -- -10 123` signals process group 10 and PID 123.
|
||||
let mut operands: Vec<&String> = Vec::new();
|
||||
let mut options_done = false;
|
||||
let mut consumed_end_of_options = false;
|
||||
for arg in &self.args {
|
||||
if let Some(possible_sigspec) = arg.strip_prefix("-") {
|
||||
// The sigspec may be a signal name (e.g. -TERM) or number (e.g. -9).
|
||||
if let Ok(parsed_trap_signal) = possible_sigspec.parse::<TrapSignal>() {
|
||||
trap_signal = parsed_trap_signal;
|
||||
} else {
|
||||
writeln!(context.stderr(), "{}: invalid signal name", context.command_name)?;
|
||||
return Ok(ExecutionExitCode::InvalidUsage.into());
|
||||
}
|
||||
} else {
|
||||
saw_pid_or_job_spec = true;
|
||||
// The first `--` ends option parsing and is not itself an operand.
|
||||
if !consumed_end_of_options && arg == "--" {
|
||||
consumed_end_of_options = true;
|
||||
options_done = true;
|
||||
continue;
|
||||
}
|
||||
if options_done {
|
||||
operands.push(arg);
|
||||
continue;
|
||||
}
|
||||
match arg.strip_prefix('-') {
|
||||
Some(possible_sigspec) if !possible_sigspec.is_empty() => {
|
||||
// Option position: interpret as a signal specification. The
|
||||
// sigspec may be a signal name (e.g. -TERM) or number (e.g. -9).
|
||||
if let Ok(parsed_trap_signal) = possible_sigspec.parse::<TrapSignal>() {
|
||||
trap_signal = parsed_trap_signal;
|
||||
options_done = true;
|
||||
} else {
|
||||
writeln!(context.stderr(), "{}: invalid signal name", context.command_name)?;
|
||||
return Ok(ExecutionExitCode::InvalidUsage.into());
|
||||
}
|
||||
},
|
||||
_ => {
|
||||
// First operand ends the option position.
|
||||
operands.push(arg);
|
||||
options_done = true;
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
if self.list_signals {
|
||||
return print_signals(&context, self.args.as_ref());
|
||||
}
|
||||
if !saw_pid_or_job_spec {
|
||||
if operands.is_empty() {
|
||||
writeln!(context.stderr(), "{}: invalid usage", context.command_name)?;
|
||||
return Ok(ExecutionExitCode::InvalidUsage.into());
|
||||
}
|
||||
|
||||
for pid_or_job_spec in self.args.iter().filter(|arg| !arg.starts_with('-')) {
|
||||
for pid_or_job_spec in operands {
|
||||
if pid_or_job_spec.starts_with('%') {
|
||||
// It's a job spec.
|
||||
if let Some(job) = context.shell.jobs_mut().resolve_job_spec(pid_or_job_spec) {
|
||||
|
||||
@@ -4,7 +4,7 @@
|
||||
|
||||
### Fixed
|
||||
|
||||
- Fixed the bash tool's `kill` builtin rejecting numeric signals and multiple process operands, and changed its default signal from `SIGKILL` to the standard `SIGTERM` ([#6779](https://github.com/can1357/oh-my-pi/issues/6779)).
|
||||
- Fixed the bash tool's `kill` builtin rejecting numeric signals and multiple process operands, and changed its default signal from `SIGKILL` to the standard `SIGTERM`. Negative PID operands (process groups per `kill(2)`) and the `--` end-of-options marker are now handled instead of being misparsed as signals ([#6779](https://github.com/can1357/oh-my-pi/issues/6779)).
|
||||
|
||||
## [17.1.5] - 2026-07-27
|
||||
|
||||
|
||||
Reference in New Issue
Block a user