From f52708a20f980c5351d363caf75b6d293c6cf597 Mon Sep 17 00:00:00 2001 From: David Adam Date: Sun, 23 Apr 2017 22:56:33 +0800 Subject: [PATCH] job_t: use the sentinel value of -2 for new job process group IDs 0 is not a good default PGID, because it's possible for a kernel process to have the PGID of 0 under Linux. This meant that job_get_from_pid could return incorrect jobs, as the PGID for internal, non-forked jobs was the same as kernel processes. Avoid this by using an invalid PGID as the initial PGID. --- src/postfork.cpp | 11 +++++++---- src/proc.cpp | 7 ++++++- src/proc.h | 1 + 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/postfork.cpp b/src/postfork.cpp index 01ed1f7c6..da9d2f26a 100644 --- a/src/postfork.cpp +++ b/src/postfork.cpp @@ -70,7 +70,8 @@ bool set_child_group(job_t *j, process_t *p, int print_errors) { bool retval = true; if (j->get_flag(JOB_CONTROL)) { - if (!j->pgid) { + // New jobs have the pgid set to -2 + if (j->pgid == -2) { j->pgid = p->pid; } @@ -322,10 +323,12 @@ bool fork_actions_make_spawn_properties(posix_spawnattr_t *attr, if (j->get_flag(JOB_CONTROL)) { should_set_process_group_id = true; - // PCA: I'm quite fuzzy on process groups, but I believe that the default value of 0 means - // that the process becomes its own group leader, which is what set_child_group did in this - // case. So we want this to be 0 if j->pgid is 0. + // set_child_group puts each job into its own process group + // do the same here if there is no PGID yet (i.e. PGID == -2) desired_process_group_id = j->pgid; + if (desired_process_group_id == -2 ) { + desired_process_group_id = 0; + } } // Set the handling for job control signals back to the default. diff --git a/src/proc.cpp b/src/proc.cpp index 3c1327329..25c37e9ad 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -362,8 +362,13 @@ process_t::process_t() { } +/// The constructor sets the pgid to -2 as a sentinel value +/// 0 should not be used; although it is not a valid PGID in userspace, +/// the Linux kernel will use it for kernel processes. +/// -1 should not be used; it is a possible return value of the getpgid() +/// function job_t::job_t(job_id_t jobid, const io_chain_t &bio) - : block_io(bio), pgid(0), tmodes(), job_id(jobid), flags(0) {} + : block_io(bio), pgid(-2), tmodes(), job_id(jobid), flags(0) {} job_t::~job_t() { release_job_id(job_id); } diff --git a/src/proc.h b/src/proc.h index 928e6a8c2..b32bb887c 100644 --- a/src/proc.h +++ b/src/proc.h @@ -217,6 +217,7 @@ class job_t { process_list_t processes; /// Process group ID for the process group that this job is running in. + /// Set to a nonexistent, non-return-value of getpgid() integer by the constructor pid_t pgid; /// The saved terminal modes of this job. This needs to be saved so that we can restore the /// terminal to the same state after temporarily taking control over the terminal when a job