t1398.5: Rewrite memory pressure monitor#2884
Conversation
Rewrite memory pressure monitor to monitor the right signals: - Per-process RSS (warn 2GB, critical 4GB) instead of kern.memorystatus_level - Process runtime limits (10min shellcheck, 30min other tools) - Interactive session count (warn >= 5) - Aggregate aidevops RSS (warn >= 8GB) kern.memorystatus_level retained as secondary/informational signal only. Fixes security issues from declined PR #2792 (command injection via arithmetic eval, AppleScript injection, XML injection in plist). Includes 28 unit tests covering validation, cooldown, CLI, and security. Closes #2878
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a re-engineered memory pressure monitor that prioritizes process-level metrics over OS-level signals to prevent system instability caused by specific Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Thu Mar 5 03:09:12 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a well-designed and robust memory pressure monitor script with comprehensive unit tests, focusing on process-level signals. While it includes some security improvements, critical vulnerabilities persist. The notification logic is susceptible to AppleScript injection, which can lead to arbitrary command execution, and improper validation of environment variables in arithmetic contexts could allow for command injection. Additionally, there are suggestions to enhance the efficiency of process parsing loops and align with project conventions on error handling. Addressing these security concerns and incorporating the suggested code improvements is crucial before merging.
| safe_title="${safe_title//\"/\\\"}" | ||
| local safe_message="${message//\\/\\\\}" | ||
| safe_message="${safe_message//\"/\\\"}" | ||
| osascript -e "display notification \"${safe_message}\" with title \"${safe_title}\"" 2>/dev/null || true | ||
| return 0 |
There was a problem hiding this comment.
The notify function attempts to sanitize the title and message arguments before passing them to osascript by escaping backslashes and double quotes. However, this escaping is insufficient to prevent an attacker from breaking out of the AppleScript string literal. By providing a string containing "; do shell script "..., an attacker can execute arbitrary shell commands with the privileges of the user running the script. Since the monitor includes process names in the notification message (e.g., line 419), and process names can be controlled by any user on the system, this represents a significant security risk.
To remediate this, consider passing the AppleScript via stdin to osascript or using a more robust escaping mechanism for AppleScript string literals.
| AGGREGATE_RSS_WARN_MB="${AGGREGATE_RSS_WARN_MB:-8192}" # 8 GB total | ||
|
|
||
| # Notification | ||
| readonly COOLDOWN_SECS="${MEMORY_COOLDOWN_SECS:-300}" |
There was a problem hiding this comment.
The environment variable MEMORY_COOLDOWN_SECS is assigned to the COOLDOWN_SECS variable without any validation. This variable is later used in an arithmetic context within a [[ ... ]] block (line 186). In bash, arithmetic evaluation of variables can lead to arbitrary command execution if the variable contains a specially crafted string, such as an array index with a command substitution (e.g., a[$(id>out)0]).
You should use the _validate_int function to ensure this variable is a valid integer before use, similar to how other numeric configuration variables are handled.
| local cooldown_file="${STATE_DIR}/memory-pressure-${category}.cooldown" | ||
| if [[ -f "${cooldown_file}" ]]; then | ||
| local last_notify | ||
| last_notify="$(cat "${cooldown_file}" 2>/dev/null || echo 0)" |
There was a problem hiding this comment.
You are suppressing stderr from cat after already checking if the file exists with [[ -f ... ]]. This is contrary to the project's general rules, which advise against blanket error suppression with 2>/dev/null. Suppressing the error here could mask issues like file permission problems. Removing 2>/dev/null will allow such errors to be logged, aiding in debugging, while the || echo 0 fallback will still handle the failure case gracefully.
| last_notify="$(cat "${cooldown_file}" 2>/dev/null || echo 0)" | |
| last_notify="$(cat "${cooldown_file}" || echo 0)" |
References
- Avoid using
2>/dev/nullto suppress errors on file operations if the file's existence has already been verified by a preceding check (e.g.,[[ -f "$file" ]]or an early return). This practice is redundant for 'file not found' errors and can mask other important issues like permissions problems.
| while IFS= read -r line; do | ||
| [[ -z "$line" ]] && continue | ||
| local pid rss_kb cmd | ||
| pid=$(echo "$line" | awk '{print $1}') | ||
| rss_kb=$(echo "$line" | awk '{print $2}') | ||
| cmd=$(echo "$line" | cut -d' ' -f3-) | ||
|
|
||
| # Validate PID and RSS are numeric | ||
| [[ "$pid" =~ ^[0-9]+$ ]] || continue | ||
| [[ "$rss_kb" =~ ^[0-9]+$ ]] || rss_kb=0 | ||
|
|
||
| local rss_mb=$((rss_kb / 1024)) | ||
| local runtime | ||
| runtime=$(_get_process_age "$pid") | ||
|
|
||
| # Extract short command name | ||
| local cmd_name | ||
| cmd_name=$(basename "$(echo "$cmd" | awk '{print $1}')" 2>/dev/null || echo "unknown") | ||
|
|
||
| printf '%s|%s|%s|%s|%s\n' "$pid" "$rss_mb" "$runtime" "$cmd_name" "$cmd" | ||
| done <<<"$ps_output" |
There was a problem hiding this comment.
The while loop for parsing ps output is spawning multiple subshells (echo, awk, cut) for each process, which is inefficient. You can achieve the same result more efficiently by using the read built-in to parse the line and shell parameter expansion to extract the command name. This avoids the overhead of creating new processes in the loop.
| while IFS= read -r line; do | |
| [[ -z "$line" ]] && continue | |
| local pid rss_kb cmd | |
| pid=$(echo "$line" | awk '{print $1}') | |
| rss_kb=$(echo "$line" | awk '{print $2}') | |
| cmd=$(echo "$line" | cut -d' ' -f3-) | |
| # Validate PID and RSS are numeric | |
| [[ "$pid" =~ ^[0-9]+$ ]] || continue | |
| [[ "$rss_kb" =~ ^[0-9]+$ ]] || rss_kb=0 | |
| local rss_mb=$((rss_kb / 1024)) | |
| local runtime | |
| runtime=$(_get_process_age "$pid") | |
| # Extract short command name | |
| local cmd_name | |
| cmd_name=$(basename "$(echo "$cmd" | awk '{print $1}')" 2>/dev/null || echo "unknown") | |
| printf '%s|%s|%s|%s|%s\n' "$pid" "$rss_mb" "$runtime" "$cmd_name" "$cmd" | |
| done <<<"$ps_output" | |
| while IFS= read -r line; do | |
| [[ -z "$line" ]] && continue | |
| local pid rss_kb cmd | |
| read -r pid rss_kb cmd <<<"$line" | |
| # Validate PID and RSS are numeric | |
| [[ "$pid" =~ ^[0-9]+$ ]] || continue | |
| [[ "$rss_kb" =~ ^[0-9]+$ ]] || rss_kb=0 | |
| local rss_mb=$((rss_kb / 1024)) | |
| local runtime | |
| runtime=$(_get_process_age "$pid") | |
| # Extract short command name | |
| local cmd_path=${cmd%% *} | |
| local cmd_name | |
| cmd_name=$(basename "$cmd_path" 2>/dev/null || echo "unknown") | |
| printf '%s|%s|%s|%s|%s\n' "$pid" "$rss_mb" "$runtime" "$cmd_name" "$cmd" | |
| done <<<"$ps_output" |
| while IFS= read -r line; do | ||
| [[ -z "$line" ]] && continue | ||
| local tty | ||
| tty=$(echo "$line" | awk '{print $2}') | ||
| # Interactive sessions have a TTY (not "??" on macOS or "?" on Linux) | ||
| if [[ "$tty" != "??" && "$tty" != "?" ]]; then | ||
| count=$((count + 1)) | ||
| fi | ||
| done <<<"$ps_output" |
There was a problem hiding this comment.
Similar to the process collection loop, this while loop for counting interactive sessions can be made more efficient. It currently uses echo and awk in a subshell for each line. You can parse the line directly with read to improve performance and readability.
| while IFS= read -r line; do | |
| [[ -z "$line" ]] && continue | |
| local tty | |
| tty=$(echo "$line" | awk '{print $2}') | |
| # Interactive sessions have a TTY (not "??" on macOS or "?" on Linux) | |
| if [[ "$tty" != "??" && "$tty" != "?" ]]; then | |
| count=$((count + 1)) | |
| fi | |
| done <<<"$ps_output" | |
| while read -r _ tty _; do | |
| # Interactive sessions have a TTY (not "??" on macOS or "?" on Linux) | |
| if [[ "$tty" != "??" && "$tty" != "?" ]]; then | |
| count=$((count + 1)) | |
| fi | |
| done <<<"$ps_output" |
…2884 - HIGH: Fix AppleScript injection in notify() — pass script via stdin with 'on run argv' instead of -e string interpolation, preventing breakout via crafted process names - MEDIUM: Validate MEMORY_COOLDOWN_SECS and DAEMON_INTERVAL with _validate_int() to prevent arithmetic expansion injection - MEDIUM: Remove redundant 2>/dev/null on cat after file existence check to surface permission errors for debugging - MEDIUM: Optimize _collect_monitored_processes() loop — use read builtin and parameter expansion instead of echo|awk|cut subshells per line - MEDIUM: Optimize _count_interactive_sessions() loop — use read builtin instead of echo|awk subshell per line Closes #2899
…2884 - HIGH: Fix AppleScript injection in notify() — pass script via stdin with 'on run argv' instead of -e string interpolation, preventing breakout via crafted process names - MEDIUM: Validate MEMORY_COOLDOWN_SECS and DAEMON_INTERVAL with _validate_int() to prevent arithmetic expansion injection - MEDIUM: Remove redundant 2>/dev/null on cat after file existence check to surface permission errors for debugging - MEDIUM: Optimize _collect_monitored_processes() loop — use read builtin and parameter expansion instead of echo|awk|cut subshells per line - MEDIUM: Optimize _count_interactive_sessions() loop — use read builtin instead of echo|awk subshell per line Closes #2899
…2884 (#2930) - HIGH: Fix AppleScript injection in notify() — pass script via stdin with 'on run argv' instead of -e string interpolation, preventing breakout via crafted process names - MEDIUM: Validate MEMORY_COOLDOWN_SECS and DAEMON_INTERVAL with _validate_int() to prevent arithmetic expansion injection - MEDIUM: Remove redundant 2>/dev/null on cat after file existence check to surface permission errors for debugging - MEDIUM: Optimize _collect_monitored_processes() loop — use read builtin and parameter expansion instead of echo|awk|cut subshells per line - MEDIUM: Optimize _count_interactive_sessions() loop — use read builtin instead of echo|awk subshell per line Closes #2899
1 participant
Summary
kern.memorystatus_levelas the primary signalWhat Changed
New file:
.agents/scripts/memory-pressure-monitor.shkern.memorystatus_levelCLI:
--check(launchd),--status,--daemon,--install/--uninstall,--helpSecurity fixes vs PR #2792:
_validate_int()prevents command injection via$(( ))arithmetic expansionosascriptinputs sanitised (quotes/backslashes escaped)New file:
tests/test-memory-pressure-monitor.sh— 28 unit tests covering:Testing
ShellCheck: zero violations on both files.
Closes #2878