Skip to content

fix(core): remove the SIGPIPE hazard from grep -q pipelines, plus two portability gaps - #151

Open
rvalitov wants to merge 1 commit into
SamNet-dev:mainfrom
rvalitov:fix/sigpipe-and-portability
Open

rvalitov wants to merge 1 commit into
SamNet-dev:mainfrom
rvalitov:fix/sigpipe-and-portability

Conversation

@rvalitov

Copy link
Copy Markdown
Contributor

Summary

This script runs with set -eo pipefail (mtproxymax.sh:10). Under that combination, the common producer | grep -q pattern idiom is unsafe: grep -q exits the moment it matches, closing the read end of the pipe while the producer may still be writing. The producer dies of SIGPIPE, and pipefail makes the pipeline report 141 instead of grep's 0 — so the intended answer to "did it match?" is replaced by "the pipeline failed". Put a ! in front of it, as several callers do, and the wrong answer is inverted into the opposite one.

All 31 occurrences are converted. This also fixes two sites that assumed ss is installed, and removes a cleanup trap that could never work.

Why all 31, and not just the ones that look risky

The fault depends on how much data the producer still has to write, which is a property of runtime data rather than of the source. Measured:

echo "$short"  | grep -q match      -> 0 0 0 0 0                 (small string)
echo "$big"    | grep -q "^1$"      -> 141 141 141 141 141       (~1.2 MB string)
seq 1 200000   | grep -q "^1$"      -> 141 141 141 141 141       (command producer)

The same echo builtin is safe with a small string and reliably broken with a large one, because a write larger than the pipe buffer can be interrupted once grep has exited. Since "is this producer small enough?" cannot be answered from the source, converting a hand-picked subset would leave the same latent flake behind in the rest.

# before
producer | grep -q pattern
# after
producer | grep -c pattern >/dev/null

grep -c reads all of its input, so the producer never receives SIGPIPE, and the exit status keeps its usual meaning (0 = matched, 1 = no match). Combined flags are preserved (-qE-cE, -qw-cw).

The concrete consequence

is_port_available() feeds the pre-flight port check in run_proxy_container, which decides whether it is safe to start the container:

if ! is_port_available "$PROXY_PORT"; then
    ... log_error "Port ... is already in use by another process"

With the SIGPIPE, the inverted pipeline returned 0 — "the port is free" — while the port was in fact occupied, so the guard silently waved the start through. A live host measured this firing on 7 of 12 consecutive probes.

Two other callers (run_doctor, port_check) report the same inverted answer, so their "Port N listening" line is unreliable in both directions.

Two sites assumed ss exists

ss is not present on a stock Debian or Fedora (iproute2 is not in the base image) and is not on Alpine either, where only busybox's netstat is available. Two call sites had no fallback:

Site Effect without ss
connection count in get_proxy_stats always 0
_check "Port N listening" in mtproxymax health deterministic failure, 6/6 runs

Both now go through a _port_listening helper that tries ss then netstat — one awk '{print $4}' serves both, since each puts the local address in column 4 of -tln output. The helper deliberately preserves the previous "unknown ⇒ available" behaviour when neither tool can answer, so hosts that cannot tell are not blocked from starting.

A trap that could never run

The generated daemon script contained:

trap _cleanup EXIT

_cleanup is defined in the manager (:57), not in the generated script, and nothing in the daemon ever appends to _TEMP_FILES — the array is only populated on the manager side. So the trap could only ever print _cleanup: command not found on every daemon exit, while cleaning nothing. Removed, with a comment explaining why there is no trap here.

Testing

New tests/test_sigpipe_pipelines.sh:

  • pins the class — no | grep -q may remain anywhere in the script
  • demonstrates the fault and the fix side by side, so the rule above is not just a style preference
  • asserts the behavioural consequence: with a fixture whose netstat reports port 443 as listening, is_port_available 443 must report not available, and a port outside the fixture's range must still report available

Before the change it fails 2/5; after, it passes on Debian 12, Alpine 3.20 and Fedora 41, with no other test result changing.

… portability gaps

The script runs with `set -eo pipefail` (mtproxymax.sh:10). `grep -q` exits as soon as it
matches, closing the read end of the pipe while the producer may still be writing; the
producer then dies of SIGPIPE and pipefail makes the pipeline report 141 instead of grep's
0. The intended "did it match?" answer is replaced by "the pipeline failed", and a `!` in
front of it inverts that into the opposite answer.

Measured: the fault depends on how much the producer still has to write. `echo` of a short
string is consistently 0; `echo` of a ~1.2 MB string is consistently 141, as is any
command producer. Whether a given producer is "small enough" is a property of runtime
data, not of the source, so all 31 occurrences are converted rather than a hand-picked
subset. `grep -c pattern >/dev/null` reads all input, so no producer ever sees SIGPIPE and
the exit status keeps its usual meaning.

The concrete consequence, at the pre-flight check in run_proxy_container: a SIGPIPE made
is_port_available answer "the port is free" while it was occupied, so the port-conflict
guard waved the container start through.

Also:

- _port_listening helper with an ss -> netstat chain, replacing the two sites that assumed
  ss exists. On a stock Debian or Fedora neither tool is installed (iproute2 is not in the
  base image) and on Alpine only busybox netstat is, so the health check's "Port N
  listening" line and the connection count both reported a deterministic false negative.
  The helper preserves the previous "unknown => available" behaviour where nothing can
  answer, so start-up is not blocked on hosts that cannot tell.

- Removed `trap _cleanup EXIT` from the generated daemon. _cleanup is defined in the
  manager (:57) and nothing in the daemon appends to _TEMP_FILES, so the trap could only
  print "_cleanup: command not found" on every daemon exit while cleaning nothing.

tests/test_sigpipe_pipelines.sh pins the class, shows the fault and the fix side by side,
and asserts the behavioural consequence. It fails 2/5 beforehand and passes on Debian 12,
Alpine 3.20 and Fedora 41.
@rvalitov
rvalitov marked this pull request as draft September 17, 2026 18:52
@rvalitov
rvalitov marked this pull request as ready for review September 17, 2026 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant