From 132f84a48567f934299d582f5fecc50fba426be5 Mon Sep 17 00:00:00 2001 From: Rasmus Lerdorf Date: Wed, 26 Aug 2026 10:05:29 +0000 Subject: [PATCH] pgrep mode: fail fast on bad args, and stop probing processes that are not PHP Three ways `-P` wastes effort or hides a mistake. A pattern containing a space silently breaks, forever. The argument string is interpolated into a shell command, so `-P '-f phan/phan -f files'` becomes two pgrep patterns; pgrep then fails on every poll with "only one pattern can be provided" while phpspy keeps running and produces an empty output file. The exit status was never checked -- neither pclose's nor popen's, which had no else branch at all. Status >= 2 is now fatal and says what to look at ("no match" is status 1 and stays the normal idle case). The pattern usually matches phpspy itself. The text appears in phpspy's own command line, and in the shell or sudo that launched it, so phpspy attaches to those and spends four popen'd shell commands per poll discovering that bash is not a PHP process. phpspy's own pid and its ancestors are now excluded -- walking PPid from status(5), not field 4 of stat(5), whose comm field can contain spaces and parens. Non-PHP matches are re-probed on every poll forever. A readlink of /proc//exe, falling back to matching `-w` against the mappings for mod_php-style targets, gates the expensive path in two syscalls. It fails open at every ambiguity: an unreadable /proc means "try it", never "skip it", so a real target is never silently dropped. And pids that cannot be attached to are now remembered for 30s rather than re-queued on every poll. On a shared host with other users' PHP processes -- where /proc is unreadable, so the filter above correctly fails open and find_addresses then fails -- that was 84 failed attach attempts per second against 11 processes. Two strikes before sidelining a pid, because address resolution legitimately fails for a process caught between fork and exec. Together these take the same 3s run from ~250 failed attaches to 13. --- pgrep.c | 213 ++++++++++++++++++++++++++++++++++- tests/test_pgrep_bad_args.sh | 29 +++++ tests/test_pgrep_self.sh | 19 ++++ 3 files changed, 256 insertions(+), 5 deletions(-) create mode 100755 tests/test_pgrep_bad_args.sh create mode 100755 tests/test_pgrep_self.sh diff --git a/pgrep.c b/pgrep.c index cff9ffc..a64dfb8 100644 --- a/pgrep.c +++ b/pgrep.c @@ -1,5 +1,23 @@ #include "phpspy.h" +#define PHPSPY_MAX_SELF_PIDS 32 + +/* How long a pid we failed to attach to is left alone before trying again. + Bounded rather than permanent so a recycled pid, or one that becomes + traceable later, is eventually picked up. */ +#define PHPSPY_BAD_PID_RETRY_S 30 + +/* Two strikes before sidelining a pid: address resolution legitimately fails + for a process caught between fork and exec. */ +#define PHPSPY_BAD_PID_STRIKES 2 + +typedef struct bad_pid_s { + int pid; + time_t when; + int fails; + UT_hash_handle hh; +} bad_pid_t; + static int wait_for_turn(char producer_or_consumer); static void pgrep_for_pids(); static void *run_work_thread(void *arg); @@ -9,12 +27,26 @@ static void deinit_work_threads(); static int block_all_signals(); static void handle_signal(int signum); static void *run_signal_thread(void *arg); +static pid_t get_ppid(pid_t pid); +static void collect_self_pids(); +static int is_self_pid(pid_t pid); +static int pid_looks_like_php(pid_t pid); +static int is_bad_pid(int pid); +static void mark_bad_pid(int pid); +static void free_bad_pids(); static int *avail_pids = NULL; static int *attached_pids = NULL; static pthread_t *work_threads = NULL; static pthread_t signal_thread; static int avail_pids_count = 0; +static int pgrep_failed = 0; +static pid_t self_pids[PHPSPY_MAX_SELF_PIDS]; +static int self_pids_len = 0; +static regex_t libname_re; +static int libname_re_ok = 0; +static bad_pid_t *bad_pids = NULL; +static pthread_mutex_t bad_pids_mutex = PTHREAD_MUTEX_INITIALIZER; static pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER; static pthread_cond_t can_produce = PTHREAD_COND_INITIALIZER; static pthread_cond_t can_consume = PTHREAD_COND_INITIALIZER; @@ -31,6 +63,13 @@ int main_pgrep() { pthread_create(&signal_thread, NULL, run_signal_thread, NULL); block_all_signals(); + collect_self_pids(); + /* awk patterns are EREs, so `-w` works here unchanged */ + libname_re_ok = regcomp(&libname_re, opt_libname_awk_patt, REG_EXTENDED | REG_NOSUB) == 0 ? 1 : 0; + if (!libname_re_ok) { + log_error("main_pgrep: Failed to compile -w pattern; not filtering non-PHP pids by maps\n"); + } + init_work_threads(); for (i = 0; i < opt_num_workers; i++) { @@ -50,8 +89,13 @@ int main_pgrep() { deinit_work_threads(); + if (libname_re_ok) { + regfree(&libname_re); + } + free_bad_pids(); + log_error("main_pgrep finished gracefully\n"); - return 0; + return pgrep_failed ? PHPSPY_ERR : 0; } static int wait_for_turn(char producer_or_consumer) { @@ -84,6 +128,7 @@ static void pgrep_for_pids() { char line[64]; int pid; int found; + int wstatus, ec; struct timespec timeout; if (asprintf(&pgrep_cmd, "pgrep %s%s", opt_pgrep_args, opt_quiet ? " 2>/dev/null" : "") < 0) { errno = ENOMEM; @@ -93,15 +138,35 @@ static void pgrep_for_pids() { while (!done) { if (wait_for_turn('p')) break; found = 0; - if ((pcmd = popen(pgrep_cmd, "r")) != NULL) { + if ((pcmd = popen(pgrep_cmd, "r")) == NULL) { + log_perror("pgrep_for_pids: popen"); + pgrep_failed = 1; + } else { while (avail_pids_count < opt_num_workers && fgets(line, sizeof(line), pcmd) != NULL) { if (strlen(line) < 1 || *line == '\n') continue; pid = atoi(line); + if (pid < 1) continue; + if (is_self_pid(pid)) continue; if (is_already_attached(pid)) continue; + if (is_bad_pid(pid)) continue; + if (!pid_looks_like_php(pid)) continue; avail_pids[avail_pids_count++] = pid; found += 1; } - pclose(pcmd); + wstatus = pclose(pcmd); + ec = WIFEXITED(wstatus) ? WEXITSTATUS(wstatus) : -1; + /* pgrep: 0=matched, 1=no match, 2=syntax error, 3=fatal. Only a + real error is fatal here; "no match" is the normal idle case. */ + if (ec >= 2 || ec < 0) { + log_error( + "pgrep_for_pids: `%s` exited with status %d; check your -P arguments\n" + " (the argument string is word-split, so a pattern containing spaces\n" + " becomes several pgrep patterns)\n", + pgrep_cmd, + ec + ); + pgrep_failed = 1; + } } if (found > 0) { pthread_cond_broadcast(&can_consume); @@ -115,24 +180,162 @@ static void pgrep_for_pids() { ); } pthread_mutex_unlock(&mutex); + if (pgrep_failed) { + /* a bad -P never fixes itself; stop rather than looping silently */ + write_done_pipe(); + break; + } } free(pgrep_cmd); } static void *run_work_thread(void *arg) { - int worker_num; + int worker_num, pid, rv; worker_num = (long)arg; while (!done) { if (wait_for_turn('c')) break; attached_pids[worker_num] = avail_pids[--avail_pids_count]; pthread_cond_signal(&can_produce); pthread_mutex_unlock(&mutex); - main_pid(attached_pids[worker_num]); + pid = attached_pids[worker_num]; + rv = main_pid(pid); + /* main_pid only fails before it starts sampling, i.e. we could not + attach at all; remember that so the producer stops handing this pid + back to us on every poll */ + if (rv != PHPSPY_OK) { + mark_bad_pid(pid); + } attached_pids[worker_num] = 0; } return NULL; } +static pid_t get_ppid(pid_t pid) { + char path[PHPSPY_STR_SIZE]; + char line[PHPSPY_STR_SIZE]; + FILE *fp; + pid_t ppid; + + ppid = 0; + snprintf(path, sizeof(path), "/proc/%d/status", (int)pid); + if ((fp = fopen(path, "r")) == NULL) { + return 0; + } + /* status(5) rather than stat(5): the latter's comm field can contain + spaces and parens, which makes positional parsing unreliable */ + while (fgets(line, sizeof(line), fp) != NULL) { + if (strncmp(line, "PPid:", 5) == 0) { + ppid = (pid_t)atoi(line + 5); + break; + } + } + fclose(fp); + + return ppid; +} + +static void collect_self_pids() { + pid_t pid; + + /* A `-P` pattern routinely matches phpspy's own command line, and the + shell or sudo that launched it, since the pattern text appears there. + Threads share the tgid, so getpid() covers every worker. */ + self_pids_len = 0; + pid = getpid(); + while (pid > 1 && self_pids_len < PHPSPY_MAX_SELF_PIDS) { + self_pids[self_pids_len++] = pid; + pid = get_ppid(pid); + } +} + +static int is_self_pid(pid_t pid) { + int i; + for (i = 0; i < self_pids_len; i++) { + if (self_pids[i] == pid) return 1; + } + return 0; +} + +static int pid_looks_like_php(pid_t pid) { + char path[PHPSPY_STR_SIZE]; + char exe[PHPSPY_STR_SIZE]; + char line[PHPSPY_STR_SIZE]; + ssize_t len; + FILE *fp; + int found; + + /* Cheap gate in front of find_addresses, which otherwise spends four + popen'd shell commands per poll discovering that bash is not PHP. + Deliberately fails open: a false negative would silently drop a real + target, so anything we cannot determine is allowed through. */ + snprintf(path, sizeof(path), "/proc/%d/exe", (int)pid); + len = readlink(path, exe, sizeof(exe) - 1); + if (len < 0) return 1; /* not permitted to look, or already gone */ + exe[len] = '\0'; + if (strstr(exe, "php") != NULL) return 1; + if (!libname_re_ok) return 1; + + /* mod_php and friends run under another exe name, so look for the lib */ + snprintf(path, sizeof(path), "/proc/%d/maps", (int)pid); + if ((fp = fopen(path, "r")) == NULL) return 1; + found = 0; + while (fgets(line, sizeof(line), fp) != NULL) { + if (regexec(&libname_re, line, 0, NULL, 0) == 0) { + found = 1; + break; + } + } + fclose(fp); + + return found; +} + +static int is_bad_pid(int pid) { + bad_pid_t *bad; + int stale; + + /* Without this, a pid we cannot attach to -- another user's PHP process, + say, whose /proc is not readable -- is re-queued on every poll, and each + attempt spends several popen'd shell commands rediscovering that. */ + pthread_mutex_lock(&bad_pids_mutex); + HASH_FIND_INT(bad_pids, &pid, bad); + stale = bad != NULL && (time(NULL) - bad->when) >= PHPSPY_BAD_PID_RETRY_S; + if (stale) { + HASH_DEL(bad_pids, bad); + free(bad); + bad = NULL; + } + pthread_mutex_unlock(&bad_pids_mutex); + + return (bad != NULL && bad->fails >= PHPSPY_BAD_PID_STRIKES) ? 1 : 0; +} + +static void mark_bad_pid(int pid) { + bad_pid_t *bad; + + pthread_mutex_lock(&bad_pids_mutex); + HASH_FIND_INT(bad_pids, &pid, bad); + if (bad == NULL) { + if ((bad = calloc(1, sizeof(bad_pid_t))) != NULL) { + bad->pid = pid; + HASH_ADD_INT(bad_pids, pid, bad); + } + } + if (bad != NULL) { + bad->when = time(NULL); + bad->fails += 1; + } + pthread_mutex_unlock(&bad_pids_mutex); +} + +static void free_bad_pids() { + bad_pid_t *bad, *bad_tmp; + HASH_ITER(hh, bad_pids, bad, bad_tmp) { + HASH_DEL(bad_pids, bad); + free(bad); + } +} + static int is_already_attached(int pid) { int i; for (i = 0; i < opt_num_workers; i++) { diff --git a/tests/test_pgrep_bad_args.sh b/tests/test_pgrep_bad_args.sh new file mode 100755 index 0000000..f15db43 --- /dev/null +++ b/tests/test_pgrep_bad_args.sh @@ -0,0 +1,29 @@ +#!/bin/bash + +if ! command -v pgrep >/dev/null 2>&1; then + echo -e " \x1b[33mSKIP\x1b[0m pgrep not available" + exit 0 +fi + +# The -P string is word-split by the shell, so a multi-word pattern becomes +# several pgrep patterns and pgrep fails on every poll. phpspy used to loop on +# that forever while writing nothing. +err=$(timeout 15 $PHPSPY --limit=1 --pgrep '-f aaaaaaaa -f bbbbbbbb' --threads 1 2>&1 >/dev/null) +ec=$? + +if [ "$ec" -eq 124 ]; then + echo -e " \x1b[31mERR \x1b[0m pgrep_failure_is_fatal (hung)" + exit 1 +fi +if grep -q 'exited with status 2' <<<"$err" && grep -q 'word-split' <<<"$err"; then + echo -e " \x1b[32mOK \x1b[0m pgrep_failure_reported" +else + echo -e " \x1b[31mERR \x1b[0m pgrep_failure_reported\n$err" + exit 1 +fi +if [ "$ec" -ne 0 ]; then + echo -e " \x1b[32mOK \x1b[0m pgrep_failure_exit_code (exit=$ec)" +else + echo -e " \x1b[31mERR \x1b[0m pgrep_failure_exit_code expected non-zero" + exit 1 +fi diff --git a/tests/test_pgrep_self.sh b/tests/test_pgrep_self.sh new file mode 100755 index 0000000..0c48f6e --- /dev/null +++ b/tests/test_pgrep_self.sh @@ -0,0 +1,19 @@ +#!/bin/bash + +if ! command -v pgrep >/dev/null 2>&1; then + echo -e " \x1b[33mSKIP\x1b[0m pgrep not available" + exit 0 +fi + +# A -P pattern nearly always matches phpspy's own command line, since the +# pattern text appears there. phpspy used to attach to itself (and to the shell +# and sudo above it), shelling out to objdump for each one on every poll. +err=$(timeout 15 $PHPSPY --limit=1 --pgrep '-f phpspy' --threads 2 --time-limit-ms=1000 2>&1 >/dev/null) + +for pat in 'objdump' 'get_symbol_offset: Failed' 'get_php_bin_path: Failed'; do + if grep -q "$pat" <<<"$err"; then + echo -e " \x1b[31mERR \x1b[0m no_self_probe: saw '$pat'\n$err" + exit 1 + fi +done +echo -e " \x1b[32mOK \x1b[0m no_self_probe"