Skip to content

t1398.2: Harden ShellCheck invocation to prevent exponential expansion #2885

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alex-solovyev merged 1 commit into from
Mar 5, 2026
Merged

t1398.2: Harden ShellCheck invocation to prevent exponential expansion #2885

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 30 additions & 11 deletions .agents/scripts/linters-local.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,12 +310,23 @@ run_shellcheck() {
return 0
fi

# t1398: ShellCheck with -x (--external-sources) and recursive -P SCRIPTDIR
# can cause exponential expansion when 100+ scripts source each other,
# consuming 5+ GB RAM and running for 35+ minutes. Mitigations:
# 1. Per-file mode with timeout to cap each invocation
# 2. ulimit to cap RSS per shellcheck process
# 3. No --external-sources in batch mode (use -P SCRIPTDIR only)
# t1398.2: Hardened ShellCheck invocation to prevent exponential expansion.
#
# Root cause: shellcheck --external-sources (-x) with source-path=SCRIPTDIR
# follows source directives across 100+ scripts, causing exponential
# expansion (5.7 GB RSS, 88% CPU, 35+ min observed — March 3 kernel panic).
#
# Hardening layers (defense in depth):
# 1. Per-file mode with timeout (30s) to cap each invocation
# 2. ulimit -v (1 GB) to cap virtual memory per shellcheck subprocess
# 3. -P SCRIPTDIR restricts source resolution to the script's own directory
# (prevents cross-directory recursive expansion chains)
# 4. .shellcheckrc source-path=SCRIPTDIR is intentionally kept for
# interactive use — the per-file timeout + ulimit prevent runaway
#
# Trade-off: linters-local.sh keeps -x for better source resolution
# (unlike pulse-wrapper.sh which uses --norc). This is acceptable because
# linters-local.sh is interactive with per-file timeout + ulimit guards.
local violations=0
local result=""
local timed_out=0
Expand All @@ -330,14 +341,18 @@ run_shellcheck() {
fi

# Per-file mode with timeout: prevents any single file from causing
# exponential expansion. Each file gets max 30s and 1GB RSS.
# exponential expansion. Each file gets max 30s and 1GB virtual memory.
local sc_timeout=30
local file_result
for file in "${ALL_SH_FILES[@]}"; do
[[ -f "$file" ]] || continue
file_result=""
if [[ -n "$timeout_cmd" ]]; then
file_result=$($timeout_cmd "${sc_timeout}s" shellcheck -x -P SCRIPTDIR --severity=warning --format=gcc "$file" 2>&1) || {
# t1398.2: run in subshell with ulimit -v to cap virtual memory
file_result=$(
ulimit -v 1048576 2>/dev/null || true
$timeout_cmd "${sc_timeout}s" shellcheck -x -P SCRIPTDIR --severity=warning --format=gcc "$file" 2>&1
) || {
local sc_exit=$?
# Exit code 124 = timeout killed the process
if [[ $sc_exit -eq 124 ]]; then
Expand All @@ -348,20 +363,24 @@ run_shellcheck() {
}
else
# Portable timeout wrapper: no timeout/gtimeout available.
# Run shellcheck in background with a sleep-based watcher that kills it
# Run ShellCheck in background with a sleep-based watcher that kills it
# after sc_timeout seconds. Drop -x to reduce recursive expansion risk.
local sc_tmpfile
sc_tmpfile=$(mktemp) || {
file_result=""
continue
}
shellcheck -P SCRIPTDIR --severity=warning --format=gcc "$file" >"$sc_tmpfile" 2>&1 &
# t1398.2: no -x in fallback path (no timeout utility = higher risk)
(
ulimit -v 1048576 2>/dev/null || true
shellcheck -P SCRIPTDIR --severity=warning --format=gcc "$file"
) >"$sc_tmpfile" 2>&1 &
local sc_bg_pid=$!
(sleep "$sc_timeout" && kill "$sc_bg_pid" 2>/dev/null) &
local sc_watcher_pid=$!
local sc_exit_code=0
wait "$sc_bg_pid" 2>/dev/null || sc_exit_code=$?
# Clean up watcher (may already be done if shellcheck finished before timeout)
# Clean up watcher (may already be done if ShellCheck finished before timeout)
kill "$sc_watcher_pid" 2>/dev/null || true
wait "$sc_watcher_pid" 2>/dev/null || true
file_result=$(cat "$sc_tmpfile")
Expand Down
35 changes: 30 additions & 5 deletions .agents/scripts/pulse-wrapper.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -857,10 +857,11 @@ prefetch_active_workers() {
#
# Scans all child processes of the current pulse (and their descendants)
# for resource violations. ShellCheck processes get stricter limits due
# to their known exponential expansion with --external-sources.
# to their known exponential expansion risk (see t1398.2).
#
# This is the primary defense against the March 3 kernel panic scenario:
# a single shellcheck invocation consuming 5+ GB RAM for 35+ minutes.
# This is a secondary defense — the primary defense is the hardened
# ShellCheck invocation (no -x, --norc, per-file timeout, ulimit -v).
# This guard catches any ShellCheck process that escapes those limits.
#
# Called from the watchdog loop inside run_pulse() every 60s.
#
Expand Down Expand Up @@ -1843,6 +1844,26 @@ _quality_sweep_for_repo() {
local sweep_high_critical=0

# --- 1. ShellCheck ---
# t1398.2: Hardened ShellCheck invocation to prevent exponential expansion.
#
# Root cause of March 3 kernel panic: shellcheck --external-sources with
# source-path=SCRIPTDIR follows source directives across 100+ scripts,
# causing exponential expansion (5.7 GB RSS, 88% CPU, 35+ min observed).
#
# Hardening layers (defense in depth):
# 1. NEVER pass -x / --external-sources — shellcheck runs in syntax-only
# mode for the quality sweep (source directives are not followed)
# 2. --norc — ignore .shellcheckrc which sets source-path=SCRIPTDIR
# (prevents implicit source following via rc file)
# 3. Per-file timeout (30s) — caps each invocation regardless
# 4. ulimit -v to cap virtual memory per shellcheck subprocess
# 5. guard_child_processes() in the watchdog loop kills any shellcheck
# exceeding SHELLCHECK_RSS_LIMIT_KB or SHELLCHECK_RUNTIME_LIMIT
#
# Trade-off: --norc means the quality sweep won't resolve source directives,
# so SC1091 ("Not following: ... was not specified as input") will appear.
# This is acceptable — the sweep is for catching real bugs, not source
# resolution. linters-local.sh (interactive, with timeout) handles that.
local shellcheck_section=""
if command -v shellcheck &>/dev/null; then
local sh_files
Expand Down Expand Up @@ -1876,8 +1897,12 @@ _quality_sweep_for_repo() {
while IFS= read -r shfile; do
[[ -z "$shfile" ]] && continue
local result
# t1398: timeout each shellcheck invocation to prevent exponential expansion
result=$($sc_timeout_cmd shellcheck -f gcc "$shfile" || true)
# t1398.2: hardened invocation — no -x, --norc, per-file timeout,
# ulimit -v in subshell to cap RSS per shellcheck process.
result=$(
ulimit -v 1048576 2>/dev/null || true
$sc_timeout_cmd shellcheck --norc -f gcc "$shfile" 2>/dev/null || true

@gemini-code-assist gemini-code-assist Bot Mar 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The use of 2>/dev/null here suppresses all standard error output from the shellcheck command. This can hide important diagnostic information if the command fails for reasons other than finding linting issues (e.g., 'command not found', file permission errors). This practice goes against the repository's general rule to avoid blanket error suppression to aid in debugging. The || true is sufficient to prevent script termination on a non-zero exit code. Removing the redirection will allow legitimate system errors to be visible while still capturing the intended shellcheck output.

Suggested change
$sc_timeout_cmd shellcheck --norc -f gcc "$shfile" 2>/dev/null || true
$sc_timeout_cmd shellcheck --norc -f gcc "$shfile" || true
References
  1. Avoid using '2>/dev/null' for blanket suppression of command errors in shell scripts to ensure that authentication, syntax, or system issues remain visible for debugging.

)
if [[ -n "$result" ]]; then
local file_errors
file_errors=$(grep -c ':.*: error:' <<<"$result") || file_errors=0
Expand Down
6 changes: 6 additions & 0 deletions .shellcheckrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@
# Resolves SC1091 ("Not following: ... was not specified as input") by telling
# ShellCheck to look in the same directory as the script being checked.
# This handles source "./shared-constants.sh" and source "./_common.sh" patterns.
#
# WARNING (t1398.2): This directive combined with -x/--external-sources can cause
# exponential expansion across 100+ scripts (observed: 5.7 GB RSS, 35+ min).
# Automated/headless invocations (pulse-wrapper.sh quality sweep) MUST use --norc
# to ignore this file. Interactive invocations (linters-local.sh) use per-file
# timeout + ulimit -v as guards. See pulse-wrapper.sh and linters-local.sh.
source-path=SCRIPTDIR

# Project-wide accepted patterns and false positives:
Expand Down
Loading