From c78c077a507100201a067b1ed49588df4d06f1f7 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Mon, 2 Nov 2026 00:27:33 +0100 Subject: [PATCH] fix(subprocess): fail the tree probe on a live member it cannot measure cbm_subprocess_tree_rss_bytes skipped any non-root process it could not measure and still returned OK with a smaller total, so a descendant the platform refused to measure silently dropped out of the RSS watchdog. Now a member is skipped only when the platform says it no longer exists: ESRCH from proc_pid_rusage on macOS, ENOENT or ESRCH for /proc//stat on Linux, ERROR_INVALID_PARAMETER from OpenProcess (or an exit code from a handle whose memory query failed) on Windows. Any other failure fails the probe, which the supervisor already turns into resource_probe_failed after three consecutive errors. The root stays strict as before. On macOS the members now come from proc_listpids(PROC_PGRP_ONLY): the old all-process scan read every process with proc_pidinfo, which answers EPERM for every process of another user (178 of 6438 on this machine), so it could not tell a foreign process from an unreadable member and fail-closed semantics would have failed every probe there. Linux keeps its /proc scan; stat files are world-readable on a default mount, and a hidepid=1 mount now fails closed, which the doc states. A test seam makes every non-root measurement fail with one platform error. RED (old skip-any-failure decision): the live-member test returns OK where ERROR is required. GREEN with the fix; the exited-member test passes in both, so verified exit races are still ignored. Signed-off-by: Martin Vogel --- docs/INDEX_RESOURCE_LIMITS.md | 6 +- src/foundation/subprocess.c | 208 +++++++++++++++++++++++++--------- src/foundation/subprocess.h | 9 +- tests/test_subprocess.c | 47 +++++++- 4 files changed, 208 insertions(+), 62 deletions(-) diff --git a/docs/INDEX_RESOURCE_LIMITS.md b/docs/INDEX_RESOURCE_LIMITS.md index 152599360..11fcf4a3d 100644 --- a/docs/INDEX_RESOURCE_LIMITS.md +++ b/docs/INDEX_RESOURCE_LIMITS.md @@ -60,7 +60,11 @@ quarantine a source file. If RSS is enabled and three consecutive probes cannot obtain any trustworthy tree measurement while the root worker is still running, CBM fails closed with -`code=resource_probe_failed`. +`code=resource_probe_failed`. A probe is untrustworthy as soon as one member of +the tree cannot be measured: only a process that exits between the listing and +its measurement is skipped, because a total without a live member would +under-report. On Linux the probe reads every `/proc//stat` to find the +tree's members, so a `/proc` mounted with `hidepid=1` makes it fail closed. ## Counting and failure semantics diff --git a/src/foundation/subprocess.c b/src/foundation/subprocess.c index 519a554e2..bce08c6bf 100644 --- a/src/foundation/subprocess.c +++ b/src/foundation/subprocess.c @@ -476,9 +476,53 @@ uint64_t cbm_subprocess_rss_sum_for_testing(const uint64_t *values, size_t count } return total; } + +static atomic_int g_tree_rss_member_fault; + +void cbm_subprocess_tree_rss_fault_members_for_testing(int platform_error) { + atomic_store_explicit(&g_tree_rss_member_fault, platform_error, memory_order_release); +} + +/* Test builds can make every NON-root member's measurement fail with one + * platform error (errno on POSIX, GetLastError() on Windows); 0 = real call. + * Production builds compile the check out entirely. */ +static int cbm_tree_rss_member_fault(bool root) { + return root ? 0 : atomic_load_explicit(&g_tree_rss_member_fault, memory_order_acquire); +} #endif +/* A member that cannot be measured is skipped only when the platform says it + * no longer exists (it exited between the listing and the measurement, and + * holds nothing). Any other failure is a live member whose memory a total + * would silently leave out, so the probe fails instead -- a watchdog that + * under-reports is one that quietly stopped watching. The root is strict + * either way. */ #ifdef _WIN32 +static HANDLE cbm_tree_rss_open_member(DWORD pid, bool root, DWORD *error_out) { +#ifdef CBM_ENABLE_TEST_SEAMS + int fault = cbm_tree_rss_member_fault(root); + if (fault != 0) { + *error_out = (DWORD)fault; + return NULL; + } +#else + (void)root; +#endif + /* GetProcessMemoryInfo documents PROCESS_QUERY_INFORMATION | + * PROCESS_VM_READ. PROCESS_QUERY_LIMITED_INFORMATION is a lesser + * alternative on newer Windows; the Job Object membership list + * already closes most of the pid-reuse window between listing and + * opening. */ + HANDLE member = OpenProcess(PROCESS_QUERY_INFORMATION | PROCESS_VM_READ, FALSE, pid); + *error_out = member ? ERROR_SUCCESS : GetLastError(); + return member; +} + +static bool cbm_tree_rss_member_exited(HANDLE member) { + DWORD exit_code = 0; + return GetExitCodeProcess(member, &exit_code) && exit_code != STILL_ACTIVE; +} + static cbm_proc_tree_rss_status_t cbm_subprocess_tree_rss_platform(cbm_subprocess_t *process, uint64_t *rss_bytes) { DWORD capacity = 32; @@ -510,18 +554,15 @@ static cbm_proc_tree_rss_status_t cbm_subprocess_tree_rss_platform(cbm_subproces uint64_t total = 0; DWORD measured = 0; - bool root_failed = false; - for (DWORD index = 0; index < processes->NumberOfProcessIdsInList; index++) { + bool failed = false; + for (DWORD index = 0; index < processes->NumberOfProcessIdsInList && !failed; index++) { DWORD pid = (DWORD)processes->ProcessIdList[index]; - /* GetProcessMemoryInfo documents PROCESS_QUERY_INFORMATION | - * PROCESS_VM_READ. PROCESS_QUERY_LIMITED_INFORMATION is a lesser - * alternative on newer Windows; the Job Object membership list - * already closes most of the pid-reuse window between listing and - * opening. A member we cannot open is a probe failure when it is - * the root. */ - HANDLE member = OpenProcess(PROCESS_QUERY_INFORMATION | PROCESS_VM_READ, FALSE, pid); + bool root = pid == process->process_id; + DWORD open_error = ERROR_SUCCESS; + HANDLE member = cbm_tree_rss_open_member(pid, root, &open_error); if (!member) { - root_failed = root_failed || pid == process->process_id; + /* ERROR_INVALID_PARAMETER: no process has this id any more. */ + failed = root || open_error != ERROR_INVALID_PARAMETER; continue; } PROCESS_MEMORY_COUNTERS memory; @@ -529,14 +570,14 @@ static cbm_proc_tree_rss_status_t cbm_subprocess_tree_rss_platform(cbm_subproces if (GetProcessMemoryInfo(member, &memory, sizeof(memory))) { cbm_rss_add_saturated(&total, (uint64_t)memory.WorkingSetSize); measured++; - } else if (pid == process->process_id) { - root_failed = true; + } else { + failed = root || !cbm_tree_rss_member_exited(member); } CloseHandle(member); } DWORD listed = processes->NumberOfProcessIdsInList; cbm_free(CBM_MEM_CLASS_OTHER, processes); - if (root_failed || (measured == 0 && listed > 0)) { + if (failed || (measured == 0 && listed > 0)) { return CBM_PROC_TREE_RSS_ERROR; } if (measured == 0) { @@ -546,56 +587,73 @@ static cbm_proc_tree_rss_status_t cbm_subprocess_tree_rss_platform(cbm_subproces return CBM_PROC_TREE_RSS_OK; } #elif defined(__APPLE__) -static cbm_proc_tree_rss_status_t cbm_subprocess_tree_rss_platform(cbm_subprocess_t *process, - uint64_t *rss_bytes) { - int capacity = proc_listallpids(NULL, 0); - if (capacity <= 0 || capacity > INT_MAX / (int)sizeof(pid_t) - 64) { - return CBM_PROC_TREE_RSS_ERROR; - } - capacity += 64; +/* The members of the contained process group, as the kernel lists them. An + * all-process scan cannot decide membership: proc_pidinfo answers EPERM for + * every process of another user, so "not in our group" and "in our group but + * unreadable" would look the same. Returns the member count, or -1. */ +static int cbm_tree_rss_list_group(pid_t pgid, pid_t **pids_out) { + int capacity = 64; pid_t *pids = NULL; - int count = 0; for (;;) { pid_t *grown = cbm_realloc(CBM_MEM_CLASS_OTHER, pids, (size_t)capacity * sizeof(*pids)); if (!grown) { cbm_free(CBM_MEM_CLASS_OTHER, pids); - return CBM_PROC_TREE_RSS_ERROR; + return -1; } pids = grown; - count = proc_listallpids(pids, capacity * (int)sizeof(*pids)); - if (count <= 0) { + errno = 0; + int bytes = + proc_listpids(PROC_PGRP_ONLY, (uint32_t)pgid, pids, capacity * (int)sizeof(*pids)); + if (bytes < 0 || (bytes == 0 && errno != 0)) { cbm_free(CBM_MEM_CLASS_OTHER, pids); - return CBM_PROC_TREE_RSS_ERROR; + return -1; } + int count = bytes / (int)sizeof(*pids); if (count < capacity) { - break; + *pids_out = pids; + return count; } if (capacity > INT_MAX / (int)sizeof(*pids) / 2) { cbm_free(CBM_MEM_CLASS_OTHER, pids); - return CBM_PROC_TREE_RSS_ERROR; + return -1; } capacity *= 2; } +} + +static int cbm_tree_rss_member_rusage(pid_t pid, bool root, struct rusage_info_v2 *usage) { +#ifdef CBM_ENABLE_TEST_SEAMS + int fault = cbm_tree_rss_member_fault(root); + if (fault != 0) { + errno = fault; + return -1; + } +#else + (void)root; +#endif + return proc_pid_rusage(pid, RUSAGE_INFO_V2, (rusage_info_t *)usage); +} + +static cbm_proc_tree_rss_status_t cbm_subprocess_tree_rss_platform(cbm_subprocess_t *process, + uint64_t *rss_bytes) { + pid_t *pids = NULL; + int count = cbm_tree_rss_list_group(process->pgid, &pids); + if (count < 0) { + return CBM_PROC_TREE_RSS_ERROR; + } uint64_t total = 0; int measured = 0; - bool root_failed = false; - for (int index = 0; index < count; index++) { - struct proc_bsdinfo info; + bool failed = false; + for (int index = 0; index < count && !failed; index++) { if (pids[index] <= 0) { continue; } - if (proc_pidinfo(pids[index], PROC_PIDTBSDINFO, 0, &info, sizeof(info)) != - (int)sizeof(info)) { - root_failed = root_failed || pids[index] == process->pid; - continue; - } - if ((pid_t)info.pbi_pgid != process->pgid) { - continue; - } + bool root = pids[index] == process->pid; struct rusage_info_v2 usage; - if (proc_pid_rusage(pids[index], RUSAGE_INFO_V2, (rusage_info_t *)&usage) != 0) { - root_failed = root_failed || pids[index] == process->pid; + if (cbm_tree_rss_member_rusage(pids[index], root, &usage) != 0) { + /* ESRCH: the member exited after the listing. */ + failed = root || errno != ESRCH; continue; } /* phys_footprint is what the OS charges, matching cbm_mem_charged(). @@ -606,7 +664,7 @@ static cbm_proc_tree_rss_status_t cbm_subprocess_tree_rss_platform(cbm_subproces measured++; } cbm_free(CBM_MEM_CLASS_OTHER, pids); - if (root_failed) { + if (failed) { return CBM_PROC_TREE_RSS_ERROR; } if (measured == 0) { @@ -628,22 +686,61 @@ static bool cbm_proc_pid_name(const char *name) { return true; } -static bool cbm_linux_proc_stat(const char *pid_name, pid_t *group, int64_t *rss_pages) { +typedef enum { + CBM_LINUX_STAT_OK = 0, + CBM_LINUX_STAT_GONE, /* the process exited after the /proc listing */ + CBM_LINUX_STAT_UNREADABLE, /* a process whose stat could not be read or parsed */ +} cbm_linux_stat_t; + +/* /proc//stat is world-readable, so on a default /proc mount the only + * expected failure is the exit race: ENOENT at open, ESRCH at read. Anything + * else (EACCES under hidepid=1, a parse failure) leaves the process's group + * unknown, and the probe fails closed rather than guess it is not ours. */ +static cbm_linux_stat_t cbm_linux_stat_failure(int error) { + return error == ENOENT || error == ESRCH ? CBM_LINUX_STAT_GONE : CBM_LINUX_STAT_UNREADABLE; +} + +static cbm_linux_stat_t cbm_linux_proc_stat_line(const char *pid_name, bool root, char *line, + size_t line_size) { char path[64]; int length = snprintf(path, sizeof(path), "/proc/%s/stat", pid_name); if (length <= 0 || length >= (int)sizeof(path)) { - return false; + return CBM_LINUX_STAT_UNREADABLE; } +#ifdef CBM_ENABLE_TEST_SEAMS + int fault = cbm_tree_rss_member_fault(root); + if (fault != 0) { + return cbm_linux_stat_failure(fault); + } +#else + (void)root; +#endif + errno = 0; FILE *stat_file = cbm_fopen(path, "rb"); if (!stat_file) { - return false; + return cbm_linux_stat_failure(errno); } - char stat_line[4096]; - bool read = fgets(stat_line, sizeof(stat_line), stat_file) != NULL; + errno = 0; + bool read = fgets(line, (int)line_size, stat_file) != NULL; + int read_error = errno; (void)fclose(stat_file); - char *command_end = read ? strrchr(stat_line, ')') : NULL; + if (!read) { + return read_error == ESRCH ? CBM_LINUX_STAT_GONE : CBM_LINUX_STAT_UNREADABLE; + } + return CBM_LINUX_STAT_OK; +} + +static cbm_linux_stat_t cbm_linux_proc_stat(const char *pid_name, bool root, pid_t *group, + int64_t *rss_pages) { + char stat_line[4096]; + cbm_linux_stat_t status = + cbm_linux_proc_stat_line(pid_name, root, stat_line, sizeof(stat_line)); + if (status != CBM_LINUX_STAT_OK) { + return status; + } + char *command_end = strrchr(stat_line, ')'); if (!command_end || command_end[1] != ' ') { - return false; + return CBM_LINUX_STAT_UNREADABLE; } char *save = NULL; @@ -656,7 +753,7 @@ static bool cbm_linux_proc_stat(const char *pid_name, pid_t *group, int64_t *rss char *end = NULL; long long value = strtoll(token, &end, 10); if (!end || *end != '\0') { - return false; + return CBM_LINUX_STAT_UNREADABLE; } if (field == 5) { *group = (pid_t)value; @@ -670,7 +767,7 @@ static bool cbm_linux_proc_stat(const char *pid_name, pid_t *group, int64_t *rss token = strtok_r(NULL, " ", &save); field++; } - return have_group && have_rss; + return have_group && have_rss ? CBM_LINUX_STAT_OK : CBM_LINUX_STAT_UNREADABLE; } static cbm_proc_tree_rss_status_t cbm_subprocess_tree_rss_platform(cbm_subprocess_t *process, @@ -687,17 +784,18 @@ static cbm_proc_tree_rss_status_t cbm_subprocess_tree_rss_platform(cbm_subproces uint64_t total = 0; int measured = 0; - bool root_failed = false; + bool failed = false; struct dirent *entry; - while ((entry = readdir(proc)) != NULL) { + while (!failed && (entry = readdir(proc)) != NULL) { if (!cbm_proc_pid_name(entry->d_name)) { continue; } pid_t group = 0; int64_t pages = 0; bool root_entry = strtol(entry->d_name, NULL, 10) == (long)process->pid; - if (!cbm_linux_proc_stat(entry->d_name, &group, &pages)) { - root_failed = root_failed || root_entry; + cbm_linux_stat_t status = cbm_linux_proc_stat(entry->d_name, root_entry, &group, &pages); + if (status != CBM_LINUX_STAT_OK) { + failed = root_entry || status != CBM_LINUX_STAT_GONE; continue; } if (group != process->pgid || pages < 0) { @@ -711,7 +809,7 @@ static cbm_proc_tree_rss_status_t cbm_subprocess_tree_rss_platform(cbm_subproces measured++; } (void)closedir(proc); - if (root_failed) { + if (failed) { return CBM_PROC_TREE_RSS_ERROR; } if (measured == 0) { diff --git a/src/foundation/subprocess.h b/src/foundation/subprocess.h index 94a8a2ed0..a5b44474a 100644 --- a/src/foundation/subprocess.h +++ b/src/foundation/subprocess.h @@ -159,8 +159,9 @@ cbm_proc_poll_t cbm_subprocess_poll(cbm_subprocess_t *process, cbm_proc_result_t * On macOS this is phys_footprint (what the OS holds against the process); * elsewhere it is RSS. OK returns an overflow-safe byte total, EMPTY means * the owned tree currently has no observable members, and ERROR means no - * trustworthy measurement could be obtained. Individual processes that exit - * during enumeration are ignored. */ + * trustworthy measurement could be obtained. A member that exits between the + * listing and its measurement is ignored (the platform reports it gone); any + * other member that cannot be measured is ERROR, never a smaller total. */ cbm_proc_tree_rss_status_t cbm_subprocess_tree_rss_bytes(cbm_subprocess_t *process, uint64_t *rss_bytes); bool cbm_subprocess_root_running(const cbm_subprocess_t *process); @@ -243,6 +244,10 @@ void cbm_subprocess_hold_parent_until_child_exits_for_testing(int spawns); * waitid does not report it yet. Test builds only. */ void cbm_subprocess_observe_exiting_child_for_testing(int spawns); uint64_t cbm_subprocess_rss_sum_for_testing(const uint64_t *values, size_t count); +/* Make the tree probe's measurement of every NON-root member fail with + * platform_error (errno on POSIX, GetLastError() on Windows), as a live or an + * exiting member would; 0 restores the real calls. Test builds only. */ +void cbm_subprocess_tree_rss_fault_members_for_testing(int platform_error); #endif /* How the POSIX fork+exec child closed its inherited descriptors (#1484). */ diff --git a/tests/test_subprocess.c b/tests/test_subprocess.c index 1c5039e87..e214a2c36 100644 --- a/tests/test_subprocess.c +++ b/tests/test_subprocess.c @@ -813,6 +813,17 @@ TEST(subprocess_spawn_returns_while_child_is_running) { * Waits below have no deadline: they end when the descendant is ready or the * tree stops running, so the outcome, not a timer, decides the verdict. */ +#ifdef _WIN32 +#define RSS_TREE_FAULT_GONE ERROR_INVALID_PARAMETER /* no process with that id any more */ +#define RSS_TREE_FAULT_LIVE ERROR_ACCESS_DENIED /* a member we may not open */ +#elif defined(__APPLE__) +#define RSS_TREE_FAULT_GONE ESRCH +#define RSS_TREE_FAULT_LIVE EPERM +#else +#define RSS_TREE_FAULT_GONE ENOENT +#define RSS_TREE_FAULT_LIVE EACCES +#endif + #define RSS_TREE_MIB UINT64_C(1048576) typedef struct { @@ -866,8 +877,9 @@ static bool rss_tree_finish(cbm_subprocess_t *process, rss_tree_sample_t *sample return result.tree_quiesced; } -/* Measure one tree whose descendant holds `mib` MiB. */ -static rss_tree_sample_t rss_tree_sample(int mib) { +/* Measure one tree whose descendant holds `mib` MiB, with every non-root + * member's measurement failing with `member_fault` (0 = real calls). */ +static rss_tree_sample_t rss_tree_sample(int mib, int member_fault) { rss_tree_sample_t sample = {.status = CBM_PROC_TREE_RSS_ERROR, .final_status = CBM_PROC_TREE_RSS_ERROR, .final_rss = UINT64_MAX}; @@ -891,7 +903,9 @@ static rss_tree_sample_t rss_tree_sample(int mib) { if (sample.spawned) { sample.ready = rss_tree_wait_ready(ready_path, process); if (sample.ready) { + cbm_subprocess_tree_rss_fault_members_for_testing(member_fault); sample.status = cbm_subprocess_tree_rss_bytes(process, &sample.rss); + cbm_subprocess_tree_rss_fault_members_for_testing(0); } sample.quiesced = rss_tree_finish(process, &sample); } @@ -904,8 +918,8 @@ static rss_tree_sample_t rss_tree_sample(int mib) { * the root. 128 MiB touched against a 96 MiB floor leaves room for allocator * and page-rounding noise and none for a root-only total. */ TEST(subprocess_tree_rss_measures_contained_descendants) { - rss_tree_sample_t idle = rss_tree_sample(0); - rss_tree_sample_t loaded = rss_tree_sample(128); + rss_tree_sample_t idle = rss_tree_sample(0, 0); + rss_tree_sample_t loaded = rss_tree_sample(128, 0); if (loaded.status == CBM_PROC_TREE_RSS_OK && idle.status == CBM_PROC_TREE_RSS_OK) { printf(" tree rss: idle=%llu MiB loaded=%llu MiB\n", (unsigned long long)(idle.rss / RSS_TREE_MIB), @@ -926,6 +940,29 @@ TEST(subprocess_tree_rss_measures_contained_descendants) { PASS(); } +/* A live member the platform will not let us measure must fail the probe: a + * total without it would under-report the tree. */ +TEST(subprocess_tree_rss_fails_when_a_live_member_cannot_be_measured) { + rss_tree_sample_t sample = rss_tree_sample(0, RSS_TREE_FAULT_LIVE); + ASSERT_TRUE(sample.spawned); + ASSERT_TRUE(sample.ready); + ASSERT_EQ(sample.status, CBM_PROC_TREE_RSS_ERROR); + ASSERT_TRUE(sample.quiesced); + PASS(); +} + +/* A member that exited between the listing and its measurement holds nothing: + * it is skipped and the rest of the tree (here the root) is still measured. */ +TEST(subprocess_tree_rss_skips_a_member_that_exited_mid_probe) { + rss_tree_sample_t sample = rss_tree_sample(0, RSS_TREE_FAULT_GONE); + ASSERT_TRUE(sample.spawned); + ASSERT_TRUE(sample.ready); + ASSERT_EQ(sample.status, CBM_PROC_TREE_RSS_OK); + ASSERT_TRUE(sample.rss > 0); + ASSERT_TRUE(sample.quiesced); + PASS(); +} + TEST(subprocess_natural_completion_is_cached_across_polls) { #ifdef _WIN32 SKIP_PLATFORM("POSIX /bin/sh completion-cache probe; native Job Object coverage pending"); @@ -2775,6 +2812,8 @@ SUITE(subprocess) { RUN_TEST(subprocess_run_null_bin_rejected); RUN_TEST(subprocess_spawn_returns_while_child_is_running); RUN_TEST(subprocess_tree_rss_measures_contained_descendants); + RUN_TEST(subprocess_tree_rss_fails_when_a_live_member_cannot_be_measured); + RUN_TEST(subprocess_tree_rss_skips_a_member_that_exited_mid_probe); RUN_TEST(subprocess_natural_completion_is_cached_across_polls); RUN_TEST(subprocess_cancel_is_idempotent_and_kills_ignoring_tree); RUN_TEST(subprocess_quiet_timeout_kills_ignoring_tree);