diff --git a/backport-0001-library-address-a-potential-race-resulting-in-a-fault.patch b/backport-0001-library-address-a-potential-race-resulting-in-a-fault.patch new file mode 100644 index 0000000000000000000000000000000000000000..4293b056338e6a6015ca2a0f2f832fb8790b269b --- /dev/null +++ b/backport-0001-library-address-a-potential-race-resulting-in-a-fault.patch @@ -0,0 +1,227 @@ +From b7d95628c14606c5d0e752c300f5dedc2c9119d4 Mon Sep 17 00:00:00 2001 +From: Jim Warner +Date: Tue, 12 Aug 2025 00:00:00 -0500 +Subject: [PATCH] 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, removed the adaptation code for docker containers + +Signed-off-by: Jim Warner +--- + NEWS | 1 + + library/pids.c | 14 +++++++++----- + library/readproc.c | 42 +++++++++++++++++++++++++++--------------- + 3 files changed, 37 insertions(+), 20 deletions(-) + +diff --git a/NEWS b/NEWS +index 3f2158d..cd1dfe6 100644 +--- a/NEWS ++++ b/NEWS +@@ -3,6 +3,7 @@ procps-ng-4.0.4 + * library (API & ABI unchanged) + increment revision: 0:2:0 + tolerates all potential 'cpuinfo' formats issue #272 ++ internal: Avoid potential caller segfault due to race issue #380 + restore the proper main thread tics valuations issue #280 + Remove myself from proc count merge #193 + Refactor the escape code Debian #1035649 +diff --git a/library/pids.c b/library/pids.c +index 6ae94ad..87feead 100644 +--- a/library/pids.c ++++ b/library/pids.c +@@ -104,10 +104,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) { +@@ -1001,10 +1003,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; ++// } + return; + } // end: pids_libflags_set + +diff --git a/library/readproc.c b/library/readproc.c +index 2dfe4c9..9a3ddae 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,16 @@ 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; + } + + +@@ -431,11 +439,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 +@@ -452,8 +457,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; +@@ -480,9 +487,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; + } + +@@ -949,7 +955,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; +@@ -1136,11 +1142,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) +@@ -1286,7 +1295,10 @@ static proc_t *simple_readtask(PROCTAB *restrict const PT, proc_t *restrict cons + statm2proc(ub.buf, t); + } + +- if (flags & PROC_FILLSTATUS) { // read /proc/#/task/#/status ++ /* 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, "status", &ub) != -1) { + rc += status2proc(ub.buf, t, 0); + if (flags & (PROC_FILL_SUPGRP & ~PROC_FILLSTATUS)) +@@ -1302,7 +1314,7 @@ static proc_t *simple_readtask(PROCTAB *restrict const PT, proc_t *restrict cons + t->fgroup = pwcache_get_group(t->fgid); + } + } +- } ++// } + + /* some number->text resolving which is time consuming */ + /* ( names are cached, so memcpy to arrays was silly ) */ +-- +2.43.0 + diff --git a/backport-0002-library-avoid-strdup-calls-in-sd2proc-if-possible.patch b/backport-0002-library-avoid-strdup-calls-in-sd2proc-if-possible.patch new file mode 100644 index 0000000000000000000000000000000000000000..aedb9bf9d53e2083147efc4a7939213ebe29d4b4 --- /dev/null +++ b/backport-0002-library-avoid-strdup-calls-in-sd2proc-if-possible.patch @@ -0,0 +1,106 @@ +From 3db558f3a8fdb2f0b84df95916a4767f50ce89a4 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 5cf1be2..816e648 100644 +--- a/library/readproc.c ++++ b/library/readproc.c +@@ -82,17 +82,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)); + +@@ -509,37 +509,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 16d9306ff688536d51c48e8870c37489a4500f13..ca141ee7bb64fd4944ad9bc8d8301e0d8be0584a 100644 --- a/procps-ng.spec +++ b/procps-ng.spec @@ -1,6 +1,6 @@ Name: procps-ng Version: 4.0.4 -Release: 7 +Release: 8 Summary: Utilities that provide system information. License: GPL+ and GPLv2 and GPLv2+ and GPLv3+ and LGPLv2+ URL: https://sourceforge.net/projects/procps-ng/ @@ -22,6 +22,8 @@ Patch10: backport-top-adapt-to-that-guest-total-tics-change-stat-api.patch Patch11: backport-0001-library-Use-u-gid-if-pwcache-returns-empty-user-grou.patch Patch12: backport-0002-ps-check-for-null-on-escape-source.patch Patch13: backport-0003-ps-mv-check-for-null-from-escape_str_utf8-to-escape_.patch +Patch14: backport-0001-library-address-a-potential-race-resulting-in-a-fault.patch +Patch15: backport-0002-library-avoid-strdup-calls-in-sd2proc-if-possible.patch BuildRequires: ncurses-devel libtool autoconf automake gcc gettext-devel systemd-devel @@ -104,6 +106,9 @@ ln -s %{_bindir}/pidof %{buildroot}%{_sbindir}/pidof %{_mandir}/man* %changelog +* Thu Aug 28 2025 Zhu Jin - 4.0.4-8 +- library: address a potential race resulting in a fault and avoid strdup() calls in sd2proc() if possible + * Fri Jun 6 2025 Zhu Jin - 4.0.4-7 - top: Synchronize community bugfix patches