diff --git a/backport-0005-library-address-a-potential-race-resulting-in-a-fault.patch b/backport-0005-library-address-a-potential-race-resulting-in-a-fault.patch new file mode 100644 index 0000000000000000000000000000000000000000..81e1c1fd2c660ead17587c14428d4dfca7f0c998 --- /dev/null +++ b/backport-0005-library-address-a-potential-race-resulting-in-a-fault.patch @@ -0,0 +1,246 @@ +From 5b95bf6f9d830d49d33d39286f24a8264bca32b8 Mon Sep 17 00:00:00 2001 +From: Jim Warner +Date: Tue, 12 Aug 2025 00:00:00 -0500 +Subject: [PATCH 1/2] library: address a potential race resulting in a fault + +The library position was to never subject callers to a +NULL string pointer. Rather, a literal "-" was usually +substituted to mean such information is not available. + +But, as revealed in the issue cited below, there was a +huge unknown gap in our goal. Under the original code, +if the 'status' file was not available in /proc/, +fields like supgrp, ruser, rgroup, etc. would be NULL. + +The same was true with lxc and docker container fields +which depend on the 'cgroup' file. Should that file be +missing it means more unexpected NULL string pointers. + +Under what conditions might such files be unavailable? +If a process exited but has not yet been fully reaped, +it could be encountered by a repeatedly run ps or by a +continuously running program like top. In such a case, +there's a possibility some files under /proc/ are +missing. A NULL string address could then be a result. + +[ as workload increases on a machine with many cores ] +[ that risk of a missing file rises. the environment ] +[ prompting this issue involved a large Epyc machine ] +[ running 72 concurrent testing processes & invoking ] +[ the ps program on average five times every second. ] + +So, this commit will initialize the 'status' dependent +string results with a pointer to a "-" string variable +whose address can be known. This will save us the cost +of strdup() calls. Then, that pids module can test for +it to avoid any free() attempt when assigning results. + +[ for lxc & docker containers, that require 'cgroup' ] +[ to be present, the solution to some potential NULL ] +[ pointer is addressed in a little different manner. ] + +Lastly, the 'stat' file is now read unconditionally to +prevent a NULL 'cmdline' pointer for kernel threads if +the 'status' file is missing or wasn't otherwise read. + +Many thanks to Mike Gulick for raising this issue plus +Chao Liu for his contributions. Also, a special thanks +to Tsukasa OI for an analysis leading to the solution. + +Reference(s): +https://gitlab.com/procps-ng/procps/-/issues/380 + +Reference:https://gitlab.com/procps-ng/procps/-/commit/a5708118a4f3184cb5a3e2faa219807e73f007d3 +Conflict: adapt context, regarding the proc path variable + +Signed-off-by: Jim Warner +--- + NEWS | 1 + + library/pids.c | 14 ++++++++----- + library/readproc.c | 51 +++++++++++++++++++++++++++++----------------- + 3 files changed, 42 insertions(+), 24 deletions(-) + +diff --git a/NEWS b/NEWS +index 4acc302..dac423d 100644 +--- a/NEWS ++++ b/NEWS +@@ -8,6 +8,7 @@ procps-ng-4.0.5 + internal: only count user sessions + internal: Recover from meminfo seek using LXC Debian #1072831 + internal: stat api no longer counts guest tics twice issue #339 ++ internal: Avoid potential caller segfault due to race issue #380 + external: zswap & zswapped added to meminfo api + external: schedule class added to pids api + external: disk sleep added to pids api, sleep revised issue #265 +diff --git a/library/pids.c b/library/pids.c +index ed06930..f079ca3 100644 +--- a/library/pids.c ++++ b/library/pids.c +@@ -105,10 +105,12 @@ struct pids_info { + + // ___ Free Storage Support ||||||||||||||||||||||||||||||||||||||||||||||||||| + ++extern char *str_none; ++ + #define freNAME(t) free_pids_ ## t + + static void freNAME(str) (struct pids_result *R) { +- if (R->result.str) free(R->result.str); ++ if (R->result.str && R->result.str != str_none) free(R->result.str); + } + + static void freNAME(strv) (struct pids_result *R) { +@@ -1067,10 +1069,12 @@ static inline void pids_libflags_set ( + info->oldflags |= Item_table[e].oldflags; + info->history_yes |= Item_table[e].needhist; + } +- if (info->oldflags & f_either) { +- if (!(info->oldflags & (f_stat | f_status))) +- info->oldflags |= f_stat; +- } ++// note: the read of f_stat has been made unconditional in readproc.c ++// so this logic is no longer useful ... ++// if (info->oldflags & f_either) { ++// if (!(info->oldflags & (f_stat | f_status))) ++// info->oldflags |= f_stat; ++// } + info->containers_yes = info->oldflags & (f_lxc | z_docker); + return; + } // end: pids_libflags_set +diff --git a/library/readproc.c b/library/readproc.c +index b4fdd63..c8b0f83 100644 +--- a/library/readproc.c ++++ b/library/readproc.c +@@ -78,6 +78,7 @@ struct utlbuf_s { + + static int task_dir_missing; + ++char *str_none = "-"; + + // free any additional dynamically acquired storage associated with a proc_t + static inline void free_acquired (proc_t *p) { +@@ -95,9 +96,15 @@ static inline void free_acquired (proc_t *p) { + if (p->sd_slice) free(p->sd_slice); + if (p->sd_unit) free(p->sd_unit); + if (p->sd_uunit) free(p->sd_uunit); +- if (p->supgid) free(p->supgid); ++ if (p->supgid && p->supgid != str_none) free(p->supgid); + + memset(p, '\0', sizeof(proc_t)); ++ // if /proc//status wasn't available we won't be calling the pwcache guys, ++ // so we'll follow the convention used elsewhere in this module ... ++ p->ruser = p->suser = p->fuser ++ = p->rgroup = p->sgroup = p->fgroup ++ = p->supgid = p->supgrp ++ = str_none; + } + + +@@ -434,11 +441,8 @@ ENTER(0x220); + #ifdef FALSE_THREADS + if (!IS_THREAD(P)) { + #endif +- if (!P->supgid) { +- P->supgid = strdup("-"); +- if (!P->supgid) +- return 1; +- } ++ if (!P->supgid) ++ P->supgid = str_none; + #ifdef FALSE_THREADS + } + #endif +@@ -455,8 +459,10 @@ static int supgrps_from_supgids (proc_t *p) { + #ifdef FALSE_THREADS + if (IS_THREAD(p)) return 0; + #endif +- if (!p->supgid || '-' == *p->supgid) ++ if (!p->supgid || p->supgid == str_none) + goto wrap_up; ++ // ensure we're not pointing to str_none ... ++ p->supgrp = NULL; + + s = p->supgid; + t = 0; +@@ -483,9 +489,8 @@ static int supgrps_from_supgids (proc_t *p) { + } while (*s); + + wrap_up: +- if (!p->supgrp +- && !(p->supgrp = strdup("-"))) +- return 1; ++ if (!p->supgrp) ++ p->supgrp = str_none; + return 0; + } + +@@ -952,7 +957,7 @@ static int fill_cmdline_cvt (const char *directory, proc_t *restrict p) { + escape_str(dst_buffer, src_buffer, MAX_BUFSZ); + else + escape_command(dst_buffer, p, MAX_BUFSZ, uFLG); +- p->cmdline = strdup(dst_buffer[0] ? dst_buffer : "?"); ++ p->cmdline = strdup(dst_buffer[0] ? dst_buffer : str_none); + if (!p->cmdline) + return 1; + return 0; +@@ -1231,11 +1236,14 @@ static proc_t *simple_readproc(PROCTAB *restrict const PT, proc_t *restrict cons + p->euid = sb.st_uid; /* need a way to get real uid */ + p->egid = sb.st_gid; /* need a way to get real gid */ + +- if (flags & PROC_FILLSTAT) { // read /proc/#/stat ++ /* this attempted read of 'stat' is now unconditional to ensure a 'cmd' name ++ as a minimum. this prevents a NULL 'cmdline' pointer for kernel threads ++ in case the 'status' file is missing or not otherwise read ... */ ++// if (flags & PROC_FILLSTAT) { // read /proc/#/stat + if (file2str(path, "stat", &ub) == -1) + goto next_proc; + rc += stat2proc(ub.buf, p); +- } ++// } + + if (flags & PROC_FILLIO) { // read /proc/#/io + if (file2str(path, "io", &ub) != -1) +@@ -1313,8 +1321,9 @@ static proc_t *simple_readproc(PROCTAB *restrict const PT, proc_t *restrict cons + if (flags & PROC_FILLSYSTEMD) // get sd-login.h stuff + rc += sd2proc(p); + +- if (flags & (PROC_FILL_LXC | PROC_FILL_DOCKER) +- && (file2str(path, "cgroup", &ub) > 0)) { ++ if (flags & (PROC_FILL_LXC | PROC_FILL_DOCKER)) { ++ // ok if nothing is read, an empty buffer will do just fine ... ++ file2str(path, "cgroup", &ub); + if (flags & PROC_FILL_LXC) // value the lxc name + p->lxcname = lxc_containers(path, &ub); + if (flags & PROC_FILL_DOCKER) { // value the dockerids +@@ -1371,11 +1380,14 @@ static proc_t *simple_readtask(PROCTAB *restrict const PT, proc_t *restrict cons + t->euid = sb.st_uid; /* need a way to get real uid */ + t->egid = sb.st_gid; /* need a way to get real gid */ + +- if (flags & PROC_FILLSTAT) { // read /proc/#/task/#/stat ++ /* this attempted read of 'stat' is now unconditional to ensure a 'cmd' name ++ as a minimum. this prevents a NULL 'cmdline' pointer for kernel threads ++ in case the 'status' file is missing or not otherwise read ... */ ++// if (flags & PROC_FILLSTAT) { // read /proc/#/task/#/stat + if (file2str(path, "stat", &ub) == -1) + goto next_task; + rc += stat2proc(ub.buf, t); +- } ++// } + + if (flags & PROC_FILLIO) { // read /proc/#/task/#/io + if (file2str(path, "io", &ub) != -1) +@@ -1458,8 +1470,9 @@ static proc_t *simple_readtask(PROCTAB *restrict const PT, proc_t *restrict cons + if (flags & PROC_FILLNS) // read /proc/#/task/#/ns/* + procps_ns_read_pid(t->tid, &(t->ns)); + +- if (flags & (PROC_FILL_LXC | PROC_FILL_DOCKER) +- && (file2str(path, "cgroup", &ub) > 0)) { ++ if (flags & (PROC_FILL_LXC | PROC_FILL_DOCKER)) { ++ // ok if nothing is read, an empty buffer will do just fine ... ++ file2str(path, "cgroup", &ub); + if (flags & PROC_FILL_LXC) // value the lxc name + t->lxcname = lxc_containers(path, &ub); + if (flags & PROC_FILL_DOCKER) { // value the dockerids +-- +2.43.0 + diff --git a/backport-0006-library-avoid-strdup-calls-in-sd2proc-if-possible.patch b/backport-0006-library-avoid-strdup-calls-in-sd2proc-if-possible.patch new file mode 100644 index 0000000000000000000000000000000000000000..99e2910ffc99b092c32ef805c3a989819d0b467a --- /dev/null +++ b/backport-0006-library-avoid-strdup-calls-in-sd2proc-if-possible.patch @@ -0,0 +1,106 @@ +From be4b9ccbb4979d59afa13b2de23eb38744d28731 Mon Sep 17 00:00:00 2001 +From: Jim Warner +Date: Wed, 13 Aug 2025 00:00:00 -0500 +Subject: [PATCH 2/2] library: avoid strdup() calls in sd2proc() if possible + +After expanding references to that 'str_none' variable +as opposed to a literal dash, mostly for documentation +purposes, it became possible to reduce strdup() calls. + +So, this patch will trade a simple assignment for many +strdup() calls while eliminating any ENOMEM potential. + +Reference:https://gitlab.com/procps-ng/procps/-/commit/148008629a291c682bc3e7fe89d471189e69b19c +Conflict: adapt context, regarding the use of 'no value' conventions + +Signed-off-by: Jim Warner +--- + library/readproc.c | 52 ++++++++++++++++++++-------------------------- + 1 file changed, 23 insertions(+), 29 deletions(-) + +diff --git a/library/readproc.c b/library/readproc.c +index c8b0f83..708011d 100644 +--- a/library/readproc.c ++++ b/library/readproc.c +@@ -86,17 +86,17 @@ static inline void free_acquired (proc_t *p) { + * here we free those items that might exist even when not explicitly | + * requested by our caller. it is expected that pid.c will then free | + * any remaining dynamic memory which might be dangling off a proc_t. | */ +- if (p->cgname) free(p->cgname); +- if (p->cgroup) free(p->cgroup); +- if (p->cmd) free(p->cmd); +- if (p->sd_mach) free(p->sd_mach); +- if (p->sd_ouid) free(p->sd_ouid); +- if (p->sd_seat) free(p->sd_seat); +- if (p->sd_sess) free(p->sd_sess); +- if (p->sd_slice) free(p->sd_slice); +- if (p->sd_unit) free(p->sd_unit); +- if (p->sd_uunit) free(p->sd_uunit); +- if (p->supgid && p->supgid != str_none) free(p->supgid); ++ if (p->cgname) free(p->cgname); ++ if (p->cgroup) free(p->cgroup); ++ if (p->cmd) free(p->cmd); ++ if (p->sd_mach && p->sd_mach != str_none) free(p->sd_mach); ++ if (p->sd_ouid && p->sd_ouid != str_none) free(p->sd_ouid); ++ if (p->sd_seat && p->sd_seat != str_none) free(p->sd_seat); ++ if (p->sd_sess && p->sd_sess != str_none) free(p->sd_sess); ++ if (p->sd_slice && p->sd_slice != str_none) free(p->sd_slice); ++ if (p->sd_unit && p->sd_unit != str_none) free(p->sd_unit); ++ if (p->sd_uunit && p->sd_uunit != str_none) free(p->sd_uunit); ++ if (p->supgid && p->supgid != str_none) free(p->supgid); + + memset(p, '\0', sizeof(proc_t)); + // if /proc//status wasn't available we won't be calling the pwcache guys, +@@ -515,37 +515,31 @@ static int sd2proc (proc_t *restrict p) { + char buf[64]; + uid_t uid; + +- if (0 > sd_pid_get_machine_name(p->tid, &p->sd_mach)) { +- if (!(p->sd_mach = strdup("-"))) +- return 1; +- } ++ if (0 > sd_pid_get_machine_name(p->tid, &p->sd_mach)) ++ p->sd_mach = str_none; ++ + if (0 > sd_pid_get_owner_uid(p->tid, &uid)) { +- if (!(p->sd_ouid = strdup("-"))) +- return 1; ++ p->sd_ouid = str_none; + } else { + snprintf(buf, sizeof(buf), "%d", (int)uid); + if (!(p->sd_ouid = strdup(buf))) + return 1; + } + if (0 > sd_pid_get_session(p->tid, &p->sd_sess)) { +- if (!(p->sd_sess = strdup("-"))) +- return 1; +- if (!(p->sd_seat = strdup("-"))) +- return 1; ++ p->sd_sess = str_none; ++ p->sd_seat = str_none; + } else { + if (0 > sd_session_get_seat(p->sd_sess, &p->sd_seat)) +- if (!(p->sd_seat = strdup("-"))) +- return 1; ++ p->sd_seat = str_none; + } + if (0 > sd_pid_get_slice(p->tid, &p->sd_slice)) +- if (!(p->sd_slice = strdup("-"))) +- return 1; ++ p->sd_slice = str_none; ++ + if (0 > sd_pid_get_unit(p->tid, &p->sd_unit)) +- if (!(p->sd_unit = strdup("-"))) +- return 1; ++ p->sd_unit = str_none; ++ + if (0 > sd_pid_get_user_unit(p->tid, &p->sd_uunit)) +- if (!(p->sd_uunit = strdup("-"))) +- return 1; ++ p->sd_uunit = str_none; + #else + if (!(p->sd_mach = strdup("?"))) + return 1; +-- +2.43.0 + diff --git a/procps-ng.spec b/procps-ng.spec index 0dce5188cbb2ab22d8713447c7a7fb2cb1a7a3ba..88913cfed3b5ca94d6ff275861588dfe65d561e2 100644 --- a/procps-ng.spec +++ b/procps-ng.spec @@ -1,6 +1,6 @@ Name: procps-ng Version: 4.0.5 -Release: 3 +Release: 4 Summary: Utilities that provide system information. License: GPL-2.0-or-later AND LGPL-2.0-or-later AND LGPL-2.1-or-later URL: https://sourceforge.net/projects/procps-ng/ @@ -16,6 +16,8 @@ Patch4: backport-0001-library-Use-u-gid-if-pwcache-returns-empty-user-grou.p Patch5: backport-0002-ps-check-for-null-on-escape-source.patch Patch6: backport-0003-ps-mv-check-for-null-from-escape_str_utf8-to-escape_.patch Patch7: backport-0004-library-internal-expand-buffer-for-stat-fd.patch +Patch8: backport-0005-library-address-a-potential-race-resulting-in-a-fault.patch +Patch9: backport-0006-library-avoid-strdup-calls-in-sd2proc-if-possible.patch BuildRequires: ncurses-devel libtool autoconf automake gcc gettext-devel systemd-devel @@ -93,6 +95,9 @@ ln -s %{_bindir}/pidof %{buildroot}%{_sbindir}/pidof %{_mandir}/man?/* %changelog +* Thu Aug 28 2025 Zhu Jin - 4.0.5-4 +- library: address a potential race resulting in a fault and avoid strdup() calls in sd2proc() if possible + * Mon Aug 25 2025 yinyongkang - 4.0.5-3 - library: internal expand buffer for stat_fd