bg/fg/wait/disown/function: check for negative PID argument

While at it, extract a function.

Seems to have regressed in 4.0.0 (fc47d9fa1d (Use strongly typed
`Pid` for job control, 2024-11-11)).

Fixes #11929
This commit is contained in:
Johannes Altmanninger
2025-10-11 08:26:42 +02:00
parent 8fe402e9f7
commit da411f6fa7
25 changed files with 182 additions and 181 deletions

View File

@@ -6,7 +6,11 @@ fish 4.1.3 (released ???)
This release fixes the following regressions identified in 4.1.0:
- Crash on invalid :doc:`function <cmds/function>` call (:issue:`11912`).
- Crash on invalid :doc:`function <cmds/function>` command (:issue:`11912`).
as well as the following regressions identified in 4.0.0:
- Crash when passing negative PIDs to :doc:`wait <cmds/wait>` (:issue:`11929`).
fish 4.1.2 (released October 7, 2025)
=====================================

View File

@@ -206,10 +206,6 @@ msgstr ""
msgid "%s: %s: invalid mode name. See `help identifiers`\n"
msgstr ""
#, c-format
msgid "%s: %s: invalid process ID"
msgstr ""
#, c-format
msgid "%s: %s: invalid scale\n"
msgstr ""
@@ -258,10 +254,6 @@ msgstr "%s: '%s' ist kein Job\n"
msgid "%s: '%s' is not a valid job ID\n"
msgstr ""
#, c-format
msgid "%s: '%s' is not a valid job specifier\n"
msgstr ""
#, c-format
msgid "%s: '%s' is not a valid process ID\n"
msgstr ""

View File

@@ -204,10 +204,6 @@ msgstr ""
msgid "%s: %s: invalid mode name. See `help identifiers`\n"
msgstr ""
#, c-format
msgid "%s: %s: invalid process ID"
msgstr ""
#, c-format
msgid "%s: %s: invalid scale\n"
msgstr ""
@@ -256,10 +252,6 @@ msgstr "%s: “%s” is not a job\n"
msgid "%s: '%s' is not a valid job ID\n"
msgstr ""
#, c-format
msgid "%s: '%s' is not a valid job specifier\n"
msgstr ""
#, c-format
msgid "%s: '%s' is not a valid process ID\n"
msgstr ""

View File

@@ -305,10 +305,6 @@ msgstr ""
msgid "%s: %s: invalid mode name. See `help identifiers`\n"
msgstr ""
#, c-format
msgid "%s: %s: invalid process ID"
msgstr ""
#, c-format
msgid "%s: %s: invalid scale\n"
msgstr ""
@@ -357,10 +353,6 @@ msgstr "%s : '%s' nest pas une tâche\n"
msgid "%s: '%s' is not a valid job ID\n"
msgstr ""
#, c-format
msgid "%s: '%s' is not a valid job specifier\n"
msgstr ""
#, c-format
msgid "%s: '%s' is not a valid process ID\n"
msgstr "%s : '%s' n'est pas un PID valide\n"

View File

@@ -200,10 +200,6 @@ msgstr ""
msgid "%s: %s: invalid mode name. See `help identifiers`\n"
msgstr ""
#, c-format
msgid "%s: %s: invalid process ID"
msgstr ""
#, c-format
msgid "%s: %s: invalid scale\n"
msgstr ""
@@ -252,10 +248,6 @@ msgstr "%s: '%s' nie jest zadaniem\n"
msgid "%s: '%s' is not a valid job ID\n"
msgstr ""
#, c-format
msgid "%s: '%s' is not a valid job specifier\n"
msgstr ""
#, c-format
msgid "%s: '%s' is not a valid process ID\n"
msgstr ""

View File

@@ -205,10 +205,6 @@ msgstr ""
msgid "%s: %s: invalid mode name. See `help identifiers`\n"
msgstr ""
#, c-format
msgid "%s: %s: invalid process ID"
msgstr ""
#, c-format
msgid "%s: %s: invalid scale\n"
msgstr ""
@@ -257,10 +253,6 @@ msgstr "%s: “%s” não é uma tarefa\n"
msgid "%s: '%s' is not a valid job ID\n"
msgstr ""
#, c-format
msgid "%s: '%s' is not a valid job specifier\n"
msgstr ""
#, c-format
msgid "%s: '%s' is not a valid process ID\n"
msgstr "%s: '%s' não é um identificador de tarefa válido\n"

View File

@@ -201,10 +201,6 @@ msgstr ""
msgid "%s: %s: invalid mode name. See `help identifiers`\n"
msgstr ""
#, c-format
msgid "%s: %s: invalid process ID"
msgstr ""
#, c-format
msgid "%s: %s: invalid scale\n"
msgstr ""
@@ -253,10 +249,6 @@ msgstr "%s: '%s' är inte ett jobb\n"
msgid "%s: '%s' is not a valid job ID\n"
msgstr ""
#, c-format
msgid "%s: '%s' is not a valid job specifier\n"
msgstr ""
#, c-format
msgid "%s: '%s' is not a valid process ID\n"
msgstr ""

View File

@@ -227,10 +227,6 @@ msgstr "%s: %s: 无效舍入模式\n"
msgid "%s: %s: invalid mode name. See `help identifiers`\n"
msgstr "%s: %s: 无效模式名。参见 `help identifiers`\n"
#, c-format
msgid "%s: %s: invalid process ID"
msgstr "%s: %s: 无效进程 ID"
#, c-format
msgid "%s: %s: invalid scale\n"
msgstr "%s: %s: 无效位数\n"
@@ -279,10 +275,6 @@ msgstr "%s: '%s' 不是一个作业\n"
msgid "%s: '%s' is not a valid job ID\n"
msgstr "%s: '%s' 不是一个有效的作业 ID\n"
#, c-format
msgid "%s: '%s' is not a valid job specifier\n"
msgstr "%s: '%s' 不是有效的作业说明符\n"
#, c-format
msgid "%s: '%s' is not a valid process ID\n"
msgstr "%s: '%s' 不是一个有效的进程 ID\n"

View File

@@ -637,10 +637,7 @@ fn throwing_main() -> i32 {
parser.get_last_status()
};
event::fire(
parser,
Event::process_exit(Pid::new(getpid()).unwrap(), exit_status),
);
event::fire(parser, Event::process_exit(Pid::new(getpid()), exit_status));
// Trigger any exit handlers.
event::fire_generic(

View File

@@ -81,14 +81,9 @@ pub fn bg(parser: &Parser, streams: &mut IoStreams, args: &mut [&wstr]) -> Built
let mut retval: BuiltinResult = Ok(SUCCESS);
let pids: Vec<Pid> = args[opts.optind..]
.iter()
.filter_map(|arg| match fish_wcstoi(arg).map(Pid::new) {
Ok(Some(pid)) => Some(pid),
.filter_map(|arg| match parse_pid(streams, cmd, arg) {
Ok(pid) => Some(pid),
_ => {
streams.err.append(wgettext_fmt!(
"%s: '%s' is not a valid job specifier\n",
cmd,
arg
));
retval = Err(STATUS_INVALID_ARGS);
None
}

View File

@@ -3,12 +3,8 @@
use super::prelude::*;
use crate::io::IoStreams;
use crate::parser::Parser;
use crate::proc::{add_disowned_job, Job, Pid};
use crate::{
builtins::shared::HelpOnlyCmdOpts,
wchar::wstr,
wutil::{fish_wcstoi, wgettext_fmt},
};
use crate::proc::{add_disowned_job, Job};
use crate::{builtins::shared::HelpOnlyCmdOpts, wchar::wstr, wutil::wgettext_fmt};
use libc::SIGCONT;
/// Helper for builtin_disown.
@@ -80,31 +76,23 @@ pub fn disown(parser: &Parser, streams: &mut IoStreams, args: &mut [&wstr]) -> B
// If one argument is not a valid pid (i.e. integer >= 0), fail without disowning anything,
// but still print errors for all of them.
// Non-existent jobs aren't an error, but information about them is useful.
let mut jobs: Vec<_> = args[1..]
let mut jobs: Vec<_> = args[opts.optind..]
.iter()
.filter_map(|arg| {
// Attempt to convert the argument to a PID.
match fish_wcstoi(arg).ok().and_then(Pid::new) {
None => {
// Invalid identifier
streams.err.append(wgettext_fmt!(
"%s: '%s' is not a valid job specifier\n",
cmd,
arg
));
retval = Err(STATUS_INVALID_ARGS);
None
let pid = match parse_pid(streams, cmd, arg) {
Ok(pid) => pid,
Err(code) => {
retval = Err(code);
return None;
}
Some(pid) => parser.job_get_from_pid(pid).or_else(|| {
};
parser.job_get_from_pid(pid).or_else(|| {
// Valid identifier but no such job
streams.err.append(wgettext_fmt!(
"%s: Could not find job '%d'\n",
cmd,
pid
));
streams
.err
.append(wgettext_fmt!("%s: Could not find job '%d'\n", cmd, pid));
None
}),
}
})
})
.collect();

View File

@@ -1,7 +1,6 @@
//! Implementation of the fg builtin.
use crate::fds::make_fd_blocking;
use crate::proc::Pid;
use crate::reader::{reader_save_screen_state, reader_write_title};
use crate::tokenizer::tok_command;
use crate::wutil::perror;
@@ -49,9 +48,9 @@ pub fn fg(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> Built
} else if optind + 1 < argv.len() {
// Specifying more than one job to put to the foreground is a syntax error, we still
// try to locate the job $argv[1], since we need to determine which error message to
// emit (ambiguous job specification vs malformed job id).
// emit (ambiguous job specification vs malformed job ID).
let mut found_job = false;
if let Ok(Some(pid)) = fish_wcstoi(argv[optind]).map(Pid::new) {
if let Ok(pid) = parse_pid(streams, cmd, argv[optind]) {
found_job = parser.job_get_from_pid(pid).is_some();
};
@@ -69,27 +68,15 @@ pub fn fg(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> Built
job_pos = None;
job = None;
} else {
match fish_wcstoi(argv[optind]) {
Ok(..0) | Err(_) => {
streams.err.append(wgettext_fmt!(
"%s: '%s' is not a valid process ID",
cmd,
argv[optind]
));
job_pos = None;
job = None;
builtin_print_error_trailer(parser, streams.err, cmd);
}
match parse_pid(streams, cmd, argv[optind]) {
Ok(pid) => {
let raw_pid = pid;
let pid = Pid::new(pid);
let j = pid.and_then(|pid| parser.job_get_with_index_from_pid(pid));
let j = parser.job_get_with_index_from_pid(pid);
if j.as_ref()
.map_or(true, |(_pos, j)| !j.is_constructed() || j.is_completed())
{
streams
.err
.append(wgettext_fmt!("%s: No suitable job: %d\n", cmd, raw_pid));
.append(wgettext_fmt!("%s: No suitable job: %d\n", cmd, pid));
job_pos = None;
job = None
} else {
@@ -99,7 +86,7 @@ pub fn fg(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> Built
streams.err.append(wgettext_fmt!(
"%s: Can't put job %d, '%s' to foreground because it is not under job control\n",
cmd,
raw_pid,
pid,
j.command()
));
None
@@ -108,8 +95,13 @@ pub fn fg(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> Built
};
}
}
Err(_err) => {
job_pos = None;
job = None;
builtin_print_error_trailer(parser, streams.err, cmd);
}
};
};
let Some(job) = job else {
return Err(STATUS_INVALID_ARGS);

View File

@@ -157,26 +157,18 @@ fn parse_cmd_opts(
e = EventDescription::CallerExit { caller_id };
} else if opt == 'p' && woptarg == "%self" {
let pid = Pid::new(getpid());
e = EventDescription::ProcessExit { pid: Some(pid) };
} else {
let pid = parse_pid_may_be_zero(streams, cmd, woptarg)?;
if opt == 'p' {
e = EventDescription::ProcessExit { pid };
} else {
let Ok(pid @ 0..) = fish_wcstoi(woptarg) else {
streams.err.append(wgettext_fmt!(
"%s: %s: invalid process ID",
cmd,
woptarg
));
return Err(STATUS_INVALID_ARGS);
};
if opt == 'p' {
e = EventDescription::ProcessExit { pid: Pid::new(pid) };
} else {
// TODO: rationalize why a default of 0 is sensible.
let internal_job_id = match Pid::new(pid) {
Some(pid) => job_id_for_pid(pid, parser).unwrap_or(0),
None => 0,
};
let internal_job_id = pid
.and_then(|pid| job_id_for_pid(pid, parser))
.unwrap_or_default();
e = EventDescription::JobExit {
pid: Pid::new(pid),
pid,
internal_job_id,
};
}

View File

@@ -5,7 +5,7 @@
use crate::io::IoStreams;
use crate::job_group::{JobId, MaybeJobId};
use crate::parser::Parser;
use crate::proc::{clock_ticks_to_seconds, have_proc_stat, proc_get_jiffies, Job, Pid};
use crate::proc::{clock_ticks_to_seconds, have_proc_stat, proc_get_jiffies, Job};
use crate::wchar_ext::WExt;
use crate::wgetopt::{wopt, ArgType, WGetopter, WOption};
use crate::wutil::wgettext;
@@ -214,19 +214,8 @@ pub fn jobs(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> Bui
}
}
} else {
match fish_wcstoi(arg).ok().and_then(Pid::new) {
None => {
streams.err.append(wgettext_fmt!(
"%s: '%s' is not a valid process ID\n",
cmd,
arg
));
return Err(STATUS_INVALID_ARGS);
}
Some(job_id) => {
j = parser.job_get_from_pid(job_id);
}
}
let pid = parse_pid(streams, cmd, arg)?;
j = parser.job_get_from_pid(pid)
}
if let Some(j) = j.filter(|j| !j.is_completed() && j.is_constructed()) {

View File

@@ -1,12 +1,12 @@
use super::prelude::*;
use crate::builtins::*;
use crate::common::{escape, get_by_sorted_name, str2wcstring, Named};
use crate::io::OutputStream;
use crate::parse_constants::UNKNOWN_BUILTIN_ERR_MSG;
use crate::parse_util::parse_util_argument_is_help;
use crate::parser::{BlockType, LoopStatus};
use crate::proc::{no_exec, ProcStatus};
use crate::proc::{no_exec, Pid, ProcStatus};
use crate::wchar::L;
use crate::{builtins::*, wutil};
use errno::errno;
use std::fs::File;
@@ -929,6 +929,41 @@ fn next(&mut self) -> Option<Self::Item> {
}
}
pub fn parse_pid(streams: &mut IoStreams, cmd: &wstr, arg: &wstr) -> Result<Pid, ErrorCode> {
parsed_pid(streams, cmd, arg, fish_wcstoi(arg))
}
pub fn parse_pid_may_be_zero(
streams: &mut IoStreams,
cmd: &wstr,
arg: &wstr,
) -> Result<Option<Pid>, ErrorCode> {
let parsed = fish_wcstoi(arg);
if parsed == Ok(0) {
return Ok(None);
}
parsed_pid(streams, cmd, arg, parsed).map(Some)
}
fn parsed_pid(
streams: &mut IoStreams,
cmd: &wstr,
arg: &wstr,
pid: Result<i32, wutil::Error>,
) -> Result<Pid, ErrorCode> {
match pid {
Ok(pid @ 1..) => Ok(Pid::new(pid)),
_ => {
streams.err.append(wgettext_fmt!(
"%s: '%s' is not a valid process ID\n",
cmd,
arg
));
Err(STATUS_INVALID_ARGS)
}
}
}
/// A generic builtin that only supports showing a help message. This is only a placeholder that
/// prints the help message. Useful for commands that live in the parser.
fn builtin_generic(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> BuiltinResult {

View File

@@ -195,21 +195,14 @@ pub fn wait(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> Bui
let optind = w.wopt_index;
for item in &argv[optind..argc] {
if iswnumeric(item) {
// argument is pid
let mpid: i32 = fish_wcstoi(item).unwrap_or(-1);
let Some(mpid) = Pid::new(mpid) else {
streams.err.append(wgettext_fmt!(
"%s: '%s' is not a valid process ID\n",
cmd,
item,
));
let Ok(pid) = parse_pid(streams, cmd, item) else {
continue;
};
if !find_wait_handles(WaitHandleQuery::Pid(mpid), parser, &mut wait_handles) {
if !find_wait_handles(WaitHandleQuery::Pid(pid), parser, &mut wait_handles) {
streams.err.append(wgettext_fmt!(
"%s: Could not find a job with process ID '%d'\n",
cmd,
mpid,
pid,
));
}
} else {

View File

@@ -751,9 +751,10 @@ fn fork_child_for_process(
}
// We are the parent. Record the pid and store the pgid for the job if it should lead the pgroup.
p.set_pid(Pid::new(pid).unwrap());
let pid = Pid::new(pid);
p.set_pid(pid);
if matches!(pgroup_policy, PgroupPolicy::Lead) {
job.group().set_pgid(Pid::new(pid).unwrap());
job.group().set_pgid(pid);
}
let count = FORK_COUNT.fetch_add(1, Ordering::Relaxed) + 1;
@@ -905,14 +906,15 @@ fn exec_external_command(
);
// these are all things do_fork() takes care of normally (for forked processes):
p.set_pid(Pid::new(pid).unwrap());
let pid = Pid::new(pid);
p.set_pid(pid);
if p.leads_pgrp {
j.group().set_pgid(Pid::new(pid).unwrap());
j.group().set_pgid(pid);
// posix_spawn should in principle set the pgid before returning.
// In glibc, posix_spawn uses fork() and the pgid group is set on the child side;
// therefore the parent may not have seen it be set yet.
// Ensure it gets set. See #4715, also https://github.com/Microsoft/WSL/issues/2997.
execute_setpgid(pid, pid, true /* is parent */);
execute_setpgid(pid.as_pid_t(), pid.as_pid_t(), true /* is parent */);
}
return Ok(());
}

View File

@@ -274,10 +274,13 @@ pub fn get_id(&self) -> u64 {
impl Pid {
#[inline(always)]
pub fn new(pid: i32) -> Option<Pid> {
// Construct a pid from an i32, which must be at least zero.
assert!(pid >= 0, "Pid must be at least zero");
NonZeroU32::new(pid as u32).map(Pid)
pub fn new(pid: i32) -> Self {
Self(
u32::try_from(pid)
.ok()
.and_then(NonZeroU32::new)
.expect("PID must be greater than zero"),
)
}
#[inline(always)]
pub fn get(&self) -> i32 {
@@ -1234,9 +1237,10 @@ fn process_mark_finished_children(parser: &Parser, block_ok: bool) {
WNOHANG | WUNTRACED | WCONTINUED,
)
};
let Some(pid) = Pid::new(pid) else {
if pid == 0 {
continue;
};
}
let pid = Pid::new(pid);
assert!(pid == proc.pid().unwrap(), "Unexpected waitpid() return");
// The process has stopped or exited! Update its status.

View File

@@ -126,7 +126,7 @@ fn test_wait_handles() {
assert_eq!(whs.size(), 0);
fn p(pid: i32) -> Pid {
Pid::new(pid).unwrap()
Pid::new(pid)
}
assert!(whs.get_by_pid(p(5)).is_none());

8
tests/checks/bg.fish Normal file
View File

@@ -0,0 +1,8 @@
# RUN: %fish %s
bg 0
# CHECKERR: bg: '0' is not a valid process ID
bg -- -1
# CHECKERR: bg: '-1' is not a valid process ID
bg -- -(math 2 ^ 31)
# CHECKERR: bg: '-2147483648' is not a valid process ID

8
tests/checks/disown.fish Normal file
View File

@@ -0,0 +1,8 @@
# RUN: %fish %s
disown 0
# CHECKERR: disown: '0' is not a valid process ID
disown -- -1
# CHECKERR: disown: '-1' is not a valid process ID
disown -- -(math 2 ^ 31)
# CHECKERR: disown: '-2147483648' is not a valid process ID

View File

@@ -1,9 +1,25 @@
# RUN: %fish %s
builtin fg -- -1
# CHECKERR: fg: '-1' is not a valid process ID
# CHECKERR: {{.*}}/fg.fish (line {{\d+}}):
# CHECKERR: builtin fg -- -1
fg (math 2 ^ 31 - 1)
# CHECKERR: fg: No suitable job: 2147483647
fg (math 2 ^ 31)
# CHECKERR: fg: '2147483648' is not a valid process ID
# CHECKERR: {{.*}}config.fish (line {{\d+}}):
# CHECKERR: and builtin fg $args[-1]
# CHECKERR: ^
# CHECKERR:
# CHECKERR: in function 'fg' with arguments '2147483648'
# CHECKERR: {{\t}}called on line {{\d+}} of file {{.*}}/fg.fish
# CHECKERR: (Type 'help fg' for related documentation)
fg 0 2>| string match --max-matches=1 '*' >&2
# CHECKERR: fg: '0' is not a valid process ID
builtin fg -- -1 2>| string match --max-matches=1 '*' >&2
# CHECKERR: fg: '-1' is not a valid process ID
builtin fg -- -1 2>| string match --max-matches=1 '*' >&2
# CHECKERR: fg: '-1' is not a valid process ID
builtin fg -- -(math 2 ^ 31) 2>| string match --max-matches=1 '*' >&2
# CHECKERR: fg: '-2147483648' is not a valid process ID

View File

@@ -147,7 +147,7 @@ rm -r $tmpdir
functions -e foo
function foo -p bar; end
# CHECKERR: {{.*}}function.fish (line {{\d+}}): function: bar: invalid process ID
# CHECKERR: {{.*}}function.fish (line {{\d+}}): function: 'bar' is not a valid process ID
# CHECKERR: function foo -p bar; end
# CHECKERR: ^~~~~~~~~~~~~~~~~~~^
@@ -214,4 +214,29 @@ end
outer 1 2 3
#CHECK: 1 2 3
for flag in --on-process-exit --on-job-exit
for invalid_pid in (math 2 ^ 31) -1 -(math 2 ^ 31)
function invalid $flag=$invalid_pid
end
end
# CHECKERR: {{.*}}/function.fish (line {{\d+}}): function: '2147483648' is not a valid process ID
# CHECKERR: function invalid $flag=$invalid_pid
# CHECKERR: ^
# CHECKERR: {{.*}}/function.fish (line {{\d+}}): function: '-1' is not a valid process ID
# CHECKERR: function invalid $flag=$invalid_pid
# CHECKERR: ^
# CHECKERR: {{.*}}/function.fish (line {{\d+}}): function: '-2147483648' is not a valid process ID
# CHECKERR: function invalid $flag=$invalid_pid
# CHECKERR: ^
# CHECKERR: {{.*}}/function.fish (line {{\d+}}): function: '2147483648' is not a valid process ID
# CHECKERR: function invalid $flag=$invalid_pid
# CHECKERR: ^
# CHECKERR: {{.*}}/function.fish (line {{\d+}}): function: '-1' is not a valid process ID
# CHECKERR: function invalid $flag=$invalid_pid
# CHECKERR: ^
# CHECKERR: {{.*}}/function.fish (line {{\d+}}): function: '-2147483648' is not a valid process ID
# CHECKERR: function invalid $flag=$invalid_pid
# CHECKERR: ^
end
exit 0

View File

@@ -67,7 +67,7 @@ echo $status
#CHECK: 2
#CHECKERR: jobs: 'foo' is not a valid process ID
disown foo
#CHECKERR: disown: 'foo' is not a valid job specifier
#CHECKERR: disown: 'foo' is not a valid process ID
disown (jobs -p)
or exit 0

View File

@@ -67,3 +67,12 @@ end
set trigger_var 123
sleep .5
# CHECK: Callback called
wait (math 2 ^ 32)
# CHECKERR: wait: '4294967296' is not a valid process ID
wait 0
# CHECKERR: wait: '0' is not a valid process ID
wait -- -1
# CHECKERR: wait: Could not find child processes with the name '-1'
wait -- -(math 2 ^ 31)
# CHECKERR: wait: Could not find child processes with the name '-2147483648'