From 5c9a5eaf8514348ed19aa762fae9c0eaa5a23802 Mon Sep 17 00:00:00 2001 From: Michael Ernst Date: Thu, 8 Oct 2026 07:19:29 -0700 Subject: [PATCH 01/28] Do not pass `cronic`'s `-u` option to the wrapped command --- cronic | 8 ++++++-- tests/cronic-test/cronic-test.sh | 22 ++++++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/cronic b/cronic index 6ecfae6..5c64f92 100755 --- a/cronic +++ b/cronic @@ -69,10 +69,14 @@ if [ "$1" == "--duplicate-stderr" ]; then DUPLICATE_STDERR=1 fi -set +e +# Run the command without this script's `-e` and `-u` options. If the caller +# exported SHELLOPTS, then the command would otherwise inherit those options, +# and a bash script that reads an unset variable (as `/usr/bin/texi2dvi` does +# when /bin/sh is bash) would fail. +set +eu "$@" > "$OUT" 2> "$TRACE" RESULT=$? -set -e +set -eu PATTERN="^${PS4:0:1}\\+${PS4:1}" if grep -q "$PATTERN" "$TRACE"; then diff --git a/tests/cronic-test/cronic-test.sh b/tests/cronic-test/cronic-test.sh index d1d8be5..9c82852 100755 --- a/tests/cronic-test/cronic-test.sh +++ b/tests/cronic-test/cronic-test.sh @@ -70,6 +70,15 @@ exit "$1" EOF chmod +x "$work/make-noise-and-stderr" +# A bash command that reads an unset variable. +cat > "$work/read-unset" <<'EOF' +#!/bin/bash +unset CRONIC_TEST_UNSET_VARIABLE +echo "value: ${CRONIC_TEST_UNSET_VARIABLE}" > /dev/null +exit "$1" +EOF +chmod +x "$work/read-unset" + # temp_files: prints `cronic`'s temporary files, in a canonical order. temp_files() { find "$TMPDIR" -mindepth 1 -maxdepth 1 2> /dev/null | sort @@ -160,4 +169,17 @@ check "make directory-change notices, exit 0" 0 silent "$work/make-noise" 0 check "make directory-change notices and real stderr, exit 0" 0 \ "report:^a real error$" "$work/make-noise-and-stderr" 0 +# When SHELLOPTS is exported, `cronic`'s own `-u` option does not reach the +# wrapped command. +cat > "$work/cronic-with-shellopts" < Date: Thu, 8 Oct 2026 07:32:31 -0700 Subject: [PATCH 02/28] Run the command under `cronic` with the caller's shell options; report usage errors Also make the `cronic` test independent of BASH_ENV and test errexit and caller-exported options. --- cronic | 42 +++++++++++----- tests/cronic-test/cronic-test.sh | 84 +++++++++++++++++++++++--------- 2 files changed, 91 insertions(+), 35 deletions(-) diff --git a/cronic b/cronic index 5c64f92..ec0a7ce 100755 --- a/cronic +++ b/cronic @@ -13,6 +13,18 @@ # Extended by Michael Ernst to add --expected-status, --permit-stderr, and --duplicate-stderr flags. # --duplicate-stderr prints stderr twice so that it is less likely to be overlooked. +# The shell options in effect before this script changes any of them. The +# wrapped command runs with exactly these options, so that the options this +# script sets for itself (such as `-e` and `-u` below) do not reach the command +# through an exported SHELLOPTS or BASHOPTS, whereas options that the caller +# exported on purpose do. +CALLER_SHELL_OPTIONS="$(set +o; shopt -p)" +# Bash turns off `-e` within a command substitution, so `set +o` above always +# reports it as off. +case $- in + *e*) CALLER_SHELL_OPTIONS+=$'\nset -o errexit' ;; +esac + # Exit if a command exits with a non-zero status, and treat unset variables # as an error. set -eu @@ -52,31 +64,39 @@ trap 'cleanup; trap - HUP EXIT; kill -s HUP $$' HUP trap 'cleanup; trap - INT EXIT; kill -s INT $$' INT trap 'cleanup; trap - TERM EXIT; kill -s TERM $$' TERM +usage() { + echo "Usage: $0 [--expected-status N] [--permit-stderr] [--duplicate-stderr] COMMAND [ARG...]" >&2 + exit 2 +} + EXPECTED=0 -if [ "$1" == "--expected-status" ]; then +if [ "${1:-}" == "--expected-status" ]; then shift + [ $# -gt 0 ] || usage EXPECTED=$1 shift fi PERMITSTDERR=0 -if [ "$1" == "--permit-stderr" ]; then +if [ "${1:-}" == "--permit-stderr" ]; then shift PERMITSTDERR=1 fi DUPLICATE_STDERR=0 -if [ "$1" == "--duplicate-stderr" ]; then +if [ "${1:-}" == "--duplicate-stderr" ]; then shift DUPLICATE_STDERR=1 fi +[ $# -gt 0 ] || usage -# Run the command without this script's `-e` and `-u` options. If the caller -# exported SHELLOPTS, then the command would otherwise inherit those options, -# and a bash script that reads an unset variable (as `/usr/bin/texi2dvi` does -# when /bin/sh is bash) would fail. -set +eu -"$@" > "$OUT" 2> "$TRACE" -RESULT=$? -set -eu +# Run the command with the caller's shell options rather than this script's. +# If the caller exported SHELLOPTS, then the command would otherwise inherit +# this script's `-e` and `-u` options, and a bash script that reads an unset +# variable (as `/usr/bin/texi2dvi` does when /bin/sh is bash) would fail. +RESULT=0 +( + eval "$CALLER_SHELL_OPTIONS" + exec "$@" +) > "$OUT" 2> "$TRACE" || RESULT=$? PATTERN="^${PS4:0:1}\\+${PS4:1}" if grep -q "$PATTERN" "$TRACE"; then diff --git a/tests/cronic-test/cronic-test.sh b/tests/cronic-test/cronic-test.sh index 9c82852..c4b15bd 100755 --- a/tests/cronic-test/cronic-test.sh +++ b/tests/cronic-test/cronic-test.sh @@ -12,6 +12,11 @@ set -eu +# Don't let the user's environment file run in `cronic` or in the bash commands +# below. It could write to stderr, especially when an exported SHELLOPTS turns +# on `-u` for it. +unset BASH_ENV + SCRIPT_DIR="$(CDPATH='' cd -- "$(dirname -- "$0")" && pwd -P)" CRONIC="$(CDPATH='' cd -- "${SCRIPT_DIR}/../.." && pwd -P)/cronic" @@ -79,6 +84,23 @@ exit "$1" EOF chmod +x "$work/read-unset" +# A bash command in which a command fails before the script exits. +cat > "$work/ignore-failure" <<'EOF' +#!/bin/bash +false +exit "$1" +EOF +chmod +x "$work/ignore-failure" + +# Runs `cronic` with SHELLOPTS set to its first argument and exported. +cat > "$work/cronic-with-shellopts" <<'EOF' +#!/bin/sh +shellopts="$1" +shift +SHELLOPTS="$shellopts" exec "$REAL_CRONIC" "$@" +EOF +chmod +x "$work/cronic-with-shellopts" + # temp_files: prints `cronic`'s temporary files, in a canonical order. temp_files() { find "$TMPDIR" -mindepth 1 -maxdepth 1 2> /dev/null | sort @@ -86,8 +108,10 @@ temp_files() { # check DESCRIPTION EXPECTED-STATUS EXPECTED-OUTPUT COMMAND...: runs `cronic` # on COMMAND and checks its exit status, whether it printed a report, and that -# it left no temporary files behind. EXPECTED-OUTPUT is "silent", "report", or -# "report:TEXT", which also requires TEXT to appear in the report. +# it left no temporary files behind. EXPECTED-OUTPUT is "silent", "report", +# "report:TEXT", which also requires TEXT to appear in the report, or +# "message:TEXT", which requires TEXT to appear in output that need not be a +# report. check() { description="$1" expected_status="$2" @@ -113,25 +137,25 @@ check() { status=1 return fi - else + elif [ "${expected_output#message:}" = "$expected_output" ]; then if ! grep -q "^END OF CRONIC OUTPUT.$" "$work/output"; then echo "FAIL: $description: expected a report, but got:" cat "$work/output" status=1 return fi - case "$expected_output" in - report:*) - if ! grep -q "${expected_output#report:}" "$work/output"; then - echo "FAIL: $description: expected the report to contain" \ - "\"${expected_output#report:}\", but got:" - cat "$work/output" - status=1 - return - fi - ;; - esac fi + case "$expected_output" in + report:* | message:*) + if ! grep -q "${expected_output#*:}" "$work/output"; then + echo "FAIL: $description: expected the output to contain" \ + "\"${expected_output#*:}\", but got:" + cat "$work/output" + status=1 + return + fi + ;; + esac if [ "$before" != "$after" ]; then echo "FAIL: $description: temporary files were left behind:" @@ -169,17 +193,29 @@ check "make directory-change notices, exit 0" 0 silent "$work/make-noise" 0 check "make directory-change notices and real stderr, exit 0" 0 \ "report:^a real error$" "$work/make-noise-and-stderr" 0 -# When SHELLOPTS is exported, `cronic`'s own `-u` option does not reach the -# wrapped command. -cat > "$work/cronic-with-shellopts" < Date: Thu, 8 Oct 2026 07:45:05 -0700 Subject: [PATCH 03/28] Restore only the caller's `-e` and `-u` around the wrapped command Run the command as `"$@"` in cronic's own shell, so that it can again be a shell function or builtin, and so that restoring options adds no xtrace noise to the command's error output. Reject a non-integer `--expected-status`, and exit with status 64 (EX_USAGE) on a usage error. --- cronic | 47 ++++++++++++++++++------------ tests/cronic-test/cronic-test.sh | 50 +++++++++++++++++++++++++------- 2 files changed, 69 insertions(+), 28 deletions(-) diff --git a/cronic b/cronic index ec0a7ce..f203b1f 100755 --- a/cronic +++ b/cronic @@ -13,16 +13,15 @@ # Extended by Michael Ernst to add --expected-status, --permit-stderr, and --duplicate-stderr flags. # --duplicate-stderr prints stderr twice so that it is less likely to be overlooked. -# The shell options in effect before this script changes any of them. The -# wrapped command runs with exactly these options, so that the options this -# script sets for itself (such as `-e` and `-u` below) do not reach the command -# through an exported SHELLOPTS or BASHOPTS, whereas options that the caller -# exported on purpose do. -CALLER_SHELL_OPTIONS="$(set +o; shopt -p)" -# Bash turns off `-e` within a command substitution, so `set +o` above always -# reports it as off. +# Which of `-e` and `-u` the caller had on, before this script turns them on +# for itself. The wrapped command runs with the caller's settings of these +# options rather than this script's; see where it is run, below. +CALLER_E_U_OPTIONS= case $- in - *e*) CALLER_SHELL_OPTIONS+=$'\nset -o errexit' ;; + *e*) CALLER_E_U_OPTIONS+=e ;; +esac +case $- in + *u*) CALLER_E_U_OPTIONS+=u ;; esac # Exit if a command exits with a non-zero status, and treat unset variables @@ -64,9 +63,13 @@ trap 'cleanup; trap - HUP EXIT; kill -s HUP $$' HUP trap 'cleanup; trap - INT EXIT; kill -s INT $$' INT trap 'cleanup; trap - TERM EXIT; kill -s TERM $$' TERM +# Exits with status 64 (EX_USAGE in sysexits.h) rather than the conventional 2, +# because a wrapped command is much more likely to exit with status 2, and a +# caller that checks for a particular status should not mistake a misuse of +# this script for the command's own result. usage() { echo "Usage: $0 [--expected-status N] [--permit-stderr] [--duplicate-stderr] COMMAND [ARG...]" >&2 - exit 2 + exit 64 } EXPECTED=0 @@ -75,6 +78,10 @@ if [ "${1:-}" == "--expected-status" ]; then [ $# -gt 0 ] || usage EXPECTED=$1 shift + if ! [[ $EXPECTED =~ ^[0-9]+$ ]]; then + echo "$0: --expected-status requires a non-negative integer, got \"$EXPECTED\"" >&2 + usage + fi fi PERMITSTDERR=0 if [ "${1:-}" == "--permit-stderr" ]; then @@ -88,15 +95,19 @@ if [ "${1:-}" == "--duplicate-stderr" ]; then fi [ $# -gt 0 ] || usage -# Run the command with the caller's shell options rather than this script's. -# If the caller exported SHELLOPTS, then the command would otherwise inherit -# this script's `-e` and `-u` options, and a bash script that reads an unset -# variable (as `/usr/bin/texi2dvi` does when /bin/sh is bash) would fail. +# Run the command with the caller's `-e` and `-u` settings rather than this +# script's. If the caller exported SHELLOPTS, then the command would otherwise +# inherit this script's `-e` and `-u` options, and a bash script that reads an +# unset variable (as `/usr/bin/texi2dvi` does when /bin/sh is bash) would fail. +# The command runs in this shell, not via `exec` in a subshell, so that it can +# be a shell function or builtin. RESULT=0 -( - eval "$CALLER_SHELL_OPTIONS" - exec "$@" -) > "$OUT" 2> "$TRACE" || RESULT=$? +set +eu +if [ -n "$CALLER_E_U_OPTIONS" ]; then + set "-$CALLER_E_U_OPTIONS" +fi +"$@" > "$OUT" 2> "$TRACE" || RESULT=$? +set -eu PATTERN="^${PS4:0:1}\\+${PS4:1}" if grep -q "$PATTERN" "$TRACE"; then diff --git a/tests/cronic-test/cronic-test.sh b/tests/cronic-test/cronic-test.sh index c4b15bd..ce8e25c 100755 --- a/tests/cronic-test/cronic-test.sh +++ b/tests/cronic-test/cronic-test.sh @@ -12,7 +12,7 @@ set -eu -# Don't let the user's environment file run in `cronic` or in the bash commands +# Do not let the user's environment file run in `cronic` or in the bash commands # below. It could write to stderr, especially when an exported SHELLOPTS turns # on `-u` for it. unset BASH_ENV @@ -53,7 +53,7 @@ chmod +x "$work/trace-and-stderr" # A command whose stderr is nothing but `make` directory-change notices. Both # the top-level form (`make:`) and the recursive form (`make[1]:`) appear; both # must be filtered out of the reduced error output. -cat > "$work/make-noise" <<'EOF' +cat > "$work/make-noise" << 'EOF' #!/bin/sh echo "make: Entering directory '/tmp/x'" 1>&2 echo "make[1]: Entering directory '/tmp/x/sub'" 1>&2 @@ -64,7 +64,7 @@ EOF chmod +x "$work/make-noise" # A command that writes a real error among the `make` directory-change notices. -cat > "$work/make-noise-and-stderr" <<'EOF' +cat > "$work/make-noise-and-stderr" << 'EOF' #!/bin/sh echo "make: Entering directory '/tmp/x'" 1>&2 echo "make[1]: Entering directory '/tmp/x/sub'" 1>&2 @@ -76,7 +76,7 @@ EOF chmod +x "$work/make-noise-and-stderr" # A bash command that reads an unset variable. -cat > "$work/read-unset" <<'EOF' +cat > "$work/read-unset" << 'EOF' #!/bin/bash unset CRONIC_TEST_UNSET_VARIABLE echo "value: ${CRONIC_TEST_UNSET_VARIABLE}" > /dev/null @@ -85,7 +85,7 @@ EOF chmod +x "$work/read-unset" # A bash command in which a command fails before the script exits. -cat > "$work/ignore-failure" <<'EOF' +cat > "$work/ignore-failure" << 'EOF' #!/bin/bash false exit "$1" @@ -93,11 +93,11 @@ EOF chmod +x "$work/ignore-failure" # Runs `cronic` with SHELLOPTS set to its first argument and exported. -cat > "$work/cronic-with-shellopts" <<'EOF' +cat > "$work/cronic-with-shellopts" << 'EOF' #!/bin/sh shellopts="$1" shift -SHELLOPTS="$shellopts" exec "$REAL_CRONIC" "$@" +exec env SHELLOPTS="$shellopts" "$REAL_CRONIC" "$@" EOF chmod +x "$work/cronic-with-shellopts" @@ -195,12 +195,21 @@ check "make directory-change notices and real stderr, exit 0" 0 \ # Without a command, `cronic` reports a usage error rather than aborting on an # unset variable. -check "no arguments" 2 "message:^Usage: " -check "--expected-status without a value" 2 "message:^Usage: " \ +check "no arguments" 64 "message:^Usage: " +check "--expected-status without a value" 64 "message:^Usage: " \ --expected-status -check "--permit-stderr without a command" 2 "message:^Usage: " \ +check "--permit-stderr without a command" 64 "message:^Usage: " \ --permit-stderr +# A non-integer expected status is a usage error, rather than a value that +# makes every exit status look expected. +check "--expected-status with a non-integer value" 64 \ + "message:requires a non-negative integer" \ + --expected-status x "$work/trace-only" 1 + +# The command may be a shell builtin. +check "builtin command" 0 silent : + # When SHELLOPTS is exported, `cronic`'s own `-e` and `-u` options do not reach # the wrapped command, but the caller's options do. REAL_CRONIC="$CRONIC" @@ -216,6 +225,27 @@ check "exported SHELLOPTS with nounset, command reads an unset variable" 1 \ "$default_shellopts:nounset" "$work/read-unset" 0 check "exported SHELLOPTS with errexit, command ignores a failure" 1 report \ "$default_shellopts:errexit" "$work/ignore-failure" 0 +# With xtrace exported, the report's trace section is the command's own trace, +# with nothing that `cronic` did to set up the command's options. `cronic`'s +# own trace goes to its stderr, which is discarded here. +"$CRONIC" "$default_shellopts:xtrace" "$work/trace-and-stderr" 0 \ + > "$work/output" 2> /dev/null +sed -n '/^TRACE-ERROR OUTPUT:$/,/^$/p' "$work/output" > "$work/trace-section" +cat > "$work/trace-section.goal" << 'EOF' +TRACE-ERROR OUTPUT: ++ set -x ++ echo 'a real error' +a real error ++ exit 0 + +EOF +if cmp -s "$work/trace-section.goal" "$work/trace-section"; then + echo "PASS: exported SHELLOPTS with xtrace, trace output" +else + echo "FAIL: exported SHELLOPTS with xtrace, trace output: got:" + cat "$work/output" + status=1 +fi CRONIC="$REAL_CRONIC" exit "$status" From ec112d5876bde40180781686d9c9bdf90502147e Mon Sep 17 00:00:00 2001 From: Michael Ernst Date: Thu, 8 Oct 2026 08:01:15 -0700 Subject: [PATCH 04/28] Address code review: validate --expected-status range, accept options in any order, run the command in a subshell --- cronic | 98 +++++++++++++++++++------------- tests/cronic-test/cronic-test.sh | 39 ++++++++++++- 2 files changed, 96 insertions(+), 41 deletions(-) diff --git a/cronic b/cronic index f203b1f..72f0ea1 100755 --- a/cronic +++ b/cronic @@ -9,20 +9,19 @@ # any non-trace error output or a non-zero result code. Cronic filters # Bash execution traces (or anything matching PS4) from the error output, # so jobs can be run with execution tracing to aid forensic -# debugging. Cronic has no options, it simply executes its arguments. -# Extended by Michael Ernst to add --expected-status, --permit-stderr, and --duplicate-stderr flags. +# debugging. Apart from the options below, Cronic simply executes its +# arguments. +# Extended by Michael Ernst to add the --expected-status, --permit-stderr, and +# --duplicate-stderr options, which may be given in any order. +# --expected-status N treats exit status N (0 to 255), rather than 0, as success. +# --permit-stderr does not treat error output as a failure. # --duplicate-stderr prints stderr twice so that it is less likely to be overlooked. +# -- ends the options, so that the command may start with "--". -# Which of `-e` and `-u` the caller had on, before this script turns them on -# for itself. The wrapped command runs with the caller's settings of these -# options rather than this script's; see where it is run, below. -CALLER_E_U_OPTIONS= -case $- in - *e*) CALLER_E_U_OPTIONS+=e ;; -esac -case $- in - *u*) CALLER_E_U_OPTIONS+=u ;; -esac +# The caller's shell options, before this script turns on `-e` and `-u` for +# itself. The wrapped command runs with the caller's settings of these options +# rather than this script's; see where it is run, below. +CALLER_OPTIONS=$- # Exit if a command exits with a non-zero status, and treat unset variables # as an error. @@ -68,46 +67,67 @@ trap 'cleanup; trap - TERM EXIT; kill -s TERM $$' TERM # caller that checks for a particular status should not mistake a misuse of # this script for the command's own result. usage() { - echo "Usage: $0 [--expected-status N] [--permit-stderr] [--duplicate-stderr] COMMAND [ARG...]" >&2 + echo "Usage: $0 [--expected-status N] [--permit-stderr] [--duplicate-stderr] [--] COMMAND [ARG...]" >&2 exit 64 } EXPECTED=0 -if [ "${1:-}" == "--expected-status" ]; then - shift - [ $# -gt 0 ] || usage - EXPECTED=$1 - shift - if ! [[ $EXPECTED =~ ^[0-9]+$ ]]; then - echo "$0: --expected-status requires a non-negative integer, got \"$EXPECTED\"" >&2 - usage - fi -fi PERMITSTDERR=0 -if [ "${1:-}" == "--permit-stderr" ]; then - shift - PERMITSTDERR=1 -fi DUPLICATE_STDERR=0 -if [ "${1:-}" == "--duplicate-stderr" ]; then - shift - DUPLICATE_STDERR=1 -fi +while [ $# -gt 0 ]; do + case $1 in + --expected-status) + shift + [ $# -gt 0 ] || usage + EXPECTED=$1 + shift + # No leading zeros, because `[[ ... -eq ... ]]` would read the value as + # octal. At most 255, because no exit status is larger. + if ! [[ $EXPECTED =~ ^(0|[1-9][0-9]{0,2})$ ]] || [ "$EXPECTED" -gt 255 ]; then + echo "$0: --expected-status requires an integer from 0 to 255, got \"$EXPECTED\"" >&2 + usage + fi + ;; + --permit-stderr) + shift + PERMITSTDERR=1 + ;; + --duplicate-stderr) + shift + DUPLICATE_STDERR=1 + ;; + --) + shift + break + ;; + --*) + echo "$0: unknown option \"$1\"" >&2 + usage + ;; + *) + break + ;; + esac +done [ $# -gt 0 ] || usage # Run the command with the caller's `-e` and `-u` settings rather than this # script's. If the caller exported SHELLOPTS, then the command would otherwise # inherit this script's `-e` and `-u` options, and a bash script that reads an # unset variable (as `/usr/bin/texi2dvi` does when /bin/sh is bash) would fail. -# The command runs in this shell, not via `exec` in a subshell, so that it can -# be a shell function or builtin. +# The command runs in a subshell, rather than via `exec`, so that it can be a +# shell builtin or an exported shell function; and in a subshell rather than in +# this shell, so that a builtin such as `exit` or `exec` cannot end this script +# before it reports. The redirections are on the command itself, not on the +# subshell, so that the trace of the `set` commands goes to this script's +# stderr rather than into the command's. RESULT=0 -set +eu -if [ -n "$CALLER_E_U_OPTIONS" ]; then - set "-$CALLER_E_U_OPTIONS" -fi -"$@" > "$OUT" 2> "$TRACE" || RESULT=$? -set -eu +( + set +eu + case $CALLER_OPTIONS in *e*) set -e ;; esac + case $CALLER_OPTIONS in *u*) set -u ;; esac + "$@" > "$OUT" 2> "$TRACE" +) || RESULT=$? PATTERN="^${PS4:0:1}\\+${PS4:1}" if grep -q "$PATTERN" "$TRACE"; then diff --git a/tests/cronic-test/cronic-test.sh b/tests/cronic-test/cronic-test.sh index ce8e25c..fbfd51e 100755 --- a/tests/cronic-test/cronic-test.sh +++ b/tests/cronic-test/cronic-test.sh @@ -204,11 +204,34 @@ check "--permit-stderr without a command" 64 "message:^Usage: " \ # A non-integer expected status is a usage error, rather than a value that # makes every exit status look expected. check "--expected-status with a non-integer value" 64 \ - "message:requires a non-negative integer" \ + "message:requires an integer from 0 to 255" \ --expected-status x "$work/trace-only" 1 +# So is a value that no exit status can equal, or that the shell's integer +# comparisons cannot handle. +check "--expected-status larger than 255" 64 \ + "message:requires an integer from 0 to 255" \ + --expected-status 256 "$work/trace-only" 1 +check "--expected-status too large for an integer comparison" 64 \ + "message:requires an integer from 0 to 255" \ + --expected-status 99999999999999999999 "$work/trace-only" 1 +check "--expected-status with a leading zero" 64 \ + "message:requires an integer from 0 to 255" \ + --expected-status 09 "$work/trace-only" 1 + +# The options may be given in any order. +check "options out of order" 3 silent \ + --permit-stderr --expected-status 3 "$work/trace-and-stderr" 3 +# An unknown option is a usage error, rather than a command to run. +check "unknown option" 64 "message:unknown option" \ + --no-such-option "$work/trace-only" 0 +# `--` ends the options. +check "-- before the command" 0 silent -- "$work/trace-only" 0 # The command may be a shell builtin. check "builtin command" 0 silent : +# A builtin that would end the shell does not prevent the report. +check "exit builtin" 3 report exit 3 +check "exec builtin" 3 report exec "$work/trace-only" 3 # When SHELLOPTS is exported, `cronic`'s own `-e` and `-u` options do not reach # the wrapped command, but the caller's options do. @@ -228,8 +251,20 @@ check "exported SHELLOPTS with errexit, command ignores a failure" 1 report \ # With xtrace exported, the report's trace section is the command's own trace, # with nothing that `cronic` did to set up the command's options. `cronic`'s # own trace goes to its stderr, which is discarded here. +before="$(temp_files)" +xtrace_status=0 "$CRONIC" "$default_shellopts:xtrace" "$work/trace-and-stderr" 0 \ - > "$work/output" 2> /dev/null + > "$work/output" 2> /dev/null || xtrace_status=$? +after="$(temp_files)" +if [ "$xtrace_status" != 0 ]; then + echo "FAIL: exported SHELLOPTS with xtrace: exit status $xtrace_status, expected 0" + status=1 +fi +if [ "$before" != "$after" ]; then + echo "FAIL: exported SHELLOPTS with xtrace: temporary files were left behind:" + echo "$after" + status=1 +fi sed -n '/^TRACE-ERROR OUTPUT:$/,/^$/p' "$work/output" > "$work/trace-section" cat > "$work/trace-section.goal" << 'EOF' TRACE-ERROR OUTPUT: From 9bbaff55b1087a51592924e8b47c0135e4ebf43f Mon Sep 17 00:00:00 2001 From: Michael Ernst Date: Thu, 8 Oct 2026 08:14:52 -0700 Subject: [PATCH 05/28] Address code review: honor the caller's `-e` in shell functions, do not trace cronic itself, add `--help` and `--expected-status=N` --- cronic | 75 +++++++++++++++++++++++--------- tests/cronic-test/cronic-test.sh | 52 +++++++++++++++++++--- 2 files changed, 102 insertions(+), 25 deletions(-) diff --git a/cronic b/cronic index 72f0ea1..67563cd 100755 --- a/cronic +++ b/cronic @@ -13,15 +13,21 @@ # arguments. # Extended by Michael Ernst to add the --expected-status, --permit-stderr, and # --duplicate-stderr options, which may be given in any order. -# --expected-status N treats exit status N (0 to 255), rather than 0, as success. +# --expected-status N (or --expected-status=N) treats exit status N (0 to 255), +# rather than 0, as success. # --permit-stderr does not treat error output as a failure. # --duplicate-stderr prints stderr twice so that it is less likely to be overlooked. # -- ends the options, so that the command may start with "--". +# -h or --help prints a usage message. # The caller's shell options, before this script turns on `-e` and `-u` for # itself. The wrapped command runs with the caller's settings of these options # rather than this script's; see where it is run, below. -CALLER_OPTIONS=$- +# This script does not trace itself even if the caller exported SHELLOPTS with +# xtrace: the trace would go to this script's stderr, and cron would send +# email for every run. The redirection discards the trace of these two +# commands. +{ CALLER_OPTIONS=$-; set +x; } 2> /dev/null # Exit if a command exits with a non-zero status, and treat unset variables # as an error. @@ -71,6 +77,30 @@ usage() { exit 64 } +# Prints a usage message to standard output and exits with status 0. +help() { + echo "Usage: $0 [--expected-status N] [--permit-stderr] [--duplicate-stderr] [--] COMMAND [ARG...]" + echo "Runs COMMAND, and produces output only if it fails or writes to stderr." + echo " --expected-status N treat exit status N (0 to 255), rather than 0, as success" + echo " --permit-stderr do not treat error output as a failure" + echo " --duplicate-stderr print the error output twice in the report" + echo " -- end the options" + echo " -h, --help print this message" + exit 0 +} + +# Sets EXPECTED to its argument, or reports a usage error if the argument is +# not a valid exit status. +set_expected() { + EXPECTED=$1 + # No leading zeros, because `[[ ... -eq ... ]]` would read the value as + # octal. At most 255, because no exit status is larger. + if ! [[ $EXPECTED =~ ^(0|[1-9][0-9]{0,2})$ ]] || [ "$EXPECTED" -gt 255 ]; then + echo "$0: --expected-status requires an integer from 0 to 255, got \"$EXPECTED\"" >&2 + usage + fi +} + EXPECTED=0 PERMITSTDERR=0 DUPLICATE_STDERR=0 @@ -79,14 +109,12 @@ while [ $# -gt 0 ]; do --expected-status) shift [ $# -gt 0 ] || usage - EXPECTED=$1 + set_expected "$1" + shift + ;; + --expected-status=*) + set_expected "${1#*=}" shift - # No leading zeros, because `[[ ... -eq ... ]]` would read the value as - # octal. At most 255, because no exit status is larger. - if ! [[ $EXPECTED =~ ^(0|[1-9][0-9]{0,2})$ ]] || [ "$EXPECTED" -gt 255 ]; then - echo "$0: --expected-status requires an integer from 0 to 255, got \"$EXPECTED\"" >&2 - usage - fi ;; --permit-stderr) shift @@ -96,6 +124,9 @@ while [ $# -gt 0 ]; do shift DUPLICATE_STDERR=1 ;; + -h | --help) + help + ;; --) shift break @@ -111,23 +142,27 @@ while [ $# -gt 0 ]; do done [ $# -gt 0 ] || usage -# Run the command with the caller's `-e` and `-u` settings rather than this -# script's. If the caller exported SHELLOPTS, then the command would otherwise -# inherit this script's `-e` and `-u` options, and a bash script that reads an +# Run the command with the caller's `-e`, `-u`, and `-x` settings rather than +# this script's. If the caller exported SHELLOPTS, then the command would +# otherwise inherit this script's options, and a bash script that reads an # unset variable (as `/usr/bin/texi2dvi` does when /bin/sh is bash) would fail. # The command runs in a subshell, rather than via `exec`, so that it can be a # shell builtin or an exported shell function; and in a subshell rather than in # this shell, so that a builtin such as `exit` or `exec` cannot end this script -# before it reports. The redirections are on the command itself, not on the -# subshell, so that the trace of the `set` commands goes to this script's -# stderr rather than into the command's. -RESULT=0 +# before it reports. The subshell is not the left operand of `||`, because +# bash would then ignore `set -e` throughout the subshell, including in a +# shell function that it runs. The redirections are on the subshell, so that +# when `-x` is set, the trace of running the command goes into the command's +# trace output rather than to this script's stderr. +set +e ( - set +eu case $CALLER_OPTIONS in *e*) set -e ;; esac - case $CALLER_OPTIONS in *u*) set -u ;; esac - "$@" > "$OUT" 2> "$TRACE" -) || RESULT=$? + case $CALLER_OPTIONS in *u*) ;; *) set +u ;; esac + case $CALLER_OPTIONS in *x*) set -x ;; esac + "$@" +) > "$OUT" 2> "$TRACE" +RESULT=$? +set -e PATTERN="^${PS4:0:1}\\+${PS4:1}" if grep -q "$PATTERN" "$TRACE"; then diff --git a/tests/cronic-test/cronic-test.sh b/tests/cronic-test/cronic-test.sh index fbfd51e..5277575 100755 --- a/tests/cronic-test/cronic-test.sh +++ b/tests/cronic-test/cronic-test.sh @@ -101,6 +101,22 @@ exec env SHELLOPTS="$shellopts" "$REAL_CRONIC" "$@" EOF chmod +x "$work/cronic-with-shellopts" +# Runs `cronic` with SHELLOPTS set to its first argument and exported, and +# with an exported shell function `fail_then_continue` that runs `false` and +# then prints a line. +cat > "$work/cronic-with-function" << 'EOF2' +#!/bin/bash +fail_then_continue() { + false + echo "after the failure" +} +export -f fail_then_continue +shellopts="$1" +shift +exec env SHELLOPTS="$shellopts" "$REAL_CRONIC" "$@" +EOF2 +chmod +x "$work/cronic-with-function" + # temp_files: prints `cronic`'s temporary files, in a canonical order. temp_files() { find "$TMPDIR" -mindepth 1 -maxdepth 1 2> /dev/null | sort @@ -224,6 +240,14 @@ check "options out of order" 3 silent \ # An unknown option is a usage error, rather than a command to run. check "unknown option" 64 "message:unknown option" \ --no-such-option "$work/trace-only" 0 +# The value of `--expected-status` may follow an equals sign. +check "--expected-status=N" 3 silent --expected-status=3 "$work/trace-only" 3 +check "--expected-status= with a non-integer value" 64 \ + "message:requires an integer from 0 to 255" \ + --expected-status=x "$work/trace-only" 1 +# `-h` and `--help` print a usage message. +check "--help" 0 "message:^Usage: " --help +check "-h" 0 "message:^Usage: " -h # `--` ends the options. check "-- before the command" 0 silent -- "$work/trace-only" 0 @@ -248,26 +272,36 @@ check "exported SHELLOPTS with nounset, command reads an unset variable" 1 \ "$default_shellopts:nounset" "$work/read-unset" 0 check "exported SHELLOPTS with errexit, command ignores a failure" 1 report \ "$default_shellopts:errexit" "$work/ignore-failure" 0 -# With xtrace exported, the report's trace section is the command's own trace, -# with nothing that `cronic` did to set up the command's options. `cronic`'s -# own trace goes to its stderr, which is discarded here. +# With xtrace exported, a successful command with only trace output on stderr +# produces no output at all: `cronic` does not trace itself. +check "exported SHELLOPTS with xtrace, trace-only stderr" 0 silent \ + "$default_shellopts:xtrace" "$work/trace-only" 0 +# With xtrace exported, the report's trace section is the trace of running the +# command, followed by the command's own trace, with nothing that `cronic` did +# to set up the command's options. before="$(temp_files)" xtrace_status=0 "$CRONIC" "$default_shellopts:xtrace" "$work/trace-and-stderr" 0 \ - > "$work/output" 2> /dev/null || xtrace_status=$? + > "$work/output" 2> "$work/stderr" || xtrace_status=$? after="$(temp_files)" if [ "$xtrace_status" != 0 ]; then echo "FAIL: exported SHELLOPTS with xtrace: exit status $xtrace_status, expected 0" status=1 fi +if [ -s "$work/stderr" ]; then + echo "FAIL: exported SHELLOPTS with xtrace: cronic wrote to stderr:" + cat "$work/stderr" + status=1 +fi if [ "$before" != "$after" ]; then echo "FAIL: exported SHELLOPTS with xtrace: temporary files were left behind:" echo "$after" status=1 fi sed -n '/^TRACE-ERROR OUTPUT:$/,/^$/p' "$work/output" > "$work/trace-section" -cat > "$work/trace-section.goal" << 'EOF' +cat > "$work/trace-section.goal" << EOF TRACE-ERROR OUTPUT: ++ $work/trace-and-stderr 0 + set -x + echo 'a real error' a real error @@ -281,6 +315,14 @@ else cat "$work/output" status=1 fi + +# The caller's `-e` applies within an exported shell function that is the +# command, and the caller's lack of `-e` does too. +CRONIC="$work/cronic-with-function" +check "exported SHELLOPTS with errexit, exported function ignores a failure" 1 \ + report "$default_shellopts:errexit" fail_then_continue +check "exported SHELLOPTS, exported function ignores a failure" 0 silent \ + "$default_shellopts" fail_then_continue CRONIC="$REAL_CRONIC" exit "$status" From a9235f1b60b865b5a611de49650899f651440095 Mon Sep 17 00:00:00 2001 From: Michael Ernst Date: Thu, 8 Oct 2026 09:12:10 -0700 Subject: [PATCH 06/28] Parse `cronic`'s options in any order, and report usage errors * Accept `--expected-status N`, `--expected-status=N`, `--permit-stderr`, and `--duplicate-stderr` in any order, and `--` to end the options. * Require `--expected-status` to be an integer from 0 to 255. * Report a missing command or an unknown option (including a single-dash one such as `-v`) as a usage error, with exit status 64. * Add `-h` and `--help`. * Parse the options before creating the temporary directory. * Prefix `cronic`'s function names with `cronic_`, so that they do not hide a wrapped command named, for example, `usage` or `cleanup`. --- cronic | 123 +++++++++++++++++++++++++------ tests/cronic-test/cronic-test.sh | 108 +++++++++++++++++++++++---- 2 files changed, 191 insertions(+), 40 deletions(-) diff --git a/cronic b/cronic index 6ecfae6..fcddf0b 100755 --- a/cronic +++ b/cronic @@ -9,14 +9,106 @@ # any non-trace error output or a non-zero result code. Cronic filters # Bash execution traces (or anything matching PS4) from the error output, # so jobs can be run with execution tracing to aid forensic -# debugging. Cronic has no options, it simply executes its arguments. -# Extended by Michael Ernst to add --expected-status, --permit-stderr, and --duplicate-stderr flags. +# debugging. Apart from the options below, Cronic simply executes its +# arguments. +# Extended by Michael Ernst to add the --expected-status, --permit-stderr, and +# --duplicate-stderr options, which may be given in any order. +# --expected-status N (or --expected-status=N) treats exit status N (0 to 255), +# rather than 0, as success. +# --permit-stderr does not treat error output as a failure. # --duplicate-stderr prints stderr twice so that it is less likely to be overlooked. +# -- ends the options, so that the command may start with "-". +# -h or --help prints a usage message. # Exit if a command exits with a non-zero status, and treat unset variables # as an error. set -eu +# The names of this script's functions start with "cronic_". The wrapped +# command runs in this shell, so a function named, say, `usage` would run in +# place of a command of the same name. + +# Prints the usage line. +cronic_usage_line() { + echo "Usage: $0 [--expected-status N] [--permit-stderr] [--duplicate-stderr] [--] COMMAND [ARG...]" +} + +# Exits with status 64 (EX_USAGE in sysexits.h) rather than the conventional 2, +# because a wrapped command is much more likely to exit with status 2, and a +# caller that checks for a particular status should not mistake a misuse of +# this script for the command's own result. +cronic_usage() { + cronic_usage_line >&2 + exit 64 +} + +# Prints a usage message to standard output and exits with status 0. +cronic_help() { + cronic_usage_line + echo "Runs COMMAND, and produces output only if it fails or writes to stderr." + echo " --expected-status N treat exit status N (0 to 255), rather than 0, as success" + echo " --permit-stderr do not treat error output as a failure" + echo " --duplicate-stderr print the error output twice in the report" + echo " -- end the options" + echo " -h, --help print this message" + exit 0 +} + +# Sets EXPECTED to its argument, or reports a usage error if the argument is +# not a valid exit status. +cronic_set_expected() { + EXPECTED=$1 + # No leading zeros, because `[[ ... -eq ... ]]` would read the value as + # octal. At most 255, because no exit status is larger. + if ! [[ $EXPECTED =~ ^(0|[1-9][0-9]{0,2})$ ]] || [ "$EXPECTED" -gt 255 ]; then + echo "$0: --expected-status requires an integer from 0 to 255, got \"$EXPECTED\"" >&2 + cronic_usage + fi +} + +# The options are parsed before the temporary directory is created, so that +# `--help` and a usage error neither create nor depend on it. +EXPECTED=0 +PERMITSTDERR=0 +DUPLICATE_STDERR=0 +while [ $# -gt 0 ]; do + case $1 in + --expected-status) + shift + [ $# -gt 0 ] || cronic_usage + cronic_set_expected "$1" + shift + ;; + --expected-status=*) + cronic_set_expected "${1#*=}" + shift + ;; + --permit-stderr) + shift + PERMITSTDERR=1 + ;; + --duplicate-stderr) + shift + DUPLICATE_STDERR=1 + ;; + -h | --help) + cronic_help + ;; + --) + shift + break + ;; + -*) + echo "$0: unknown option \"$1\"" >&2 + cronic_usage + ;; + *) + break + ;; + esac +done +[ $# -gt 0 ] || cronic_usage + DEBUG=false # DEBUG=true @@ -35,10 +127,10 @@ TRACE=$CRONIC_TMPDIR/trace # retained; don't "echo" their location, because that would be appended to # stderr. # shellcheck disable=SC2329 # the function is invoked from the trap handlers below. -cleanup() { +cronic_cleanup() { [ "$DEBUG" = "true" ] || rm -rf "$CRONIC_TMPDIR" } -trap cleanup EXIT +trap cronic_cleanup EXIT # Clean up and then die of the signal, rather than exiting with status # 128+signum, so that a supervisor can distinguish "cronic was killed by @@ -48,26 +140,9 @@ trap cleanup EXIT # installed, bash dies as soon as the signal arrives, abandoning the wrapped # command and deleting the directory that the command is still writing into, # whereas a handler does not run until the wrapped command has finished. -trap 'cleanup; trap - HUP EXIT; kill -s HUP $$' HUP -trap 'cleanup; trap - INT EXIT; kill -s INT $$' INT -trap 'cleanup; trap - TERM EXIT; kill -s TERM $$' TERM - -EXPECTED=0 -if [ "$1" == "--expected-status" ]; then - shift - EXPECTED=$1 - shift -fi -PERMITSTDERR=0 -if [ "$1" == "--permit-stderr" ]; then - shift - PERMITSTDERR=1 -fi -DUPLICATE_STDERR=0 -if [ "$1" == "--duplicate-stderr" ]; then - shift - DUPLICATE_STDERR=1 -fi +trap 'cronic_cleanup; trap - HUP EXIT; kill -s HUP $$' HUP +trap 'cronic_cleanup; trap - INT EXIT; kill -s INT $$' INT +trap 'cronic_cleanup; trap - TERM EXIT; kill -s TERM $$' TERM set +e "$@" > "$OUT" 2> "$TRACE" diff --git a/tests/cronic-test/cronic-test.sh b/tests/cronic-test/cronic-test.sh index d1d8be5..6c171bd 100755 --- a/tests/cronic-test/cronic-test.sh +++ b/tests/cronic-test/cronic-test.sh @@ -48,7 +48,7 @@ chmod +x "$work/trace-and-stderr" # A command whose stderr is nothing but `make` directory-change notices. Both # the top-level form (`make:`) and the recursive form (`make[1]:`) appear; both # must be filtered out of the reduced error output. -cat > "$work/make-noise" <<'EOF' +cat > "$work/make-noise" << 'EOF' #!/bin/sh echo "make: Entering directory '/tmp/x'" 1>&2 echo "make[1]: Entering directory '/tmp/x/sub'" 1>&2 @@ -59,7 +59,7 @@ EOF chmod +x "$work/make-noise" # A command that writes a real error among the `make` directory-change notices. -cat > "$work/make-noise-and-stderr" <<'EOF' +cat > "$work/make-noise-and-stderr" << 'EOF' #!/bin/sh echo "make: Entering directory '/tmp/x'" 1>&2 echo "make[1]: Entering directory '/tmp/x/sub'" 1>&2 @@ -77,8 +77,10 @@ temp_files() { # check DESCRIPTION EXPECTED-STATUS EXPECTED-OUTPUT COMMAND...: runs `cronic` # on COMMAND and checks its exit status, whether it printed a report, and that -# it left no temporary files behind. EXPECTED-OUTPUT is "silent", "report", or -# "report:TEXT", which also requires TEXT to appear in the report. +# it left no temporary files behind. EXPECTED-OUTPUT is "silent", "report", +# "report:TEXT", which also requires TEXT to appear in the report, or +# "message:TEXT", which requires TEXT to appear in output that need not be a +# report. check() { description="$1" expected_status="$2" @@ -104,25 +106,25 @@ check() { status=1 return fi - else + elif [ "${expected_output#message:}" = "$expected_output" ]; then if ! grep -q "^END OF CRONIC OUTPUT.$" "$work/output"; then echo "FAIL: $description: expected a report, but got:" cat "$work/output" status=1 return fi - case "$expected_output" in - report:*) - if ! grep -q "${expected_output#report:}" "$work/output"; then - echo "FAIL: $description: expected the report to contain" \ - "\"${expected_output#report:}\", but got:" - cat "$work/output" - status=1 - return - fi - ;; - esac fi + case "$expected_output" in + report:* | message:*) + if ! grep -q "${expected_output#*:}" "$work/output"; then + echo "FAIL: $description: expected the output to contain" \ + "\"${expected_output#*:}\", but got:" + cat "$work/output" + status=1 + return + fi + ;; + esac if [ "$before" != "$after" ]; then echo "FAIL: $description: temporary files were left behind:" @@ -160,4 +162,78 @@ check "make directory-change notices, exit 0" 0 silent "$work/make-noise" 0 check "make directory-change notices and real stderr, exit 0" 0 \ "report:^a real error$" "$work/make-noise-and-stderr" 0 +# Without a command, `cronic` reports a usage error rather than aborting on an +# unset variable. +check "no arguments" 64 "message:^Usage: " +check "--expected-status without a value" 64 "message:^Usage: " \ + --expected-status +check "--permit-stderr without a command" 64 "message:^Usage: " \ + --permit-stderr + +# A non-integer expected status is a usage error, rather than a value that +# makes every exit status look expected. +check "--expected-status with a non-integer value" 64 \ + "message:requires an integer from 0 to 255" \ + --expected-status x "$work/trace-only" 1 +# So is a value that no exit status can equal, or that the shell's integer +# comparisons cannot handle. +check "--expected-status larger than 255" 64 \ + "message:requires an integer from 0 to 255" \ + --expected-status 256 "$work/trace-only" 1 +check "--expected-status too large for an integer comparison" 64 \ + "message:requires an integer from 0 to 255" \ + --expected-status 99999999999999999999 "$work/trace-only" 1 +check "--expected-status with a leading zero" 64 \ + "message:requires an integer from 0 to 255" \ + --expected-status 09 "$work/trace-only" 1 + +# The options may be given in any order. +check "options out of order" 3 silent \ + --permit-stderr --expected-status 3 "$work/trace-and-stderr" 3 +# An unknown option is a usage error, rather than a command to run. +check "unknown option" 64 "message:unknown option" \ + --no-such-option "$work/trace-only" 0 +check "unknown single-dash option" 64 "message:unknown option" \ + -v "$work/trace-only" 0 +# The value of `--expected-status` may follow an equals sign. +check "--expected-status=N" 3 silent --expected-status=3 "$work/trace-only" 3 +check "--expected-status= with a non-integer value" 64 \ + "message:requires an integer from 0 to 255" \ + --expected-status=x "$work/trace-only" 1 +# `-h` and `--help` print a usage message. +check "--help" 0 "message:^Usage: " --help +check "-h" 0 "message:^Usage: " -h +# `--` ends the options, so the command may start with "-". +check "-- before the command" 0 silent -- "$work/trace-only" 0 +ln -s "$work/trace-only" "$work/-dash-command" +saved_path="$PATH" +PATH="$work:$PATH" +check "-- before a command that starts with a dash" 3 report -- -dash-command 3 +PATH="$saved_path" + +# `--help` and a usage error work even when no temporary directory can be +# created. +saved_tmpdir="$TMPDIR" +TMPDIR="$work/no-such-directory" +check "--help without a temporary directory" 0 "message:^Usage: " --help +check "usage error without a temporary directory" 64 "message:^Usage: " +TMPDIR="$saved_tmpdir" + +# A command whose name is a plain word that a script might use for a shell +# function runs the program of that name, not one of `cronic`'s functions. +mkdir "$work/bin" +for name in cleanup set_expected usage; do + cat > "$work/bin/$name" << 'EOF' +#!/bin/sh +exit 3 +EOF + chmod +x "$work/bin/$name" +done +saved_path="$PATH" +PATH="$work/bin:$PATH" +check "command named cleanup" 3 silent --expected-status 3 cleanup +check "command named set_expected" 3 silent --expected-status 3 set_expected +check "command named usage" 3 silent --expected-status 3 usage +PATH="$saved_path" + exit "$status" From a630decc99714731ac5297b80cf803c86e1eba76 Mon Sep 17 00:00:00 2001 From: Michael Ernst Date: Thu, 8 Oct 2026 09:13:56 -0700 Subject: [PATCH 07/28] Move option parsing to its own pull request; restore all of the caller's shell options * Option parsing and usage errors are now in #130. * Run the command with all of the caller's shell options, as `set +o` reports them, rather than only `-e`, `-u`, and `-x`. * Turn off tracing of `cronic` only when xtrace came from an exported SHELLOPTS, so that `bash -x cronic` still traces `cronic`. * Prefix `cronic`'s variables with `CRONIC_`, so that `cronic` does not change a variable such as `DEBUG` or `OUT` that the caller exported, and unset them before running the command, so that a builtin or shell function does not see them. --- cronic | 201 ++++++++++++------------------- tests/cronic-test/cronic-test.sh | 121 +++++++++---------- 2 files changed, 135 insertions(+), 187 deletions(-) diff --git a/cronic b/cronic index 67563cd..36a2fc4 100755 --- a/cronic +++ b/cronic @@ -9,50 +9,51 @@ # any non-trace error output or a non-zero result code. Cronic filters # Bash execution traces (or anything matching PS4) from the error output, # so jobs can be run with execution tracing to aid forensic -# debugging. Apart from the options below, Cronic simply executes its -# arguments. -# Extended by Michael Ernst to add the --expected-status, --permit-stderr, and -# --duplicate-stderr options, which may be given in any order. -# --expected-status N (or --expected-status=N) treats exit status N (0 to 255), -# rather than 0, as success. -# --permit-stderr does not treat error output as a failure. +# debugging. Cronic has no options, it simply executes its arguments. +# Extended by Michael Ernst to add --expected-status, --permit-stderr, and --duplicate-stderr flags. # --duplicate-stderr prints stderr twice so that it is less likely to be overlooked. -# -- ends the options, so that the command may start with "--". -# -h or --help prints a usage message. -# The caller's shell options, before this script turns on `-e` and `-u` for -# itself. The wrapped command runs with the caller's settings of these options -# rather than this script's; see where it is run, below. -# This script does not trace itself even if the caller exported SHELLOPTS with -# xtrace: the trace would go to this script's stderr, and cron would send -# email for every run. The redirection discards the trace of these two -# commands. -{ CALLER_OPTIONS=$-; set +x; } 2> /dev/null +# The caller's shell options, as commands that restore them, before this +# script changes any of them for itself. The wrapped command runs with the +# caller's options rather than this script's; see where it is run, below. +# If the caller exported SHELLOPTS with xtrace, then this script does not trace +# itself: the trace would go to this script's stderr, and cron would send +# email for every run. `bash -x cronic` does not export SHELLOPTS, so it still +# traces this script. The redirection discards the trace of these commands. +# `set +o` reports `-e` as off, because bash turns `-e` off in a command +# substitution, so `$-` supplies it. +{ + CRONIC_CALLER_OPTIONS=$(set +o) + case $- in + *e*) CRONIC_CALLER_OPTIONS=${CRONIC_CALLER_OPTIONS/set +o errexit/set -o errexit} ;; + esac + case ${SHELLOPTS@a} in *x*) set +x ;; esac +} 2> /dev/null # Exit if a command exits with a non-zero status, and treat unset variables # as an error. set -eu -DEBUG=false -# DEBUG=true +CRONIC_DEBUG=false +# CRONIC_DEBUG=true # A private directory, rather than predictable `/tmp/cronic.*.$$` names: on a # shared machine, process ids are guessable and reused, so another user could # pre-create those paths as symlinks and have the wrapped command's output # written through them. CRONIC_TMPDIR=$(mktemp -d "${TMPDIR:-/tmp}/cronic.XXXXXX") || exit 1 -OUT=$CRONIC_TMPDIR/out -ERR=$CRONIC_TMPDIR/err -REDUCED_ERR=$CRONIC_TMPDIR/err.reduced -TRACE=$CRONIC_TMPDIR/trace +CRONIC_OUT=$CRONIC_TMPDIR/out +CRONIC_ERR=$CRONIC_TMPDIR/err +CRONIC_REDUCED_ERR=$CRONIC_TMPDIR/err.reduced +CRONIC_TRACE=$CRONIC_TMPDIR/trace # Remove the temporary files however this script terminates, including when it -# is interrupted before it reaches the end. When DEBUG is true, the files are -# retained; don't "echo" their location, because that would be appended to -# stderr. +# is interrupted before it reaches the end. When CRONIC_DEBUG is true, the +# files are retained; don't "echo" their location, because that would be +# appended to stderr. # shellcheck disable=SC2329 # the function is invoked from the trap handlers below. cleanup() { - [ "$DEBUG" = "true" ] || rm -rf "$CRONIC_TMPDIR" + [ "$CRONIC_DEBUG" = "true" ] || rm -rf "$CRONIC_TMPDIR" } trap cleanup EXIT @@ -68,84 +69,27 @@ trap 'cleanup; trap - HUP EXIT; kill -s HUP $$' HUP trap 'cleanup; trap - INT EXIT; kill -s INT $$' INT trap 'cleanup; trap - TERM EXIT; kill -s TERM $$' TERM -# Exits with status 64 (EX_USAGE in sysexits.h) rather than the conventional 2, -# because a wrapped command is much more likely to exit with status 2, and a -# caller that checks for a particular status should not mistake a misuse of -# this script for the command's own result. -usage() { - echo "Usage: $0 [--expected-status N] [--permit-stderr] [--duplicate-stderr] [--] COMMAND [ARG...]" >&2 - exit 64 -} - -# Prints a usage message to standard output and exits with status 0. -help() { - echo "Usage: $0 [--expected-status N] [--permit-stderr] [--duplicate-stderr] [--] COMMAND [ARG...]" - echo "Runs COMMAND, and produces output only if it fails or writes to stderr." - echo " --expected-status N treat exit status N (0 to 255), rather than 0, as success" - echo " --permit-stderr do not treat error output as a failure" - echo " --duplicate-stderr print the error output twice in the report" - echo " -- end the options" - echo " -h, --help print this message" - exit 0 -} - -# Sets EXPECTED to its argument, or reports a usage error if the argument is -# not a valid exit status. -set_expected() { - EXPECTED=$1 - # No leading zeros, because `[[ ... -eq ... ]]` would read the value as - # octal. At most 255, because no exit status is larger. - if ! [[ $EXPECTED =~ ^(0|[1-9][0-9]{0,2})$ ]] || [ "$EXPECTED" -gt 255 ]; then - echo "$0: --expected-status requires an integer from 0 to 255, got \"$EXPECTED\"" >&2 - usage - fi -} - -EXPECTED=0 -PERMITSTDERR=0 -DUPLICATE_STDERR=0 -while [ $# -gt 0 ]; do - case $1 in - --expected-status) - shift - [ $# -gt 0 ] || usage - set_expected "$1" - shift - ;; - --expected-status=*) - set_expected "${1#*=}" - shift - ;; - --permit-stderr) - shift - PERMITSTDERR=1 - ;; - --duplicate-stderr) - shift - DUPLICATE_STDERR=1 - ;; - -h | --help) - help - ;; - --) - shift - break - ;; - --*) - echo "$0: unknown option \"$1\"" >&2 - usage - ;; - *) - break - ;; - esac -done -[ $# -gt 0 ] || usage +CRONIC_EXPECTED=0 +if [ "$1" == "--expected-status" ]; then + shift + CRONIC_EXPECTED=$1 + shift +fi +CRONIC_PERMITSTDERR=0 +if [ "$1" == "--permit-stderr" ]; then + shift + CRONIC_PERMITSTDERR=1 +fi +CRONIC_DUPLICATE_STDERR=0 +if [ "$1" == "--duplicate-stderr" ]; then + shift + CRONIC_DUPLICATE_STDERR=1 +fi -# Run the command with the caller's `-e`, `-u`, and `-x` settings rather than -# this script's. If the caller exported SHELLOPTS, then the command would -# otherwise inherit this script's options, and a bash script that reads an -# unset variable (as `/usr/bin/texi2dvi` does when /bin/sh is bash) would fail. +# Run the command with all of the caller's shell options rather than this +# script's. If the caller exported SHELLOPTS, then the command would otherwise +# inherit this script's options, and a bash script that reads an unset variable +# (as `/usr/bin/texi2dvi` does when /bin/sh is bash) would fail. # The command runs in a subshell, rather than via `exec`, so that it can be a # shell builtin or an exported shell function; and in a subshell rather than in # this shell, so that a builtin such as `exit` or `exec` cannot end this script @@ -154,71 +98,76 @@ done # shell function that it runs. The redirections are on the subshell, so that # when `-x` is set, the trace of running the command goes into the command's # trace output rather than to this script's stderr. +# The subshell unsets every variable that this script has set, so that a +# builtin or shell function that is the command does not see them. The +# `unset` runs within the `eval`, whose argument is expanded first; the +# `unset` thus precedes the restoring of the caller's `-x` and is not traced. set +e ( - case $CALLER_OPTIONS in *e*) set -e ;; esac - case $CALLER_OPTIONS in *u*) ;; *) set +u ;; esac - case $CALLER_OPTIONS in *x*) set -x ;; esac + eval "unset -v CRONIC_CALLER_OPTIONS CRONIC_DEBUG CRONIC_TMPDIR CRONIC_OUT \ + CRONIC_ERR CRONIC_REDUCED_ERR CRONIC_TRACE CRONIC_EXPECTED \ + CRONIC_PERMITSTDERR CRONIC_DUPLICATE_STDERR + $CRONIC_CALLER_OPTIONS" "$@" -) > "$OUT" 2> "$TRACE" +) > "$CRONIC_OUT" 2> "$CRONIC_TRACE" RESULT=$? set -e PATTERN="^${PS4:0:1}\\+${PS4:1}" -if grep -q "$PATTERN" "$TRACE"; then +if grep -q "$PATTERN" "$CRONIC_TRACE"; then # `|| true` because `grep -v` exits with status 1 when it selects no lines, # which happens whenever stderr consists only of trace lines. Without it, # `set -e` would abort the script here, before it produces its report, # exits with the wrapped command's status, and removes its temporary files. - grep -v "$PATTERN" "$TRACE" > "$ERR" || true + grep -v "$PATTERN" "$CRONIC_TRACE" > "$CRONIC_ERR" || true else - ERR=$TRACE + CRONIC_ERR=$CRONIC_TRACE fi # Certain Unix programs send output to stderr even when a command is successful! # For example, git does this, and the POSIX standard requires it for make. -# The full stderr output is shown, but file REDUCED_ERR is checked for emptiness below. +# The full stderr output is shown, but file CRONIC_REDUCED_ERR is checked for emptiness below. # TODO: Expand this. -cat "$ERR" \ +cat "$CRONIC_ERR" \ | grep -v "^Everything up-to-date$" \ | grep -v '^make\(\[[0-9]\+\]\)\?: \(Entering\|Leaving\) directory '"'"'.*'"'"'$' \ - > "$REDUCED_ERR" || true + > "$CRONIC_REDUCED_ERR" || true -if [ $RESULT -ne "$EXPECTED" ] || { [ $PERMITSTDERR -eq 0 ] && [ -s "$REDUCED_ERR" ]; }; then +if [ $RESULT -ne "$CRONIC_EXPECTED" ] || { [ $CRONIC_PERMITSTDERR -eq 0 ] && [ -s "$CRONIC_REDUCED_ERR" ]; }; then echo "Cronic detected failure or error output for the command:" echo "$@" echo - if [[ $EXPECTED -eq 0 ]]; then + if [[ $CRONIC_EXPECTED -eq 0 ]]; then if [[ $RESULT -eq 0 ]]; then echo "RESULT CODE: $RESULT (success)" else echo "RESULT CODE: $RESULT (failure)" fi else - echo "RESULT CODE: $RESULT (expected: $EXPECTED)" + echo "RESULT CODE: $RESULT (expected: $CRONIC_EXPECTED)" fi echo echo "ERROR OUTPUT:" - cat "$ERR" + cat "$CRONIC_ERR" echo - if [ "$DEBUG" != "true" ]; then - if ! cmp --silent "$ERR" "$REDUCED_ERR"; then + if [ "$CRONIC_DEBUG" != "true" ]; then + if ! cmp --silent "$CRONIC_ERR" "$CRONIC_REDUCED_ERR"; then echo "REDUCED ERROR OUTPUT:" - cat "$REDUCED_ERR" + cat "$CRONIC_REDUCED_ERR" echo fi fi echo "STANDARD OUTPUT:" - cat "$OUT" - if [ "$TRACE" != "$ERR" ]; then + cat "$CRONIC_OUT" + if [ "$CRONIC_TRACE" != "$CRONIC_ERR" ]; then echo echo "TRACE-ERROR OUTPUT:" - cat "$TRACE" + cat "$CRONIC_TRACE" fi - if [ $DUPLICATE_STDERR -eq 1 ]; then + if [ $CRONIC_DUPLICATE_STDERR -eq 1 ]; then echo echo "ERROR OUTPUT:" - cat "$ERR" + cat "$CRONIC_ERR" fi echo echo "END OF CRONIC OUTPUT." diff --git a/tests/cronic-test/cronic-test.sh b/tests/cronic-test/cronic-test.sh index 5277575..43c4c60 100755 --- a/tests/cronic-test/cronic-test.sh +++ b/tests/cronic-test/cronic-test.sh @@ -102,15 +102,25 @@ EOF chmod +x "$work/cronic-with-shellopts" # Runs `cronic` with SHELLOPTS set to its first argument and exported, and -# with an exported shell function `fail_then_continue` that runs `false` and -# then prints a line. +# with these exported shell functions: +# * `fail_then_continue` runs `false` and then prints a line. +# * `fail_in_pipeline` runs a pipeline whose first command fails. +# * `report_cronic_variables` writes to stderr any variable of `cronic`'s that +# it can see. cat > "$work/cronic-with-function" << 'EOF2' #!/bin/bash fail_then_continue() { false echo "after the failure" } -export -f fail_then_continue +fail_in_pipeline() { + false | true +} +report_cronic_variables() { + set | grep '^CRONIC_' >&2 + return 0 +} +export -f fail_then_continue fail_in_pipeline report_cronic_variables shellopts="$1" shift exec env SHELLOPTS="$shellopts" "$REAL_CRONIC" "$@" @@ -124,10 +134,8 @@ temp_files() { # check DESCRIPTION EXPECTED-STATUS EXPECTED-OUTPUT COMMAND...: runs `cronic` # on COMMAND and checks its exit status, whether it printed a report, and that -# it left no temporary files behind. EXPECTED-OUTPUT is "silent", "report", -# "report:TEXT", which also requires TEXT to appear in the report, or -# "message:TEXT", which requires TEXT to appear in output that need not be a -# report. +# it left no temporary files behind. EXPECTED-OUTPUT is "silent", "report", or +# "report:TEXT", which also requires TEXT to appear in the report. check() { description="$1" expected_status="$2" @@ -153,25 +161,25 @@ check() { status=1 return fi - elif [ "${expected_output#message:}" = "$expected_output" ]; then + else if ! grep -q "^END OF CRONIC OUTPUT.$" "$work/output"; then echo "FAIL: $description: expected a report, but got:" cat "$work/output" status=1 return fi + case "$expected_output" in + report:*) + if ! grep -q "${expected_output#report:}" "$work/output"; then + echo "FAIL: $description: expected the report to contain" \ + "\"${expected_output#report:}\", but got:" + cat "$work/output" + status=1 + return + fi + ;; + esac fi - case "$expected_output" in - report:* | message:*) - if ! grep -q "${expected_output#*:}" "$work/output"; then - echo "FAIL: $description: expected the output to contain" \ - "\"${expected_output#*:}\", but got:" - cat "$work/output" - status=1 - return - fi - ;; - esac if [ "$before" != "$after" ]; then echo "FAIL: $description: temporary files were left behind:" @@ -209,48 +217,6 @@ check "make directory-change notices, exit 0" 0 silent "$work/make-noise" 0 check "make directory-change notices and real stderr, exit 0" 0 \ "report:^a real error$" "$work/make-noise-and-stderr" 0 -# Without a command, `cronic` reports a usage error rather than aborting on an -# unset variable. -check "no arguments" 64 "message:^Usage: " -check "--expected-status without a value" 64 "message:^Usage: " \ - --expected-status -check "--permit-stderr without a command" 64 "message:^Usage: " \ - --permit-stderr - -# A non-integer expected status is a usage error, rather than a value that -# makes every exit status look expected. -check "--expected-status with a non-integer value" 64 \ - "message:requires an integer from 0 to 255" \ - --expected-status x "$work/trace-only" 1 -# So is a value that no exit status can equal, or that the shell's integer -# comparisons cannot handle. -check "--expected-status larger than 255" 64 \ - "message:requires an integer from 0 to 255" \ - --expected-status 256 "$work/trace-only" 1 -check "--expected-status too large for an integer comparison" 64 \ - "message:requires an integer from 0 to 255" \ - --expected-status 99999999999999999999 "$work/trace-only" 1 -check "--expected-status with a leading zero" 64 \ - "message:requires an integer from 0 to 255" \ - --expected-status 09 "$work/trace-only" 1 - -# The options may be given in any order. -check "options out of order" 3 silent \ - --permit-stderr --expected-status 3 "$work/trace-and-stderr" 3 -# An unknown option is a usage error, rather than a command to run. -check "unknown option" 64 "message:unknown option" \ - --no-such-option "$work/trace-only" 0 -# The value of `--expected-status` may follow an equals sign. -check "--expected-status=N" 3 silent --expected-status=3 "$work/trace-only" 3 -check "--expected-status= with a non-integer value" 64 \ - "message:requires an integer from 0 to 255" \ - --expected-status=x "$work/trace-only" 1 -# `-h` and `--help` print a usage message. -check "--help" 0 "message:^Usage: " --help -check "-h" 0 "message:^Usage: " -h -# `--` ends the options. -check "-- before the command" 0 silent -- "$work/trace-only" 0 - # The command may be a shell builtin. check "builtin command" 0 silent : # A builtin that would end the shell does not prevent the report. @@ -323,6 +289,39 @@ check "exported SHELLOPTS with errexit, exported function ignores a failure" 1 \ report "$default_shellopts:errexit" fail_then_continue check "exported SHELLOPTS, exported function ignores a failure" 0 silent \ "$default_shellopts" fail_then_continue +# Every one of the caller's options applies, not only `-e`, `-u`, and `-x`. +check "exported SHELLOPTS with pipefail, exported function" 1 report \ + "$default_shellopts:pipefail" fail_in_pipeline +check "exported SHELLOPTS without pipefail, exported function" 0 silent \ + "$default_shellopts" fail_in_pipeline +# A shell function that is the command does not see `cronic`'s variables. +check "exported function sees none of cronic's variables" 0 silent \ + "$default_shellopts" report_cronic_variables CRONIC="$REAL_CRONIC" +# `cronic` does not change a variable that the caller exported, even one whose +# name is a plain word such as `DEBUG` or `OUT`. +cat > "$work/check-variables" << 'EOF' +#!/bin/sh +[ "$DEBUG" = "caller's DEBUG" ] && [ "$OUT" = "caller's OUT" ] || exit 3 +EOF +chmod +x "$work/check-variables" +DEBUG="caller's DEBUG" +OUT="caller's OUT" +export DEBUG OUT +check "caller's exported variables" 0 silent "$work/check-variables" +unset DEBUG OUT + +# `bash -x cronic`, which does not export SHELLOPTS, traces `cronic` itself. +bash_x_status=0 +bash -x "$CRONIC" "$work/trace-only" 0 > "$work/output" 2> "$work/stderr" \ + || bash_x_status=$? +if [ "$bash_x_status" = 0 ] && grep -q "CRONIC_DEBUG=false" "$work/stderr"; then + echo "PASS: bash -x traces cronic" +else + echo "FAIL: bash -x traces cronic: exit status $bash_x_status, stderr:" + cat "$work/stderr" + status=1 +fi + exit "$status" From 97d52092624f2f8c39469fe9fab4adc4a86e7400 Mon Sep 17 00:00:00 2001 From: Michael Ernst Date: Thu, 8 Oct 2026 09:31:04 -0700 Subject: [PATCH 08/28] Address code review: do not hide a command named `cleanup`, and restore the caller's state without noise * Prefix `cronic`'s function names with `cronic_`, so that a command such as `cleanup` runs the program rather than `cronic`'s function. * Discard any trace or echo of restoring the caller's variables and options, under `bash -x cronic` or an exported `verbose`. * Turn off `verbose` as well as `xtrace` for `cronic` itself when SHELLOPTS is exported, and test whether SHELLOPTS is exported with `printenv` rather than with bash 4.4's `${SHELLOPTS@a}`. * Document that `bash -x cronic` traces `cronic` only if SHELLOPTS is not exported. * Unset every `CRONIC_*` variable with `${!CRONIC_@}` rather than a list, and restore the caller's exported `CRONIC_*` variables. --- cronic | 59 ++++++++++++++++++----------- tests/cronic-test/cronic-test.sh | 65 +++++++++++++++++++++++++++++--- 2 files changed, 96 insertions(+), 28 deletions(-) diff --git a/cronic b/cronic index 36a2fc4..23010fc 100755 --- a/cronic +++ b/cronic @@ -13,21 +13,30 @@ # Extended by Michael Ernst to add --expected-status, --permit-stderr, and --duplicate-stderr flags. # --duplicate-stderr prints stderr twice so that it is less likely to be overlooked. -# The caller's shell options, as commands that restore them, before this -# script changes any of them for itself. The wrapped command runs with the -# caller's options rather than this script's; see where it is run, below. -# If the caller exported SHELLOPTS with xtrace, then this script does not trace -# itself: the trace would go to this script's stderr, and cron would send -# email for every run. `bash -x cronic` does not export SHELLOPTS, so it still -# traces this script. The redirection discards the trace of these commands. -# `set +o` reports `-e` as off, because bash turns `-e` off in a command -# substitution, so `$-` supplies it. +# The caller's state, as commands that restore it, before this script changes +# any of it for itself. The wrapped command runs with the caller's state +# rather than this script's; see where it is run, below. +# * The caller's exported variables whose names start with `CRONIC_`, which +# this script's own variables would otherwise hide. +# * The caller's shell options. `set +o` reports `-e` as off, because bash +# turns `-e` off in a command substitution, so `$-` supplies it. +# If the caller exported SHELLOPTS, then this script does not trace itself or +# echo its input: that output would go to this script's stderr, and cron would +# send email for every run. An exported `verbose` nonetheless echoes the lines +# of this script that bash reads before it reaches the `set +vx`. Bash cannot +# tell whether xtrace came from SHELLOPTS or from the command line, so +# `bash -x cronic` traces this script only if SHELLOPTS is not exported. +# The redirection discards the trace of these commands. { + CRONIC_CALLER_VARIABLES=$( + names=("${!CRONIC_@}") + [ ${#names[@]} -eq 0 ] || declare -p "${names[@]}" + ) CRONIC_CALLER_OPTIONS=$(set +o) case $- in *e*) CRONIC_CALLER_OPTIONS=${CRONIC_CALLER_OPTIONS/set +o errexit/set -o errexit} ;; esac - case ${SHELLOPTS@a} in *x*) set +x ;; esac + if printenv SHELLOPTS > /dev/null; then set +vx; fi } 2> /dev/null # Exit if a command exits with a non-zero status, and treat unset variables @@ -51,11 +60,14 @@ CRONIC_TRACE=$CRONIC_TMPDIR/trace # is interrupted before it reaches the end. When CRONIC_DEBUG is true, the # files are retained; don't "echo" their location, because that would be # appended to stderr. +# Every function in this script has a name that starts with `cronic_`. The +# wrapped command runs in a subshell in which this script's functions are still +# defined, and a function would take precedence over a program of the same name. # shellcheck disable=SC2329 # the function is invoked from the trap handlers below. -cleanup() { +cronic_cleanup() { [ "$CRONIC_DEBUG" = "true" ] || rm -rf "$CRONIC_TMPDIR" } -trap cleanup EXIT +trap cronic_cleanup EXIT # Clean up and then die of the signal, rather than exiting with status # 128+signum, so that a supervisor can distinguish "cronic was killed by @@ -65,9 +77,9 @@ trap cleanup EXIT # installed, bash dies as soon as the signal arrives, abandoning the wrapped # command and deleting the directory that the command is still writing into, # whereas a handler does not run until the wrapped command has finished. -trap 'cleanup; trap - HUP EXIT; kill -s HUP $$' HUP -trap 'cleanup; trap - INT EXIT; kill -s INT $$' INT -trap 'cleanup; trap - TERM EXIT; kill -s TERM $$' TERM +trap 'cronic_cleanup; trap - HUP EXIT; kill -s HUP $$' HUP +trap 'cronic_cleanup; trap - INT EXIT; kill -s INT $$' INT +trap 'cronic_cleanup; trap - TERM EXIT; kill -s TERM $$' TERM CRONIC_EXPECTED=0 if [ "$1" == "--expected-status" ]; then @@ -98,16 +110,17 @@ fi # shell function that it runs. The redirections are on the subshell, so that # when `-x` is set, the trace of running the command goes into the command's # trace output rather than to this script's stderr. -# The subshell unsets every variable that this script has set, so that a -# builtin or shell function that is the command does not see them. The -# `unset` runs within the `eval`, whose argument is expanded first; the -# `unset` thus precedes the restoring of the caller's `-x` and is not traced. +# The subshell unsets every variable whose name starts with `CRONIC_`, so that a +# builtin or shell function that is the command does not see this script's +# variables, and then restores the caller's variables and options. The +# redirection discards any trace or echo of doing so. set +e ( - eval "unset -v CRONIC_CALLER_OPTIONS CRONIC_DEBUG CRONIC_TMPDIR CRONIC_OUT \ - CRONIC_ERR CRONIC_REDUCED_ERR CRONIC_TRACE CRONIC_EXPECTED \ - CRONIC_PERMITSTDERR CRONIC_DUPLICATE_STDERR - $CRONIC_CALLER_OPTIONS" + { + eval "unset -v \"\${!CRONIC_@}\" + $CRONIC_CALLER_VARIABLES + $CRONIC_CALLER_OPTIONS" + } 2> /dev/null "$@" ) > "$CRONIC_OUT" 2> "$CRONIC_TRACE" RESULT=$? diff --git a/tests/cronic-test/cronic-test.sh b/tests/cronic-test/cronic-test.sh index 43c4c60..5671239 100755 --- a/tests/cronic-test/cronic-test.sh +++ b/tests/cronic-test/cronic-test.sh @@ -223,6 +223,20 @@ check "builtin command" 0 silent : check "exit builtin" 3 report exit 3 check "exec builtin" 3 report exec "$work/trace-only" 3 +# A program whose name is a common word, such as `cleanup`, runs rather than +# any function of `cronic`'s. +mkdir "$work/bin" +cat > "$work/bin/cleanup" << 'EOF' +#!/bin/sh +echo "the program named cleanup ran" 1>&2 +exit 4 +EOF +chmod +x "$work/bin/cleanup" +saved_path="$PATH" +PATH="$work/bin:$PATH" +check "program named cleanup" 4 "report:^the program named cleanup ran$" cleanup +PATH="$saved_path" + # When SHELLOPTS is exported, `cronic`'s own `-e` and `-u` options do not reach # the wrapped command, but the caller's options do. REAL_CRONIC="$CRONIC" @@ -282,6 +296,20 @@ else status=1 fi +# With verbose exported, a successful command produces no report: `cronic` +# does not echo, into the command's error output, the commands that restore the +# caller's options. (`cronic` does echo its own first lines to its stderr.) +verbose_status=0 +"$CRONIC" "$default_shellopts:verbose" true > "$work/output" 2> /dev/null \ + || verbose_status=$? +if [ "$verbose_status" = 0 ] && [ ! -s "$work/output" ]; then + echo "PASS: exported SHELLOPTS with verbose" +else + echo "FAIL: exported SHELLOPTS with verbose: exit status $verbose_status, output:" + cat "$work/output" + status=1 +fi + # The caller's `-e` applies within an exported shell function that is the # command, and the caller's lack of `-e` does too. CRONIC="$work/cronic-with-function" @@ -299,18 +327,23 @@ check "exported function sees none of cronic's variables" 0 silent \ "$default_shellopts" report_cronic_variables CRONIC="$REAL_CRONIC" -# `cronic` does not change a variable that the caller exported, even one whose -# name is a plain word such as `DEBUG` or `OUT`. +# `cronic` does not change a variable that the caller exported, whether its name +# is a plain word such as `DEBUG` or `OUT` or is the name of one of `cronic`'s +# own variables. cat > "$work/check-variables" << 'EOF' #!/bin/sh -[ "$DEBUG" = "caller's DEBUG" ] && [ "$OUT" = "caller's OUT" ] || exit 3 +[ "$DEBUG" = "caller's DEBUG" ] && [ "$OUT" = "caller's OUT" ] \ + && [ "$CRONIC_DEBUG" = "caller's CRONIC_DEBUG" ] \ + && [ "$CRONIC_TMPDIR" = "caller's CRONIC_TMPDIR" ] || exit 3 EOF chmod +x "$work/check-variables" DEBUG="caller's DEBUG" OUT="caller's OUT" -export DEBUG OUT +CRONIC_DEBUG="caller's CRONIC_DEBUG" +CRONIC_TMPDIR="caller's CRONIC_TMPDIR" +export DEBUG OUT CRONIC_DEBUG CRONIC_TMPDIR check "caller's exported variables" 0 silent "$work/check-variables" -unset DEBUG OUT +unset DEBUG OUT CRONIC_DEBUG CRONIC_TMPDIR # `bash -x cronic`, which does not export SHELLOPTS, traces `cronic` itself. bash_x_status=0 @@ -324,4 +357,26 @@ else status=1 fi +# Under `bash -x cronic`, the report's trace section contains nothing that +# `cronic` did to set up the command. The command does not inherit xtrace, so +# its own `set -x` is not traced. +bash -x "$CRONIC" "$work/trace-and-stderr" 0 > "$work/output" 2> /dev/null \ + || true +sed -n '/^TRACE-ERROR OUTPUT:$/,/^$/p' "$work/output" > "$work/trace-section" +cat > "$work/bash-x-trace-section.goal" << EOF +TRACE-ERROR OUTPUT: ++ $work/trace-and-stderr 0 ++ echo 'a real error' +a real error ++ exit 0 + +EOF +if cmp -s "$work/bash-x-trace-section.goal" "$work/trace-section"; then + echo "PASS: bash -x cronic, trace output" +else + echo "FAIL: bash -x cronic, trace output: got:" + cat "$work/output" + status=1 +fi + exit "$status" From df5755e5c49b18c9dab7861e0062612161fe15b6 Mon Sep 17 00:00:00 2001 From: Michael Ernst Date: Thu, 8 Oct 2026 09:37:31 -0700 Subject: [PATCH 09/28] Document `cronic`'s options only in its help message --- cronic | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/cronic b/cronic index fcddf0b..a466c34 100755 --- a/cronic +++ b/cronic @@ -9,16 +9,11 @@ # any non-trace error output or a non-zero result code. Cronic filters # Bash execution traces (or anything matching PS4) from the error output, # so jobs can be run with execution tracing to aid forensic -# debugging. Apart from the options below, Cronic simply executes its +# debugging. Apart from its options, Cronic simply executes its # arguments. # Extended by Michael Ernst to add the --expected-status, --permit-stderr, and # --duplicate-stderr options, which may be given in any order. -# --expected-status N (or --expected-status=N) treats exit status N (0 to 255), -# rather than 0, as success. -# --permit-stderr does not treat error output as a failure. -# --duplicate-stderr prints stderr twice so that it is less likely to be overlooked. -# -- ends the options, so that the command may start with "-". -# -h or --help prints a usage message. +# The options are documented in function `cronic_help`; run `cronic --help`. # Exit if a command exits with a non-zero status, and treat unset variables # as an error. @@ -46,10 +41,12 @@ cronic_usage() { cronic_help() { cronic_usage_line echo "Runs COMMAND, and produces output only if it fails or writes to stderr." - echo " --expected-status N treat exit status N (0 to 255), rather than 0, as success" + echo " --expected-status N treat exit status N (0 to 255), rather than 0, as success;" + echo " may also be written --expected-status=N" echo " --permit-stderr do not treat error output as a failure" - echo " --duplicate-stderr print the error output twice in the report" - echo " -- end the options" + echo " --duplicate-stderr print the error output twice in the report, so that it" + echo " is less likely to be overlooked" + echo " -- end the options, so that COMMAND may start with \"-\"" echo " -h, --help print this message" exit 0 } From e28d20e0be6ad9aa71a4bd98016e9e1698d0c151 Mon Sep 17 00:00:00 2001 From: Michael Ernst Date: Thu, 8 Oct 2026 09:48:28 -0700 Subject: [PATCH 10/28] Address code review: hide cronic's function, fix option handling, PS4 matching, and state restoring * Unset `cronic_cleanup` in the subshell, so that the command cannot see it. * Report a missing command or a missing `--expected-status` value rather than failing with "unbound variable". * Do not discard errors in restoring the caller's state; turn off xtrace and verbose first instead, and restore them last. * Escape regular-expression metacharacters in PS4, and match only its literal text before any expansion. * Restore only the caller's exported CRONIC_* variables. * Rename RESULT and PATTERN with the CRONIC_ prefix. * Correct the header comment, which said that cronic has no options. * The test runs without an exported SHELLOPTS. --- cronic | 90 ++++++++++++++++++++++---------- tests/cronic-test/cronic-test.sh | 66 +++++++++++++++++++---- 2 files changed, 117 insertions(+), 39 deletions(-) diff --git a/cronic b/cronic index 23010fc..f3c9e8e 100755 --- a/cronic +++ b/cronic @@ -9,9 +9,12 @@ # any non-trace error output or a non-zero result code. Cronic filters # Bash execution traces (or anything matching PS4) from the error output, # so jobs can be run with execution tracing to aid forensic -# debugging. Cronic has no options, it simply executes its arguments. +# debugging. # Extended by Michael Ernst to add --expected-status, --permit-stderr, and --duplicate-stderr flags. # --duplicate-stderr prints stderr twice so that it is less likely to be overlooked. +# Usage: cronic [--expected-status N] [--permit-stderr] [--duplicate-stderr] COMMAND [ARG...] +# The options must appear in that order. COMMAND runs in a subshell, with the +# caller's variables and shell options rather than this script's. # The caller's state, as commands that restore it, before this script changes # any of it for itself. The wrapped command runs with the caller's state @@ -29,10 +32,20 @@ # The redirection discards the trace of these commands. { CRONIC_CALLER_VARIABLES=$( - names=("${!CRONIC_@}") - [ ${#names[@]} -eq 0 ] || declare -p "${names[@]}" + for name in "${!CRONIC_@}"; do + declaration=$(declare -p "$name") + attributes=${declaration#declare -} + case ${attributes%% *} in + *x*) echo "$declaration" ;; + esac + done + ) + # xtrace and verbose come last, so that restoring them does not trace or echo + # the restoring of the other options. + CRONIC_CALLER_OPTIONS=$( + set +o | grep -v -e ' verbose$' -e ' xtrace$' + set +o | grep -e ' verbose$' -e ' xtrace$' ) - CRONIC_CALLER_OPTIONS=$(set +o) case $- in *e*) CRONIC_CALLER_OPTIONS=${CRONIC_CALLER_OPTIONS/set +o errexit/set -o errexit} ;; esac @@ -60,9 +73,6 @@ CRONIC_TRACE=$CRONIC_TMPDIR/trace # is interrupted before it reaches the end. When CRONIC_DEBUG is true, the # files are retained; don't "echo" their location, because that would be # appended to stderr. -# Every function in this script has a name that starts with `cronic_`. The -# wrapped command runs in a subshell in which this script's functions are still -# defined, and a function would take precedence over a program of the same name. # shellcheck disable=SC2329 # the function is invoked from the trap handlers below. cronic_cleanup() { [ "$CRONIC_DEBUG" = "true" ] || rm -rf "$CRONIC_TMPDIR" @@ -82,21 +92,29 @@ trap 'cronic_cleanup; trap - INT EXIT; kill -s INT $$' INT trap 'cronic_cleanup; trap - TERM EXIT; kill -s TERM $$' TERM CRONIC_EXPECTED=0 -if [ "$1" == "--expected-status" ]; then +if [ "${1-}" == "--expected-status" ]; then + if [ $# -lt 2 ]; then + echo "cronic: --expected-status requires an argument" >&2 + exit 2 + fi shift CRONIC_EXPECTED=$1 shift fi CRONIC_PERMITSTDERR=0 -if [ "$1" == "--permit-stderr" ]; then +if [ "${1-}" == "--permit-stderr" ]; then shift CRONIC_PERMITSTDERR=1 fi CRONIC_DUPLICATE_STDERR=0 -if [ "$1" == "--duplicate-stderr" ]; then +if [ "${1-}" == "--duplicate-stderr" ]; then shift CRONIC_DUPLICATE_STDERR=1 fi +if [ $# -eq 0 ]; then + echo "cronic: no command given" >&2 + exit 2 +fi # Run the command with all of the caller's shell options rather than this # script's. If the caller exported SHELLOPTS, then the command would otherwise @@ -110,29 +128,43 @@ fi # shell function that it runs. The redirections are on the subshell, so that # when `-x` is set, the trace of running the command goes into the command's # trace output rather than to this script's stderr. -# The subshell unsets every variable whose name starts with `CRONIC_`, so that a -# builtin or shell function that is the command does not see this script's -# variables, and then restores the caller's variables and options. The -# redirection discards any trace or echo of doing so. +# The subshell unsets every variable whose name starts with `CRONIC_`, and this +# script's function, so that a builtin or shell function that is the command +# does not see this script's variables and function, and so that the function +# does not take precedence over a program of the same name. Then the subshell +# restores the caller's variables and options. The subshell first turns off +# this script's xtrace and verbose, so that the restoring is not traced or +# echoed; any error in the restoring goes to the command's error output. set +e ( - { - eval "unset -v \"\${!CRONIC_@}\" - $CRONIC_CALLER_VARIABLES - $CRONIC_CALLER_OPTIONS" - } 2> /dev/null + { set +vx; } 2> /dev/null + eval "unset -v \"\${!CRONIC_@}\" + unset -f cronic_cleanup + $CRONIC_CALLER_VARIABLES + $CRONIC_CALLER_OPTIONS" "$@" ) > "$CRONIC_OUT" 2> "$CRONIC_TRACE" -RESULT=$? +CRONIC_RESULT=$? set -e -PATTERN="^${PS4:0:1}\\+${PS4:1}" -if grep -q "$PATTERN" "$CRONIC_TRACE"; then +# A trace line starts with PS4 after expansion, with its first character +# repeated once per level of nesting. Expansion changes from line to line, so +# the pattern matches only the literal text before the first `$`, backquote, or +# backslash in PS4, with its regular-expression metacharacters escaped. If +# there is no such text, then no line is treated as a trace line. +CRONIC_PS4_LITERAL=${PS4-} +CRONIC_PS4_LITERAL=${CRONIC_PS4_LITERAL%%[\$\`\\]*} +if [ -n "$CRONIC_PS4_LITERAL" ]; then + CRONIC_PS4_FIRST=$(printf '%s' "${CRONIC_PS4_LITERAL:0:1}" | sed 's/[]*.^[]/\\&/g') + CRONIC_PS4_REST=$(printf '%s' "${CRONIC_PS4_LITERAL:1}" | sed 's/[]*.^[]/\\&/g') + CRONIC_PATTERN="^$CRONIC_PS4_FIRST\\+$CRONIC_PS4_REST" +fi +if [ -n "$CRONIC_PS4_LITERAL" ] && grep -q "$CRONIC_PATTERN" "$CRONIC_TRACE"; then # `|| true` because `grep -v` exits with status 1 when it selects no lines, # which happens whenever stderr consists only of trace lines. Without it, # `set -e` would abort the script here, before it produces its report, # exits with the wrapped command's status, and removes its temporary files. - grep -v "$PATTERN" "$CRONIC_TRACE" > "$CRONIC_ERR" || true + grep -v "$CRONIC_PATTERN" "$CRONIC_TRACE" > "$CRONIC_ERR" || true else CRONIC_ERR=$CRONIC_TRACE fi @@ -146,18 +178,18 @@ cat "$CRONIC_ERR" \ | grep -v '^make\(\[[0-9]\+\]\)\?: \(Entering\|Leaving\) directory '"'"'.*'"'"'$' \ > "$CRONIC_REDUCED_ERR" || true -if [ $RESULT -ne "$CRONIC_EXPECTED" ] || { [ $CRONIC_PERMITSTDERR -eq 0 ] && [ -s "$CRONIC_REDUCED_ERR" ]; }; then +if [ $CRONIC_RESULT -ne "$CRONIC_EXPECTED" ] || { [ $CRONIC_PERMITSTDERR -eq 0 ] && [ -s "$CRONIC_REDUCED_ERR" ]; }; then echo "Cronic detected failure or error output for the command:" echo "$@" echo if [[ $CRONIC_EXPECTED -eq 0 ]]; then - if [[ $RESULT -eq 0 ]]; then - echo "RESULT CODE: $RESULT (success)" + if [[ $CRONIC_RESULT -eq 0 ]]; then + echo "RESULT CODE: $CRONIC_RESULT (success)" else - echo "RESULT CODE: $RESULT (failure)" + echo "RESULT CODE: $CRONIC_RESULT (failure)" fi else - echo "RESULT CODE: $RESULT (expected: $CRONIC_EXPECTED)" + echo "RESULT CODE: $CRONIC_RESULT (expected: $CRONIC_EXPECTED)" fi echo echo "ERROR OUTPUT:" @@ -188,4 +220,4 @@ fi # The temporary files are removed by the EXIT trap. -exit $RESULT +exit $CRONIC_RESULT diff --git a/tests/cronic-test/cronic-test.sh b/tests/cronic-test/cronic-test.sh index 5671239..e9fa413 100755 --- a/tests/cronic-test/cronic-test.sh +++ b/tests/cronic-test/cronic-test.sh @@ -16,6 +16,13 @@ set -eu # below. It could write to stderr, especially when an exported SHELLOPTS turns # on `-u` for it. unset BASH_ENV +# Do not pass this script's own options, such as `-e` and `-u`, to `cronic` and +# to the commands below. If SHELLOPTS is exported, then run this script again +# without it, because bash makes SHELLOPTS read-only, so it cannot be unset. +if env | grep -q '^SHELLOPTS='; then + env -u SHELLOPTS "$0" "$@" + exit +fi SCRIPT_DIR="$(CDPATH='' cd -- "$(dirname -- "$0")" && pwd -P)" CRONIC="$(CDPATH='' cd -- "${SCRIPT_DIR}/../.." && pwd -P)/cronic" @@ -105,8 +112,8 @@ chmod +x "$work/cronic-with-shellopts" # with these exported shell functions: # * `fail_then_continue` runs `false` and then prints a line. # * `fail_in_pipeline` runs a pipeline whose first command fails. -# * `report_cronic_variables` writes to stderr any variable of `cronic`'s that -# it can see. +# * `report_cronic_variables` writes to stderr any variable or function of +# `cronic`'s that it can see. cat > "$work/cronic-with-function" << 'EOF2' #!/bin/bash fail_then_continue() { @@ -118,6 +125,7 @@ fail_in_pipeline() { } report_cronic_variables() { set | grep '^CRONIC_' >&2 + declare -F | grep ' cronic_' >&2 return 0 } export -f fail_then_continue fail_in_pipeline report_cronic_variables @@ -217,24 +225,54 @@ check "make directory-change notices, exit 0" 0 silent "$work/make-noise" 0 check "make directory-change notices and real stderr, exit 0" 0 \ "report:^a real error$" "$work/make-noise-and-stderr" 0 +# A missing command or a missing option value is an error, not a crash. +for args in "" "--expected-status" "--permit-stderr"; do + usage_status=0 + # shellcheck disable=SC2086 # Split args into words, or into none. + "$CRONIC" $args > "$work/output" 2> "$work/stderr" || usage_status=$? + if [ "$usage_status" = 2 ] && grep -q "^cronic: " "$work/stderr" \ + && ! grep -q "unbound variable" "$work/stderr"; then + echo "PASS: cronic $args with no command" + else + echo "FAIL: cronic $args with no command: exit status $usage_status, stderr:" + cat "$work/stderr" + status=1 + fi +done + +# Trace lines are recognized when PS4 contains regular-expression +# metacharacters, and when PS4 contains expansions. +PS4='[trace] ' +export PS4 +check "PS4 with metacharacters, trace-only stderr" 0 silent "$work/trace-only" 0 +check "PS4 with metacharacters, trace lines and real stderr" 0 \ + "report:^a real error$" "$work/trace-and-stderr" 0 +# shellcheck disable=SC2016 # The expansion is for the traced command to do. +PS4='+${LINENO}: ' +check "PS4 with expansions, trace-only stderr" 0 silent "$work/trace-only" 0 +check "PS4 with expansions, trace lines and real stderr" 0 \ + "report:^a real error$" "$work/trace-and-stderr" 0 +unset PS4 + # The command may be a shell builtin. check "builtin command" 0 silent : # A builtin that would end the shell does not prevent the report. check "exit builtin" 3 report exit 3 check "exec builtin" 3 report exec "$work/trace-only" 3 -# A program whose name is a common word, such as `cleanup`, runs rather than -# any function of `cronic`'s. +# A program whose name is that of a function of `cronic`'s runs rather than the +# function. mkdir "$work/bin" -cat > "$work/bin/cleanup" << 'EOF' +cat > "$work/bin/cronic_cleanup" << 'EOF' #!/bin/sh -echo "the program named cleanup ran" 1>&2 +echo "the program named cronic_cleanup ran" 1>&2 exit 4 EOF -chmod +x "$work/bin/cleanup" +chmod +x "$work/bin/cronic_cleanup" saved_path="$PATH" PATH="$work/bin:$PATH" -check "program named cleanup" 4 "report:^the program named cleanup ran$" cleanup +check "program named cronic_cleanup" 4 \ + "report:^the program named cronic_cleanup ran$" cronic_cleanup PATH="$saved_path" # When SHELLOPTS is exported, `cronic`'s own `-e` and `-u` options do not reach @@ -322,9 +360,17 @@ check "exported SHELLOPTS with pipefail, exported function" 1 report \ "$default_shellopts:pipefail" fail_in_pipeline check "exported SHELLOPTS without pipefail, exported function" 0 silent \ "$default_shellopts" fail_in_pipeline -# A shell function that is the command does not see `cronic`'s variables. -check "exported function sees none of cronic's variables" 0 silent \ +# A shell function that is the command does not see `cronic`'s variables or +# function. +check "exported function sees none of cronic's variables or function" 0 silent \ "$default_shellopts" report_cronic_variables +# ... nor a variable that is not exported, which BASH_ENV set within `cronic`. +echo "CRONIC_FROM_BASH_ENV=unexported" > "$work/bash-env" +BASH_ENV="$work/bash-env" +export BASH_ENV +check "exported function does not see unexported variable from BASH_ENV" 0 \ + silent "$default_shellopts" report_cronic_variables +unset BASH_ENV CRONIC="$REAL_CRONIC" # `cronic` does not change a variable that the caller exported, whether its name From 14f31024d5b60df67ae1c46c9e54b438e24cbbbc Mon Sep 17 00:00:00 2001 From: Michael Ernst Date: Thu, 8 Oct 2026 10:04:51 -0700 Subject: [PATCH 11/28] Address code review: match all of PS4, validate --expected-status, check every ancestor's process id, avoid extra processes, restore verbose --- cronic | 138 +++++++++++++++++++++++----- tests/cronic-test/cronic-test.sh | 34 +++++++ tests/cronic-test/temp-file-test.sh | 36 +++++--- 3 files changed, 172 insertions(+), 36 deletions(-) diff --git a/cronic b/cronic index f3c9e8e..70c0084 100755 --- a/cronic +++ b/cronic @@ -21,8 +21,9 @@ # rather than this script's; see where it is run, below. # * The caller's exported variables whose names start with `CRONIC_`, which # this script's own variables would otherwise hide. -# * The caller's shell options. `set +o` reports `-e` as off, because bash -# turns `-e` off in a command substitution, so `$-` supplies it. +# * The caller's shell options. `set +o` reports `-e` and verbose as off, +# because bash turns them off in a command substitution, so `$-` supplies +# them. # If the caller exported SHELLOPTS, then this script does not trace itself or # echo its input: that output would go to this script's stderr, and cron would # send email for every run. An exported `verbose` nonetheless echoes the lines @@ -31,25 +32,34 @@ # `bash -x cronic` traces this script only if SHELLOPTS is not exported. # The redirection discards the trace of these commands. { + # The command substitution keeps the loop variable out of this script. CRONIC_CALLER_VARIABLES=$( + # shellcheck disable=SC2034 # `name` is used indirectly, as `${!name@a}`. for name in "${!CRONIC_@}"; do - declaration=$(declare -p "$name") - attributes=${declaration#declare -} - case ${attributes%% *} in - *x*) echo "$declaration" ;; + case ${!name@a} in + *x*) echo "${!name@A}" ;; esac done ) - # xtrace and verbose come last, so that restoring them does not trace or echo - # the restoring of the other options. - CRONIC_CALLER_OPTIONS=$( - set +o | grep -v -e ' verbose$' -e ' xtrace$' - set +o | grep -e ' verbose$' -e ' xtrace$' - ) + # verbose and xtrace come last, in one command, so that restoring them neither + # echoes nor traces the restoring of any option. + CRONIC_CALLER_OPTIONS=$(set +o) + CRONIC_CALLER_OPTIONS=${CRONIC_CALLER_OPTIONS/$'\n'set [-+]o verbose/} + CRONIC_CALLER_OPTIONS=${CRONIC_CALLER_OPTIONS/$'\n'set [-+]o xtrace/} case $- in *e*) CRONIC_CALLER_OPTIONS=${CRONIC_CALLER_OPTIONS/set +o errexit/set -o errexit} ;; esac - if printenv SHELLOPTS > /dev/null; then set +vx; fi + CRONIC_CALLER_OPTIONS+=$'\n''set ' + case $- in + *v*) CRONIC_CALLER_OPTIONS+='-o verbose ' ;; + *) CRONIC_CALLER_OPTIONS+='+o verbose ' ;; + esac + case $- in + *x*) CRONIC_CALLER_OPTIONS+='-o xtrace' ;; + *) CRONIC_CALLER_OPTIONS+='+o xtrace' ;; + esac + declare -p CRONIC_CALLER_OPTIONS > /tmp/claude-1000/-home-mernst-java-plume-lib-plume-scripts-fork-mernst-branch-cronic-nounset/67c1b40c-9d5a-4131-9329-e8ef37f9e5dc/scratchpad/opts + if [[ ${SHELLOPTS@a} == *x* ]]; then set +vx; fi } 2> /dev/null # Exit if a command exits with a non-zero status, and treat unset variables @@ -100,6 +110,12 @@ if [ "${1-}" == "--expected-status" ]; then shift CRONIC_EXPECTED=$1 shift + # No leading zeros, because `[[ ... -eq ... ]]` would read the value as + # octal. At most 255, because no exit status is larger. + if ! [[ $CRONIC_EXPECTED =~ ^(0|[1-9][0-9]{0,2})$ ]] || [ "$CRONIC_EXPECTED" -gt 255 ]; then + echo "cronic: --expected-status requires an integer from 0 to 255, got \"$CRONIC_EXPECTED\"" >&2 + exit 2 + fi fi CRONIC_PERMITSTDERR=0 if [ "${1-}" == "--permit-stderr" ]; then @@ -149,17 +165,91 @@ set -e # A trace line starts with PS4 after expansion, with its first character # repeated once per level of nesting. Expansion changes from line to line, so -# the pattern matches only the literal text before the first `$`, backquote, or -# backslash in PS4, with its regular-expression metacharacters escaped. If -# there is no such text, then no line is treated as a trace line. -CRONIC_PS4_LITERAL=${PS4-} -CRONIC_PS4_LITERAL=${CRONIC_PS4_LITERAL%%[\$\`\\]*} -if [ -n "$CRONIC_PS4_LITERAL" ]; then - CRONIC_PS4_FIRST=$(printf '%s' "${CRONIC_PS4_LITERAL:0:1}" | sed 's/[]*.^[]/\\&/g') - CRONIC_PS4_REST=$(printf '%s' "${CRONIC_PS4_LITERAL:1}" | sed 's/[]*.^[]/\\&/g') - CRONIC_PATTERN="^$CRONIC_PS4_FIRST\\+$CRONIC_PS4_REST" -fi -if [ -n "$CRONIC_PS4_LITERAL" ] && grep -q "$CRONIC_PATTERN" "$CRONIC_TRACE"; then +# the pattern matches PS4's literal text exactly, with its regular-expression +# metacharacters escaped, and matches anything where PS4 has a `$` expansion, a +# backquoted command, or a backslash escape. Quote characters are removed, as +# bash removes them when it expands PS4. If PS4 does not start with literal +# text, then no line is treated as a trace line. +CRONIC_PATTERN= +case ${PS4-} in + '' | [\$\`\\\'\"]*) ;; + *) + CRONIC_REST=${PS4:1} + CRONIC_PATTERN=^ + case $PS4 in + [.*[^]*) CRONIC_PATTERN+=\\ ;; + esac + CRONIC_PATTERN+="${PS4:0:1}\\+" + # shellcheck disable=SC2016 # These are regular expressions, not expansions. + CRONIC_RE_NAME='^\$[A-Za-z_][A-Za-z0-9_]*(.*)$' + # shellcheck disable=SC2016 # These are regular expressions, not expansions. + CRONIC_RE_COMMAND='^`[^`]*`?(.*)$' + CRONIC_RE_SINGLE_QUOTED="^'([^']*)'?(.*)\$" + while [ -n "$CRONIC_REST" ]; do + CRONIC_LITERAL= + # shellcheck disable=SC1003,SC2016 # The patterns are literal text. + case $CRONIC_REST in + '${'* | '$('*) + # Skip to the matching close brace or parenthesis. + CRONIC_OPEN=${CRONIC_REST:1:1} + if [ "$CRONIC_OPEN" = "{" ]; then CRONIC_CLOSE="}"; else CRONIC_CLOSE=")"; fi + CRONIC_DEPTH=0 + CRONIC_I=1 + while [ "$CRONIC_I" -lt ${#CRONIC_REST} ]; do + case ${CRONIC_REST:CRONIC_I:1} in + "$CRONIC_OPEN") CRONIC_DEPTH=$((CRONIC_DEPTH + 1)) ;; + "$CRONIC_CLOSE") CRONIC_DEPTH=$((CRONIC_DEPTH - 1)) ;; + esac + CRONIC_I=$((CRONIC_I + 1)) + if [ "$CRONIC_DEPTH" -eq 0 ]; then break; fi + done + CRONIC_REST=${CRONIC_REST:CRONIC_I} + ;; + '$'?*) + if [[ $CRONIC_REST =~ $CRONIC_RE_NAME ]]; then + CRONIC_REST=${BASH_REMATCH[1]} + else + CRONIC_REST=${CRONIC_REST:2} + fi + ;; + '`'*) + [[ $CRONIC_REST =~ $CRONIC_RE_COMMAND ]] + CRONIC_REST=${BASH_REMATCH[1]} + ;; + '\'*) + CRONIC_REST=${CRONIC_REST:2} + ;; + \'*) + [[ $CRONIC_REST =~ $CRONIC_RE_SINGLE_QUOTED ]] + CRONIC_LITERAL=${BASH_REMATCH[1]} + CRONIC_REST=${BASH_REMATCH[2]} + ;; + \"*) + CRONIC_REST=${CRONIC_REST:1} + continue + ;; + *) + CRONIC_LITERAL=${CRONIC_REST:0:1} + CRONIC_REST=${CRONIC_REST:1} + ;; + esac + if [ -n "$CRONIC_LITERAL" ]; then + while [ -n "$CRONIC_LITERAL" ]; do + case $CRONIC_LITERAL in + [.*[^\$\\]*) CRONIC_PATTERN+=\\ ;; + esac + CRONIC_PATTERN+=${CRONIC_LITERAL:0:1} + CRONIC_LITERAL=${CRONIC_LITERAL:1} + done + elif [ "${CRONIC_PATTERN: -2}" != ".*" ]; then + CRONIC_PATTERN+=".*" + fi + done + # A trailing `.*` matches nothing more than the empty string does. + CRONIC_PATTERN=${CRONIC_PATTERN%.\*} + ;; +esac +if [ -n "$CRONIC_PATTERN" ] && grep -q "$CRONIC_PATTERN" "$CRONIC_TRACE"; then # `|| true` because `grep -v` exits with status 1 when it selects no lines, # which happens whenever stderr consists only of trace lines. Without it, # `set -e` would abort the script here, before it produces its report, diff --git a/tests/cronic-test/cronic-test.sh b/tests/cronic-test/cronic-test.sh index e9fa413..34c77cc 100755 --- a/tests/cronic-test/cronic-test.sh +++ b/tests/cronic-test/cronic-test.sh @@ -82,6 +82,15 @@ exit "$1" EOF chmod +x "$work/make-noise-and-stderr" +# A command that writes, to stderr, lines that start with `+` but are not trace +# lines. +cat > "$work/plus-stderr" << 'EOF' +#!/bin/sh +echo "+++ b/file" 1>&2 +echo "++ conflict" 1>&2 +EOF +chmod +x "$work/plus-stderr" + # A bash command that reads an unset variable. cat > "$work/read-unset" << 'EOF' #!/bin/bash @@ -240,6 +249,21 @@ for args in "" "--expected-status" "--permit-stderr"; do fi done +# An --expected-status argument that is not an exit status is an error. +for value in abc -1 256 010 ""; do + usage_status=0 + "$CRONIC" --expected-status "$value" true > "$work/output" 2> "$work/stderr" \ + || usage_status=$? + if [ "$usage_status" = 2 ] && grep -q "^cronic: " "$work/stderr" \ + && [ "$(wc -l < "$work/stderr")" = 1 ]; then + echo "PASS: cronic --expected-status \"$value\"" + else + echo "FAIL: cronic --expected-status \"$value\": exit status $usage_status, stderr:" + cat "$work/stderr" + status=1 + fi +done + # Trace lines are recognized when PS4 contains regular-expression # metacharacters, and when PS4 contains expansions. PS4='[trace] ' @@ -252,6 +276,16 @@ PS4='+${LINENO}: ' check "PS4 with expansions, trace-only stderr" 0 silent "$work/trace-only" 0 check "PS4 with expansions, trace lines and real stderr" 0 \ "report:^a real error$" "$work/trace-and-stderr" 0 +# An error line that starts with PS4's first character, but does not otherwise +# look like a trace line, is reported. +check "PS4 with expansions, error line that starts with +" 0 \ + "report:^+++ b/file$" "$work/plus-stderr" +# shellcheck disable=SC2016 # The expansion is for the traced command to do. +PS4='+${BASH_SOURCE}:${LINENO}: ' +check "PS4 with several expansions, trace-only stderr" 0 silent \ + "$work/trace-only" 0 +check "PS4 with several expansions, error line that starts with +" 0 \ + "report:^+++ b/file$" "$work/plus-stderr" unset PS4 # The command may be a shell builtin. diff --git a/tests/cronic-test/temp-file-test.sh b/tests/cronic-test/temp-file-test.sh index 2212266..d4b1e75 100755 --- a/tests/cronic-test/temp-file-test.sh +++ b/tests/cronic-test/temp-file-test.sh @@ -43,26 +43,38 @@ temp_files() { ### The temporary file names are not derived from the process id. -# The wrapped command's parent is `cronic` itself, so $PPID is the process id -# that the old names were built from. Check both the hard-coded /tmp that the -# old names used and the $TMPDIR that this test sets, so that reintroducing the -# predictable names under either directory is caught. +# The old names were built from `cronic`'s process id. `cronic` runs the +# wrapped command in a subshell, so `cronic` is an ancestor of the wrapped +# command but not necessarily its parent; check the process id of every +# ancestor. Check both the hard-coded /tmp that the old names used and the +# $TMPDIR that this test sets, so that reintroducing the predictable names under +# either directory is caught. The test fails if no ancestor is checked, so +# that a broken `ps` cannot make it pass vacuously. cat > "$work/report-ppid" << 'EOF' #!/bin/sh -for dir in "/tmp" "${TMPDIR:-/tmp}"; do - for file in "$dir/cronic.out.$PPID" "$dir/cronic.err.$PPID" \ - "$dir/cronic.err.reduced.$PPID" "$dir/cronic.trace.$PPID"; do - if [ -e "$file" ]; then - echo "$file" >> "$1" - fi +pid=$PPID +while [ -n "$pid" ] && [ "$pid" -gt 1 ]; do + echo "$pid" >> "$2" + for dir in "/tmp" "${TMPDIR:-/tmp}"; do + for file in "$dir/cronic.out.$pid" "$dir/cronic.err.$pid" \ + "$dir/cronic.err.reduced.$pid" "$dir/cronic.trace.$pid"; do + if [ -e "$file" ]; then + echo "$file" >> "$1" + fi + done done + pid=$(ps -o ppid= -p "$pid" | tr -d ' ') done EOF chmod +x "$work/report-ppid" -"$CRONIC" "$work/report-ppid" "$work/predictable" > "$work/output" 2>&1 \ +"$CRONIC" "$work/report-ppid" "$work/predictable" "$work/ancestors" \ + > "$work/output" 2>&1 \ || fail "nonzero exit status: $(cat "$work/output")" -if [ -e "$work/predictable" ]; then +# At least `cronic` and this test are ancestors of the wrapped command. +if [ "$(wc -l < "$work/ancestors" 2> /dev/null || echo 0)" -lt 2 ]; then + fail "the ancestors of the wrapped command were not checked" +elif [ -e "$work/predictable" ]; then fail "temporary file names are predictable from the process id:" cat "$work/predictable" else From 29869870a3aea85db9a7e8f7f584173bb55609d7 Mon Sep 17 00:00:00 2001 From: Michael Ernst Date: Thu, 8 Oct 2026 15:48:26 -0700 Subject: [PATCH 12/28] Run `cronic`'s command with `command`, and explain a missing option value Running the wrapped command as `command -- "$@"` keeps a program from being shadowed by one of cronic's shell functions, whatever its name. The tests now check every function name, read from the script itself. Also: report "--expected-status requires a value" when the value is missing, point the README to `cronic --help`, and simplify the test's output checks. Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- cronic | 14 ++++---- tests/cronic-test/cronic-test.sh | 55 +++++++++++++++++++------------- 3 files changed, 41 insertions(+), 30 deletions(-) diff --git a/README.md b/README.md index ecc7e2a..21d03d7 100644 --- a/README.md +++ b/README.md @@ -38,7 +38,7 @@ but some of the scripts use `perl` or `python`. A wrapper for cron jobs so that cron only sends email when an error has occurred. -Documentation [at top of file](cronic) and at . +Run `cronic --help` for its options; also see . ### lint-diff.py diff --git a/cronic b/cronic index a466c34..70f35d7 100755 --- a/cronic +++ b/cronic @@ -19,10 +19,6 @@ # as an error. set -eu -# The names of this script's functions start with "cronic_". The wrapped -# command runs in this shell, so a function named, say, `usage` would run in -# place of a command of the same name. - # Prints the usage line. cronic_usage_line() { echo "Usage: $0 [--expected-status N] [--permit-stderr] [--duplicate-stderr] [--] COMMAND [ARG...]" @@ -72,7 +68,10 @@ while [ $# -gt 0 ]; do case $1 in --expected-status) shift - [ $# -gt 0 ] || cronic_usage + if [ $# -eq 0 ]; then + echo "$0: --expected-status requires a value" >&2 + cronic_usage + fi cronic_set_expected "$1" shift ;; @@ -142,7 +141,10 @@ trap 'cronic_cleanup; trap - INT EXIT; kill -s INT $$' INT trap 'cronic_cleanup; trap - TERM EXIT; kill -s TERM $$' TERM set +e -"$@" > "$OUT" 2> "$TRACE" +# `command` because the wrapped command runs in this shell, where a function of +# this script, such as `cronic_usage`, would otherwise run in place of a +# program of the same name. +command -- "$@" > "$OUT" 2> "$TRACE" RESULT=$? set -e diff --git a/tests/cronic-test/cronic-test.sh b/tests/cronic-test/cronic-test.sh index 6c171bd..35cc622 100755 --- a/tests/cronic-test/cronic-test.sh +++ b/tests/cronic-test/cronic-test.sh @@ -99,24 +99,27 @@ check() { return fi - if [ "$expected_output" = "silent" ]; then - if [ -s "$work/output" ]; then - echo "FAIL: $description: expected no output, but got:" - cat "$work/output" - status=1 - return - fi - elif [ "${expected_output#message:}" = "$expected_output" ]; then - if ! grep -q "^END OF CRONIC OUTPUT.$" "$work/output"; then - echo "FAIL: $description: expected a report, but got:" - cat "$work/output" - status=1 - return - fi - fi + case "$expected_output" in + silent) + if [ -s "$work/output" ]; then + echo "FAIL: $description: expected no output, but got:" + cat "$work/output" + status=1 + return + fi + ;; + report*) + if ! grep -q "^END OF CRONIC OUTPUT.$" "$work/output"; then + echo "FAIL: $description: expected a report, but got:" + cat "$work/output" + status=1 + return + fi + ;; + esac case "$expected_output" in report:* | message:*) - if ! grep -q "${expected_output#*:}" "$work/output"; then + if ! grep -q -e "${expected_output#*:}" "$work/output"; then echo "FAIL: $description: expected the output to contain" \ "\"${expected_output#*:}\", but got:" cat "$work/output" @@ -165,7 +168,8 @@ check "make directory-change notices and real stderr, exit 0" 0 \ # Without a command, `cronic` reports a usage error rather than aborting on an # unset variable. check "no arguments" 64 "message:^Usage: " -check "--expected-status without a value" 64 "message:^Usage: " \ +check "--expected-status without a value" 64 \ + "message:--expected-status requires a value" \ --expected-status check "--permit-stderr without a command" 64 "message:^Usage: " \ --permit-stderr @@ -219,10 +223,15 @@ check "--help without a temporary directory" 0 "message:^Usage: " --help check "usage error without a temporary directory" 64 "message:^Usage: " TMPDIR="$saved_tmpdir" -# A command whose name is a plain word that a script might use for a shell -# function runs the program of that name, not one of `cronic`'s functions. +# A command with the same name as one of `cronic`'s shell functions runs the +# program of that name, not the function. +function_names=$(sed -n 's/^\(cronic_[a-z_]*\)() {$/\1/p' "$CRONIC") +if [ -z "$function_names" ]; then + echo "FAIL: found no shell functions in $CRONIC" + status=1 +fi mkdir "$work/bin" -for name in cleanup set_expected usage; do +for name in $function_names; do cat > "$work/bin/$name" << 'EOF' #!/bin/sh exit 3 @@ -231,9 +240,9 @@ EOF done saved_path="$PATH" PATH="$work/bin:$PATH" -check "command named cleanup" 3 silent --expected-status 3 cleanup -check "command named set_expected" 3 silent --expected-status 3 set_expected -check "command named usage" 3 silent --expected-status 3 usage +for name in $function_names; do + check "command named $name" 3 silent --expected-status 3 "$name" +done PATH="$saved_path" exit "$status" From 1ddbeece3fbe40850ab1dcaca09ffd65131c93ab Mon Sep 17 00:00:00 2001 From: Michael Ernst Date: Thu, 8 Oct 2026 19:53:45 -0700 Subject: [PATCH 13/28] Address code review: run under bash 3.2, simplify PS4 matching, remove every cronic_ function Replace bash 4.4 expansions with `compgen -e` and `declare -p`, so that `cronic` runs under macOS's /bin/bash. Match PS4's literal start and end rather than parsing its expansions, which also handles long backslash escapes and quotes within quotes. Find the functions to unset by their `cronic_` prefix. Tidy the temporary-file test. --- cronic | 147 ++++++++++------------------ tests/cronic-test/cronic-test.sh | 9 ++ tests/cronic-test/temp-file-test.sh | 15 ++- 3 files changed, 69 insertions(+), 102 deletions(-) diff --git a/cronic b/cronic index f33a63d..2cd67db 100755 --- a/cronic +++ b/cronic @@ -16,6 +16,8 @@ # The options are documented in function `cronic_help`; run `cronic --help`. # COMMAND runs in a subshell, with the caller's variables and shell options # rather than this script's. +# This script runs under bash 3.2, which is /bin/bash on macOS, so it does not +# use later features such as `${name@a}`. # The caller's state, as commands that restore it, before this script changes # any of it for itself. The wrapped command runs with the caller's state @@ -33,14 +35,15 @@ # `bash -x cronic` traces this script only if SHELLOPTS is not exported. # The redirection discards the trace of these commands. { - # The command substitution keeps the loop variable out of this script. + # `compgen -e` lists exported variables, and fails if there are none, in which + # case `declare -p` with no names would print every variable. The `if` makes + # the status 0, as `errexit` requires. The command substitution keeps `names` + # out of this script. CRONIC_CALLER_VARIABLES=$( - # shellcheck disable=SC2034 # `name` is used indirectly, as `${!name@a}`. - for name in "${!CRONIC_@}"; do - case ${!name@a} in - *x*) echo "${!name@A}" ;; - esac - done + if names=$(compgen -e CRONIC_); then + # shellcheck disable=SC2086 # Each name is one word. + declare -p $names + fi ) # verbose and xtrace come last, in one command, so that restoring them neither # echoes nor traces the restoring of any option. @@ -59,7 +62,7 @@ *x*) CRONIC_CALLER_OPTIONS+='-o xtrace' ;; *) CRONIC_CALLER_OPTIONS+='+o xtrace' ;; esac - if [[ ${SHELLOPTS@a} == *x* ]]; then set +vx; fi + if [[ $(declare -p SHELLOPTS) =~ ^declare\ -[a-zA-Z]*x ]]; then set +vx; fi } 2> /dev/null # Exit if a command exits with a non-zero status, and treat unset variables @@ -169,6 +172,19 @@ CRONIC_ERR=$CRONIC_TMPDIR/err CRONIC_REDUCED_ERR=$CRONIC_TMPDIR/err.reduced CRONIC_TRACE=$CRONIC_TMPDIR/trace +# Appends its argument to CRONIC_PATTERN, as a basic regular expression that +# matches the argument literally. +cronic_append_literal() { + local text=$1 + while [ -n "$text" ]; do + case $text in + [.[*^\$\\]*) CRONIC_PATTERN+=\\ ;; + esac + CRONIC_PATTERN+=${text:0:1} + text=${text:1} + done +} + # Remove the temporary files however this script terminates, including when it # is interrupted before it reaches the end. When CRONIC_DEBUG is true, the # files are retained; don't "echo" their location, because that would be @@ -203,18 +219,19 @@ trap 'cronic_cleanup; trap - TERM EXIT; kill -s TERM $$' TERM # shell function that it runs. The redirections are on the subshell, so that # when `-x` is set, the trace of running the command goes into the command's # trace output rather than to this script's stderr. -# The subshell unsets every variable whose name starts with `CRONIC_`, and this -# script's functions, so that a builtin or shell function that is the command -# does not see this script's variables and functions, and so that a function -# does not take precedence over a program of the same name. Then the subshell -# restores the caller's variables and options. The subshell first turns off -# this script's xtrace and verbose, so that the restoring is not traced or -# echoed; any error in the restoring goes to the command's error output. +# The subshell unsets every variable whose name starts with `CRONIC_` and every +# function whose name starts with `cronic_`, so that a builtin or shell function +# that is the command does not see this script's variables and functions, and +# so that a function does not take precedence over a program of the same name. +# Then the subshell restores the caller's variables and options. The subshell +# first turns off this script's xtrace and verbose, so that the restoring is not +# traced or echoed; any error in the restoring goes to the command's error +# output. set +e ( { set +vx; } 2> /dev/null eval "unset -v \"\${!CRONIC_@}\" - unset -f cronic_cleanup cronic_usage_line cronic_usage cronic_help cronic_set_expected + unset -f \$(compgen -A function cronic_) $CRONIC_CALLER_VARIABLES $CRONIC_CALLER_OPTIONS" "$@" @@ -222,90 +239,32 @@ set +e CRONIC_RESULT=$? set -e -# A trace line starts with PS4 after expansion, with its first character -# repeated once per level of nesting. Expansion changes from line to line, so -# the pattern matches PS4's literal text exactly, with its regular-expression -# metacharacters escaped, and matches anything where PS4 has a `$` expansion, a -# backquoted command, or a backslash escape. Quote characters are removed, as -# bash removes them when it expands PS4. If PS4 does not start with literal -# text, then no line is treated as a trace line. +# A trace line starts with PS4 after expansion, with PS4's first character +# repeated once per level of nesting. If PS4 contains no `$`, backquote, +# backslash, or quote character, then the pattern matches PS4 exactly. +# Otherwise, expansion changes from line to line, so the pattern matches only +# PS4's literal text before the first such character, followed (if PS4 ends +# with literal text) by anything and then PS4's literal ending. The literal +# ending excludes letters, digits, and the other characters that can end an +# expansion or escape. If PS4 does not start with literal text, then no line is +# treated as a trace line. CRONIC_PATTERN= case ${PS4-} in '' | [\$\`\\\'\"]*) ;; *) - CRONIC_REST=${PS4:1} CRONIC_PATTERN=^ - case $PS4 in - [.*[^]*) CRONIC_PATTERN+=\\ ;; - esac - CRONIC_PATTERN+="${PS4:0:1}\\+" - # shellcheck disable=SC2016 # These are regular expressions, not expansions. - CRONIC_RE_NAME='^\$[A-Za-z_][A-Za-z0-9_]*(.*)$' - # shellcheck disable=SC2016 # These are regular expressions, not expansions. - CRONIC_RE_COMMAND='^`[^`]*`?(.*)$' - CRONIC_RE_SINGLE_QUOTED="^'([^']*)'?(.*)\$" - while [ -n "$CRONIC_REST" ]; do - CRONIC_LITERAL= - # shellcheck disable=SC1003,SC2016 # The patterns are literal text. - case $CRONIC_REST in - '${'* | '$('*) - # Skip to the matching close brace or parenthesis. - CRONIC_OPEN=${CRONIC_REST:1:1} - if [ "$CRONIC_OPEN" = "{" ]; then CRONIC_CLOSE="}"; else CRONIC_CLOSE=")"; fi - CRONIC_DEPTH=0 - CRONIC_I=1 - while [ "$CRONIC_I" -lt ${#CRONIC_REST} ]; do - case ${CRONIC_REST:CRONIC_I:1} in - "$CRONIC_OPEN") CRONIC_DEPTH=$((CRONIC_DEPTH + 1)) ;; - "$CRONIC_CLOSE") CRONIC_DEPTH=$((CRONIC_DEPTH - 1)) ;; - esac - CRONIC_I=$((CRONIC_I + 1)) - if [ "$CRONIC_DEPTH" -eq 0 ]; then break; fi - done - CRONIC_REST=${CRONIC_REST:CRONIC_I} - ;; - '$'?*) - if [[ $CRONIC_REST =~ $CRONIC_RE_NAME ]]; then - CRONIC_REST=${BASH_REMATCH[1]} - else - CRONIC_REST=${CRONIC_REST:2} - fi - ;; - '`'*) - [[ $CRONIC_REST =~ $CRONIC_RE_COMMAND ]] - CRONIC_REST=${BASH_REMATCH[1]} - ;; - '\'*) - CRONIC_REST=${CRONIC_REST:2} - ;; - \'*) - [[ $CRONIC_REST =~ $CRONIC_RE_SINGLE_QUOTED ]] - CRONIC_LITERAL=${BASH_REMATCH[1]} - CRONIC_REST=${BASH_REMATCH[2]} - ;; - \"*) - CRONIC_REST=${CRONIC_REST:1} - continue - ;; - *) - CRONIC_LITERAL=${CRONIC_REST:0:1} - CRONIC_REST=${CRONIC_REST:1} - ;; - esac - if [ -n "$CRONIC_LITERAL" ]; then - while [ -n "$CRONIC_LITERAL" ]; do - case $CRONIC_LITERAL in - [.*[^\$\\]*) CRONIC_PATTERN+=\\ ;; - esac - CRONIC_PATTERN+=${CRONIC_LITERAL:0:1} - CRONIC_LITERAL=${CRONIC_LITERAL:1} - done - elif [ "${CRONIC_PATTERN: -2}" != ".*" ]; then - CRONIC_PATTERN+=".*" + cronic_append_literal "${PS4:0:1}" + CRONIC_PATTERN+='\+' + CRONIC_PREFIX=${PS4:1} + CRONIC_PREFIX=${CRONIC_PREFIX%%[\$\`\\\'\"]*} + cronic_append_literal "$CRONIC_PREFIX" + if [ "$CRONIC_PREFIX" != "${PS4:1}" ]; then + CRONIC_SUFFIX=${PS4##*[][\$\`\\\'\"\!\{\}\(\)_[:alnum:]]} + if [ -n "$CRONIC_SUFFIX" ]; then + CRONIC_PATTERN+='.*' + cronic_append_literal "$CRONIC_SUFFIX" fi - done - # A trailing `.*` matches nothing more than the empty string does. - CRONIC_PATTERN=${CRONIC_PATTERN%.\*} + fi ;; esac if [ -n "$CRONIC_PATTERN" ] && grep -q "$CRONIC_PATTERN" "$CRONIC_TRACE"; then diff --git a/tests/cronic-test/cronic-test.sh b/tests/cronic-test/cronic-test.sh index 409049b..89adb64 100755 --- a/tests/cronic-test/cronic-test.sh +++ b/tests/cronic-test/cronic-test.sh @@ -342,6 +342,15 @@ check "PS4 with several expansions, trace-only stderr" 0 silent \ "$work/trace-only" 0 check "PS4 with several expansions, error line that starts with +" 0 \ "report:^+++ b/file$" "$work/plus-stderr" +# A backslash escape can be longer than two characters. +PS4='+\D{%H}: ' +check "PS4 with a long escape, trace-only stderr" 0 silent "$work/trace-only" 0 +check "PS4 with a long escape, error line that starts with +" 0 \ + "report:^+++ b/file$" "$work/plus-stderr" +# A single quote within double quotes is literal. +PS4="+\"it's\" " +check "PS4 with a quote within quotes, trace-only stderr" 0 silent \ + "$work/trace-only" 0 unset PS4 # The command may be a shell builtin. diff --git a/tests/cronic-test/temp-file-test.sh b/tests/cronic-test/temp-file-test.sh index d4b1e75..c5508dd 100755 --- a/tests/cronic-test/temp-file-test.sh +++ b/tests/cronic-test/temp-file-test.sh @@ -43,13 +43,12 @@ temp_files() { ### The temporary file names are not derived from the process id. -# The old names were built from `cronic`'s process id. `cronic` runs the -# wrapped command in a subshell, so `cronic` is an ancestor of the wrapped -# command but not necessarily its parent; check the process id of every -# ancestor. Check both the hard-coded /tmp that the old names used and the -# $TMPDIR that this test sets, so that reintroducing the predictable names under -# either directory is caught. The test fails if no ancestor is checked, so -# that a broken `ps` cannot make it pass vacuously. +# A temporary file name built from `cronic`'s process id would be predictable. +# `cronic` runs the wrapped command in a subshell, so `cronic` is an ancestor of +# the wrapped command but not necessarily its parent; check the process id of +# every ancestor. Check both /tmp and the $TMPDIR that this test sets, so that +# predictable names under either directory are caught. The test fails if no +# ancestor is checked, so that a broken `ps` cannot make it pass vacuously. cat > "$work/report-ppid" << 'EOF' #!/bin/sh pid=$PPID @@ -72,7 +71,7 @@ chmod +x "$work/report-ppid" > "$work/output" 2>&1 \ || fail "nonzero exit status: $(cat "$work/output")" # At least `cronic` and this test are ancestors of the wrapped command. -if [ "$(wc -l < "$work/ancestors" 2> /dev/null || echo 0)" -lt 2 ]; then +if [ "$(wc -l 2> /dev/null < "$work/ancestors" || echo 0)" -lt 2 ]; then fail "the ancestors of the wrapped command were not checked" elif [ -e "$work/predictable" ]; then fail "temporary file names are predictable from the process id:" From 7c34a33379c83d4926314e1a379481f3a0c8647b Mon Sep 17 00:00:00 2001 From: Michael Ernst Date: Thu, 8 Oct 2026 20:04:31 -0700 Subject: [PATCH 14/28] Filter trace lines only when PS4 is literal, so that no real error is hidden --- cronic | 26 +++++------------- tests/cronic-test/cronic-test.sh | 45 +++++++++++++++++++------------- 2 files changed, 34 insertions(+), 37 deletions(-) diff --git a/cronic b/cronic index 2cd67db..688f318 100755 --- a/cronic +++ b/cronic @@ -14,6 +14,7 @@ # Extended by Michael Ernst to add the --expected-status, --permit-stderr, and # --duplicate-stderr options, which may be given in any order. # The options are documented in function `cronic_help`; run `cronic --help`. +# Trace lines are filtered only if PS4 contains no `$`, backquote, or backslash. # COMMAND runs in a subshell, with the caller's variables and shell options # rather than this script's. # This script runs under bash 3.2, which is /bin/bash on macOS, so it does not @@ -240,31 +241,18 @@ CRONIC_RESULT=$? set -e # A trace line starts with PS4 after expansion, with PS4's first character -# repeated once per level of nesting. If PS4 contains no `$`, backquote, -# backslash, or quote character, then the pattern matches PS4 exactly. -# Otherwise, expansion changes from line to line, so the pattern matches only -# PS4's literal text before the first such character, followed (if PS4 ends -# with literal text) by anything and then PS4's literal ending. The literal -# ending excludes letters, digits, and the other characters that can end an -# expansion or escape. If PS4 does not start with literal text, then no line is -# treated as a trace line. +# repeated once per level of nesting. If PS4 contains a `$`, backquote, or +# backslash, then its expansion can differ from line to line, so no line is +# treated as a trace line: any pattern for such a PS4 could also match, and +# hide, a real error. CRONIC_PATTERN= case ${PS4-} in - '' | [\$\`\\\'\"]*) ;; + '' | *[\$\`\\]*) ;; *) CRONIC_PATTERN=^ cronic_append_literal "${PS4:0:1}" CRONIC_PATTERN+='\+' - CRONIC_PREFIX=${PS4:1} - CRONIC_PREFIX=${CRONIC_PREFIX%%[\$\`\\\'\"]*} - cronic_append_literal "$CRONIC_PREFIX" - if [ "$CRONIC_PREFIX" != "${PS4:1}" ]; then - CRONIC_SUFFIX=${PS4##*[][\$\`\\\'\"\!\{\}\(\)_[:alnum:]]} - if [ -n "$CRONIC_SUFFIX" ]; then - CRONIC_PATTERN+='.*' - cronic_append_literal "$CRONIC_SUFFIX" - fi - fi + cronic_append_literal "${PS4:1}" ;; esac if [ -n "$CRONIC_PATTERN" ] && grep -q "$CRONIC_PATTERN" "$CRONIC_TRACE"; then diff --git a/tests/cronic-test/cronic-test.sh b/tests/cronic-test/cronic-test.sh index 89adb64..00851a3 100755 --- a/tests/cronic-test/cronic-test.sh +++ b/tests/cronic-test/cronic-test.sh @@ -91,6 +91,18 @@ echo "++ conflict" 1>&2 EOF chmod +x "$work/plus-stderr" +# Commands whose error output resembles trace lines for a PS4 with expansions. +cat > "$work/plus-colon-stderr" << 'EOF' +#!/bin/sh +echo "+++ conflict: in file.c" 1>&2 +EOF +chmod +x "$work/plus-colon-stderr" +cat > "$work/plus-space-stderr" << 'EOF' +#!/bin/sh +echo "+ error happened" 1>&2 +EOF +chmod +x "$work/plus-space-stderr" + # A bash command that reads an unset variable. cat > "$work/read-unset" << 'EOF' #!/bin/bash @@ -321,36 +333,33 @@ PATH="$saved_path" # Trace lines are recognized when PS4 contains regular-expression -# metacharacters, and when PS4 contains expansions. +# metacharacters or quotes. PS4='[trace] ' export PS4 check "PS4 with metacharacters, trace-only stderr" 0 silent "$work/trace-only" 0 check "PS4 with metacharacters, trace lines and real stderr" 0 \ "report:^a real error$" "$work/trace-and-stderr" 0 +# Quotes in PS4 are literal. +PS4="+\"it's\" " +check "PS4 with quotes, trace-only stderr" 0 silent "$work/trace-only" 0 +# When PS4 contains an expansion or escape, no line is treated as a trace line, +# so trace lines are reported, and so is an error line that resembles one. # shellcheck disable=SC2016 # The expansion is for the traced command to do. PS4='+${LINENO}: ' -check "PS4 with expansions, trace-only stderr" 0 silent "$work/trace-only" 0 -check "PS4 with expansions, trace lines and real stderr" 0 \ - "report:^a real error$" "$work/trace-and-stderr" 0 -# An error line that starts with PS4's first character, but does not otherwise -# look like a trace line, is reported. +check "PS4 with expansions, trace-only stderr" 0 \ + "report:^+3: echo 'the standard output'$" "$work/trace-only" 0 check "PS4 with expansions, error line that starts with +" 0 \ "report:^+++ b/file$" "$work/plus-stderr" # shellcheck disable=SC2016 # The expansion is for the traced command to do. PS4='+${BASH_SOURCE}:${LINENO}: ' -check "PS4 with several expansions, trace-only stderr" 0 silent \ - "$work/trace-only" 0 -check "PS4 with several expansions, error line that starts with +" 0 \ - "report:^+++ b/file$" "$work/plus-stderr" -# A backslash escape can be longer than two characters. +check "PS4 with several expansions, error line that resembles a trace line" 0 \ + "report:^+++ conflict: in file.c$" "$work/plus-colon-stderr" PS4='+\D{%H}: ' -check "PS4 with a long escape, trace-only stderr" 0 silent "$work/trace-only" 0 -check "PS4 with a long escape, error line that starts with +" 0 \ - "report:^+++ b/file$" "$work/plus-stderr" -# A single quote within double quotes is literal. -PS4="+\"it's\" " -check "PS4 with a quote within quotes, trace-only stderr" 0 silent \ - "$work/trace-only" 0 +check "PS4 with an escape, trace-only stderr" 0 report "$work/trace-only" 0 +# shellcheck disable=SC2016 # The expansion is for the traced command to do. +PS4='+$LINENO' +check "PS4 ending with an expansion, error line that starts with +" 0 \ + "report:^+ error happened$" "$work/plus-space-stderr" unset PS4 # The command may be a shell builtin. From ea5b9e9c8f5174c8e760685f61f7a5e6f01ff9d8 Mon Sep 17 00:00:00 2001 From: Michael Ernst Date: Thu, 8 Oct 2026 20:12:30 -0700 Subject: [PATCH 15/28] Address code review: keep the caller's cronic_ functions, simplify option restoring and PS4 escaping, and check only cronic's process id --- cronic | 75 ++++++++---------- tests/cronic-test/cronic-test.sh | 24 ++++++ tests/cronic-test/temp-file-test.sh | 117 ++++++++++++++-------------- 3 files changed, 116 insertions(+), 100 deletions(-) diff --git a/cronic b/cronic index 688f318..23e974a 100755 --- a/cronic +++ b/cronic @@ -23,11 +23,13 @@ # The caller's state, as commands that restore it, before this script changes # any of it for itself. The wrapped command runs with the caller's state # rather than this script's; see where it is run, below. -# * The caller's exported variables whose names start with `CRONIC_`, which -# this script's own variables would otherwise hide. -# * The caller's shell options. `set +o` reports `-e` and verbose as off, -# because bash turns them off in a command substitution, so `$-` supplies -# them. +# * The caller's exported variables whose names start with `CRONIC_`, and +# exported functions whose names start with `cronic_`, which this script's +# own variables and functions would otherwise hide. +# * The caller's `set -o` shell options. `set +o` reports `-e` and verbose as +# off, because bash turns them off in a command substitution, so `$-` +# supplies them. This script changes no `shopt` option, so the command +# inherits the caller's `shopt` options without any restoring. # If the caller exported SHELLOPTS, then this script does not trace itself or # echo its input: that output would go to this script's stderr, and cron would # send email for every run. An exported `verbose` nonetheless echoes the lines @@ -38,31 +40,36 @@ { # `compgen -e` lists exported variables, and fails if there are none, in which # case `declare -p` with no names would print every variable. The `if` makes - # the status 0, as `errexit` requires. The command substitution keeps `names` - # out of this script. + # the status 0, as `errexit` requires. `declare -F` marks an exported + # function with `x`. The command substitution keeps `names`, `flags`, and + # `name` out of this script. CRONIC_CALLER_VARIABLES=$( if names=$(compgen -e CRONIC_); then # shellcheck disable=SC2086 # Each name is one word. declare -p $names fi + declare -F | while read -r _ flags name; do + case "$flags $name" in + -*x*' 'cronic_*) + declare -f "$name" + echo "export -f $name" + ;; + esac + done ) - # verbose and xtrace come last, in one command, so that restoring them neither - # echoes nor traces the restoring of any option. + # `$-` supplies errexit, verbose, and xtrace. They come last, in one + # command, so that restoring verbose and xtrace neither echoes nor traces the + # restoring of any other option. CRONIC_CALLER_OPTIONS=$(set +o) - CRONIC_CALLER_OPTIONS=${CRONIC_CALLER_OPTIONS/$'\n'set [-+]o verbose/} - CRONIC_CALLER_OPTIONS=${CRONIC_CALLER_OPTIONS/$'\n'set [-+]o xtrace/} - case $- in - *e*) CRONIC_CALLER_OPTIONS=${CRONIC_CALLER_OPTIONS/set +o errexit/set -o errexit} ;; - esac - CRONIC_CALLER_OPTIONS+=$'\n''set ' - case $- in - *v*) CRONIC_CALLER_OPTIONS+='-o verbose ' ;; - *) CRONIC_CALLER_OPTIONS+='+o verbose ' ;; - esac - case $- in - *x*) CRONIC_CALLER_OPTIONS+='-o xtrace' ;; - *) CRONIC_CALLER_OPTIONS+='+o xtrace' ;; - esac + CRONIC_CALLER_SET='set' + for CRONIC_OPTION in e:errexit v:verbose x:xtrace; do + CRONIC_CALLER_OPTIONS=${CRONIC_CALLER_OPTIONS/$'\n'set [-+]o ${CRONIC_OPTION#*:}/} + case $- in + *"${CRONIC_OPTION%%:*}"*) CRONIC_CALLER_SET+=" -o ${CRONIC_OPTION#*:}" ;; + *) CRONIC_CALLER_SET+=" +o ${CRONIC_OPTION#*:}" ;; + esac + done + CRONIC_CALLER_OPTIONS+=$'\n'$CRONIC_CALLER_SET if [[ $(declare -p SHELLOPTS) =~ ^declare\ -[a-zA-Z]*x ]]; then set +vx; fi } 2> /dev/null @@ -173,19 +180,6 @@ CRONIC_ERR=$CRONIC_TMPDIR/err CRONIC_REDUCED_ERR=$CRONIC_TMPDIR/err.reduced CRONIC_TRACE=$CRONIC_TMPDIR/trace -# Appends its argument to CRONIC_PATTERN, as a basic regular expression that -# matches the argument literally. -cronic_append_literal() { - local text=$1 - while [ -n "$text" ]; do - case $text in - [.[*^\$\\]*) CRONIC_PATTERN+=\\ ;; - esac - CRONIC_PATTERN+=${text:0:1} - text=${text:1} - done -} - # Remove the temporary files however this script terminates, including when it # is interrupted before it reaches the end. When CRONIC_DEBUG is true, the # files are retained; don't "echo" their location, because that would be @@ -208,7 +202,7 @@ trap 'cronic_cleanup; trap - HUP EXIT; kill -s HUP $$' HUP trap 'cronic_cleanup; trap - INT EXIT; kill -s INT $$' INT trap 'cronic_cleanup; trap - TERM EXIT; kill -s TERM $$' TERM -# Run the command with all of the caller's shell options rather than this +# Run the command with the caller's `set -o` shell options rather than this # script's. If the caller exported SHELLOPTS, then the command would otherwise # inherit this script's options, and a bash script that reads an unset variable # (as `/usr/bin/texi2dvi` does when /bin/sh is bash) would fail. @@ -245,14 +239,13 @@ set -e # backslash, then its expansion can differ from line to line, so no line is # treated as a trace line: any pattern for such a PS4 could also match, and # hide, a real error. +# The `sed` command escapes PS4's basic-regular-expression metacharacters, then +# makes the first (possibly escaped) character repeatable. CRONIC_PATTERN= case ${PS4-} in '' | *[\$\`\\]*) ;; *) - CRONIC_PATTERN=^ - cronic_append_literal "${PS4:0:1}" - CRONIC_PATTERN+='\+' - cronic_append_literal "${PS4:1}" + CRONIC_PATTERN=^$(printf '%s\n' "$PS4" | sed 's/[*.[^$]/\\&/g; s/^\\\{0,1\}./&\\+/') ;; esac if [ -n "$CRONIC_PATTERN" ] && grep -q "$CRONIC_PATTERN" "$CRONIC_TRACE"; then diff --git a/tests/cronic-test/cronic-test.sh b/tests/cronic-test/cronic-test.sh index 00851a3..2a5bd75 100755 --- a/tests/cronic-test/cronic-test.sh +++ b/tests/cronic-test/cronic-test.sh @@ -498,6 +498,30 @@ export DEBUG OUT CRONIC_DEBUG CRONIC_TMPDIR check "caller's exported variables" 0 silent "$work/check-variables" unset DEBUG OUT CRONIC_DEBUG CRONIC_TMPDIR +# `cronic` does not remove a function that the caller exported, whether its +# name is one of `cronic`'s own functions or merely starts like them. The +# function is visible both to the command and to the command's children. +cat > "$work/cronic-with-cronic-functions" << 'EOF' +#!/bin/bash +cronic_mine() { + exit 3 +} +cronic_usage() { + exit 4 +} +export -f cronic_mine cronic_usage +exec "$1" "${@:2}" +EOF +chmod +x "$work/cronic-with-cronic-functions" +CRONIC="$work/cronic-with-cronic-functions" +check "caller's exported function named cronic_mine" 3 silent \ + "$REAL_CRONIC" --expected-status 3 cronic_mine +check "caller's exported function named cronic_mine, in a child" 3 silent \ + "$REAL_CRONIC" --expected-status 3 bash -c cronic_mine +check "caller's exported function named cronic_usage" 4 silent \ + "$REAL_CRONIC" --expected-status 4 cronic_usage +CRONIC="$REAL_CRONIC" + # `bash -x cronic`, which does not export SHELLOPTS, traces `cronic` itself. bash_x_status=0 bash -x "$CRONIC" "$work/trace-only" 0 > "$work/output" 2> "$work/stderr" \ diff --git a/tests/cronic-test/temp-file-test.sh b/tests/cronic-test/temp-file-test.sh index c5508dd..f135101 100755 --- a/tests/cronic-test/temp-file-test.sh +++ b/tests/cronic-test/temp-file-test.sh @@ -2,11 +2,12 @@ # Tests how `cronic` creates and removes its temporary files. # -# `cronic` used to write to `/tmp/cronic.out.$$` and three sibling paths. -# Process ids are guessable and reused, so on a multi-user machine another user -# could pre-create those paths as symlinks and have the wrapped command's -# output written through them. `cronic` also had no signal handler, so a run -# that was interrupted before its final `rm` left all four files behind. +# Temporary file names built from the process id, such as `/tmp/cronic.out.$$`, +# would be unsafe: process ids are guessable and reused, so on a multi-user +# machine another user could pre-create those paths as symlinks and have the +# wrapped command's output written through them. Without a signal handler, a +# run that was interrupted before its final cleanup would leave its temporary +# files behind. set -eu @@ -41,48 +42,7 @@ temp_files() { find "$TMPDIR" -mindepth 1 -maxdepth 1 2> /dev/null | sort } -### The temporary file names are not derived from the process id. - -# A temporary file name built from `cronic`'s process id would be predictable. -# `cronic` runs the wrapped command in a subshell, so `cronic` is an ancestor of -# the wrapped command but not necessarily its parent; check the process id of -# every ancestor. Check both /tmp and the $TMPDIR that this test sets, so that -# predictable names under either directory are caught. The test fails if no -# ancestor is checked, so that a broken `ps` cannot make it pass vacuously. -cat > "$work/report-ppid" << 'EOF' -#!/bin/sh -pid=$PPID -while [ -n "$pid" ] && [ "$pid" -gt 1 ]; do - echo "$pid" >> "$2" - for dir in "/tmp" "${TMPDIR:-/tmp}"; do - for file in "$dir/cronic.out.$pid" "$dir/cronic.err.$pid" \ - "$dir/cronic.err.reduced.$pid" "$dir/cronic.trace.$pid"; do - if [ -e "$file" ]; then - echo "$file" >> "$1" - fi - done - done - pid=$(ps -o ppid= -p "$pid" | tr -d ' ') -done -EOF -chmod +x "$work/report-ppid" - -"$CRONIC" "$work/report-ppid" "$work/predictable" "$work/ancestors" \ - > "$work/output" 2>&1 \ - || fail "nonzero exit status: $(cat "$work/output")" -# At least `cronic` and this test are ancestors of the wrapped command. -if [ "$(wc -l 2> /dev/null < "$work/ancestors" || echo 0)" -lt 2 ]; then - fail "the ancestors of the wrapped command were not checked" -elif [ -e "$work/predictable" ]; then - fail "temporary file names are predictable from the process id:" - cat "$work/predictable" -else - pass "temporary file names are not predictable from the process id" -fi - -### An interrupted run leaves no temporary files behind. - -# Waits for the test to create the "release" file, so that the signal arrives +# Waits for the test to create the "release" file, so that the test can act # while `cronic` is running the command rather than before or after. cat > "$work/wait-for-release" << 'EOF' #!/bin/sh @@ -93,25 +53,64 @@ done EOF chmod +x "$work/wait-for-release" -before="$(temp_files)" -"$CRONIC" "$work/wait-for-release" "$work/started" "$work/release" \ +# wait_for_start FILE: waits until the wrapped command has created FILE. +wait_for_start() { + waited=0 + while [ ! -e "$1" ]; do + if [ "$waited" -ge 100 ]; then + echo "the wrapped command did not start" + exit 1 + fi + sleep 0.1 + waited=$((waited + 1)) + done +} + +### The temporary file names are not derived from the process id. + +# A temporary file name built from `$$` would be predictable. `$$` is +# `cronic`'s process id even within its subshells, so `cronic`'s process id is +# the only one to check. While the wrapped command runs, look for that id in +# the names under the $TMPDIR that this test sets, and for the predictable names +# under /tmp. The test fails if `cronic` has no temporary files at that time, +# so that it cannot pass vacuously. +"$CRONIC" "$work/wait-for-release" "$work/started-1" "$work/release-1" \ > "$work/output" 2>&1 & cronic_pid=$! +wait_for_start "$work/started-1" +during="$(temp_files)" +predictable="$( + find "$TMPDIR" -name "*$cronic_pid*" + for file in "/tmp/cronic.out.$cronic_pid" "/tmp/cronic.err.$cronic_pid" \ + "/tmp/cronic.err.reduced.$cronic_pid" "/tmp/cronic.trace.$cronic_pid"; do + if [ -e "$file" ]; then + echo "$file" + fi + done +)" +touch "$work/release-1" +wait "$cronic_pid" || fail "nonzero exit status: $(cat "$work/output")" +if [ -z "$during" ]; then + fail "cronic had no temporary files while the wrapped command ran" +elif [ -n "$predictable" ]; then + fail "temporary file names are predictable from the process id:" + echo "$predictable" +else + pass "temporary file names are not predictable from the process id" +fi -waited=0 -while [ ! -e "$work/started" ]; do - if [ "$waited" -ge 100 ]; then - echo "the wrapped command did not start" - exit 1 - fi - sleep 0.1 - waited=$((waited + 1)) -done +### An interrupted run leaves no temporary files behind. + +before="$(temp_files)" +"$CRONIC" "$work/wait-for-release" "$work/started-2" "$work/release-2" \ + > "$work/output" 2>&1 & +cronic_pid=$! +wait_for_start "$work/started-2" # `cronic` is waiting for the wrapped command, so it handles the signal only # once that command has finished; release the command after signaling. kill -TERM "$cronic_pid" -touch "$work/release" +touch "$work/release-2" cronic_status=0 # Redirect stderr to discard the shell's "Terminated" job message, which would # otherwise look like a failure in the test output. From 676facf2c526f4017488d2f442b97cad42ad520d Mon Sep 17 00:00:00 2001 From: Michael Ernst Date: Thu, 8 Oct 2026 20:12:57 -0700 Subject: [PATCH 16/28] Describe what an unguarded grep would do, not what it did --- tests/cronic-test/cronic-test.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/cronic-test/cronic-test.sh b/tests/cronic-test/cronic-test.sh index 2a5bd75..cb52265 100755 --- a/tests/cronic-test/cronic-test.sh +++ b/tests/cronic-test/cronic-test.sh @@ -6,9 +6,9 @@ # # `cronic` filters trace lines out of stderr with `grep -v`, which exits with # status 1 when it selects no lines. `cronic` runs under `set -e`, so an -# unguarded `grep -v` aborted the script at that point: no report was printed, -# the exit status was grep's 1 rather than the wrapped command's, and all of -# the temporary files under /tmp were left behind. +# unguarded `grep -v` would abort the script at that point: no report would be +# printed, the exit status would be grep's 1 rather than the wrapped command's, +# and the temporary files would be left behind. set -eu From 5e73d5129361a6d9effdfe86210e5c46e8bf3159 Mon Sep 17 00:00:00 2001 From: Michael Ernst Date: Thu, 8 Oct 2026 20:20:05 -0700 Subject: [PATCH 17/28] Check for predictable temporary file names while `cronic` runs the command --- tests/cronic-test/temp-file-test.sh | 102 +++++++++++++++------------- 1 file changed, 56 insertions(+), 46 deletions(-) diff --git a/tests/cronic-test/temp-file-test.sh b/tests/cronic-test/temp-file-test.sh index 2212266..f135101 100755 --- a/tests/cronic-test/temp-file-test.sh +++ b/tests/cronic-test/temp-file-test.sh @@ -2,11 +2,12 @@ # Tests how `cronic` creates and removes its temporary files. # -# `cronic` used to write to `/tmp/cronic.out.$$` and three sibling paths. -# Process ids are guessable and reused, so on a multi-user machine another user -# could pre-create those paths as symlinks and have the wrapped command's -# output written through them. `cronic` also had no signal handler, so a run -# that was interrupted before its final `rm` left all four files behind. +# Temporary file names built from the process id, such as `/tmp/cronic.out.$$`, +# would be unsafe: process ids are guessable and reused, so on a multi-user +# machine another user could pre-create those paths as symlinks and have the +# wrapped command's output written through them. Without a signal handler, a +# run that was interrupted before its final cleanup would leave its temporary +# files behind. set -eu @@ -41,66 +42,75 @@ temp_files() { find "$TMPDIR" -mindepth 1 -maxdepth 1 2> /dev/null | sort } +# Waits for the test to create the "release" file, so that the test can act +# while `cronic` is running the command rather than before or after. +cat > "$work/wait-for-release" << 'EOF' +#!/bin/sh +touch "$1" +while [ ! -e "$2" ]; do + sleep 0.1 +done +EOF +chmod +x "$work/wait-for-release" + +# wait_for_start FILE: waits until the wrapped command has created FILE. +wait_for_start() { + waited=0 + while [ ! -e "$1" ]; do + if [ "$waited" -ge 100 ]; then + echo "the wrapped command did not start" + exit 1 + fi + sleep 0.1 + waited=$((waited + 1)) + done +} + ### The temporary file names are not derived from the process id. -# The wrapped command's parent is `cronic` itself, so $PPID is the process id -# that the old names were built from. Check both the hard-coded /tmp that the -# old names used and the $TMPDIR that this test sets, so that reintroducing the -# predictable names under either directory is caught. -cat > "$work/report-ppid" << 'EOF' -#!/bin/sh -for dir in "/tmp" "${TMPDIR:-/tmp}"; do - for file in "$dir/cronic.out.$PPID" "$dir/cronic.err.$PPID" \ - "$dir/cronic.err.reduced.$PPID" "$dir/cronic.trace.$PPID"; do +# A temporary file name built from `$$` would be predictable. `$$` is +# `cronic`'s process id even within its subshells, so `cronic`'s process id is +# the only one to check. While the wrapped command runs, look for that id in +# the names under the $TMPDIR that this test sets, and for the predictable names +# under /tmp. The test fails if `cronic` has no temporary files at that time, +# so that it cannot pass vacuously. +"$CRONIC" "$work/wait-for-release" "$work/started-1" "$work/release-1" \ + > "$work/output" 2>&1 & +cronic_pid=$! +wait_for_start "$work/started-1" +during="$(temp_files)" +predictable="$( + find "$TMPDIR" -name "*$cronic_pid*" + for file in "/tmp/cronic.out.$cronic_pid" "/tmp/cronic.err.$cronic_pid" \ + "/tmp/cronic.err.reduced.$cronic_pid" "/tmp/cronic.trace.$cronic_pid"; do if [ -e "$file" ]; then - echo "$file" >> "$1" + echo "$file" fi done -done -EOF -chmod +x "$work/report-ppid" - -"$CRONIC" "$work/report-ppid" "$work/predictable" > "$work/output" 2>&1 \ - || fail "nonzero exit status: $(cat "$work/output")" -if [ -e "$work/predictable" ]; then +)" +touch "$work/release-1" +wait "$cronic_pid" || fail "nonzero exit status: $(cat "$work/output")" +if [ -z "$during" ]; then + fail "cronic had no temporary files while the wrapped command ran" +elif [ -n "$predictable" ]; then fail "temporary file names are predictable from the process id:" - cat "$work/predictable" + echo "$predictable" else pass "temporary file names are not predictable from the process id" fi ### An interrupted run leaves no temporary files behind. -# Waits for the test to create the "release" file, so that the signal arrives -# while `cronic` is running the command rather than before or after. -cat > "$work/wait-for-release" << 'EOF' -#!/bin/sh -touch "$1" -while [ ! -e "$2" ]; do - sleep 0.1 -done -EOF -chmod +x "$work/wait-for-release" - before="$(temp_files)" -"$CRONIC" "$work/wait-for-release" "$work/started" "$work/release" \ +"$CRONIC" "$work/wait-for-release" "$work/started-2" "$work/release-2" \ > "$work/output" 2>&1 & cronic_pid=$! - -waited=0 -while [ ! -e "$work/started" ]; do - if [ "$waited" -ge 100 ]; then - echo "the wrapped command did not start" - exit 1 - fi - sleep 0.1 - waited=$((waited + 1)) -done +wait_for_start "$work/started-2" # `cronic` is waiting for the wrapped command, so it handles the signal only # once that command has finished; release the command after signaling. kill -TERM "$cronic_pid" -touch "$work/release" +touch "$work/release-2" cronic_status=0 # Redirect stderr to discard the shell's "Terminated" job message, which would # otherwise look like a failure in the test output. From 42adb1beb21395ed28fb72bb77bf938eae419434 Mon Sep 17 00:00:00 2001 From: Michael Ernst Date: Thu, 8 Oct 2026 20:20:06 -0700 Subject: [PATCH 18/28] Prefix `cronic`'s variables with `CRONIC_`, so that it does not change the caller's exported variables --- cronic | 96 +++++++++++++++++--------------- tests/cronic-test/cronic-test.sh | 14 +++++ 2 files changed, 65 insertions(+), 45 deletions(-) diff --git a/cronic b/cronic index 70f35d7..87c8811 100755 --- a/cronic +++ b/cronic @@ -19,6 +19,11 @@ # as an error. set -eu +# The names of this script's functions start with "cronic_", and the names of +# its variables start with "CRONIC_", so that they are unlikely to be the names +# of the caller's exported variables. An assignment to a variable that the +# caller exported would change the value that the wrapped command sees. + # Prints the usage line. cronic_usage_line() { echo "Usage: $0 [--expected-status N] [--permit-stderr] [--duplicate-stderr] [--] COMMAND [ARG...]" @@ -47,23 +52,23 @@ cronic_help() { exit 0 } -# Sets EXPECTED to its argument, or reports a usage error if the argument is -# not a valid exit status. +# Sets CRONIC_EXPECTED to its argument, or reports a usage error if the +# argument is not a valid exit status. cronic_set_expected() { - EXPECTED=$1 + CRONIC_EXPECTED=$1 # No leading zeros, because `[[ ... -eq ... ]]` would read the value as # octal. At most 255, because no exit status is larger. - if ! [[ $EXPECTED =~ ^(0|[1-9][0-9]{0,2})$ ]] || [ "$EXPECTED" -gt 255 ]; then - echo "$0: --expected-status requires an integer from 0 to 255, got \"$EXPECTED\"" >&2 + if ! [[ $CRONIC_EXPECTED =~ ^(0|[1-9][0-9]{0,2})$ ]] || [ "$CRONIC_EXPECTED" -gt 255 ]; then + echo "$0: --expected-status requires an integer from 0 to 255, got \"$CRONIC_EXPECTED\"" >&2 cronic_usage fi } # The options are parsed before the temporary directory is created, so that # `--help` and a usage error neither create nor depend on it. -EXPECTED=0 -PERMITSTDERR=0 -DUPLICATE_STDERR=0 +CRONIC_EXPECTED=0 +CRONIC_PERMITSTDERR=0 +CRONIC_DUPLICATE_STDERR=0 while [ $# -gt 0 ]; do case $1 in --expected-status) @@ -81,11 +86,11 @@ while [ $# -gt 0 ]; do ;; --permit-stderr) shift - PERMITSTDERR=1 + CRONIC_PERMITSTDERR=1 ;; --duplicate-stderr) shift - DUPLICATE_STDERR=1 + CRONIC_DUPLICATE_STDERR=1 ;; -h | --help) cronic_help @@ -105,26 +110,26 @@ while [ $# -gt 0 ]; do done [ $# -gt 0 ] || cronic_usage -DEBUG=false -# DEBUG=true +CRONIC_DEBUG=false +# CRONIC_DEBUG=true # A private directory, rather than predictable `/tmp/cronic.*.$$` names: on a # shared machine, process ids are guessable and reused, so another user could # pre-create those paths as symlinks and have the wrapped command's output # written through them. CRONIC_TMPDIR=$(mktemp -d "${TMPDIR:-/tmp}/cronic.XXXXXX") || exit 1 -OUT=$CRONIC_TMPDIR/out -ERR=$CRONIC_TMPDIR/err -REDUCED_ERR=$CRONIC_TMPDIR/err.reduced -TRACE=$CRONIC_TMPDIR/trace +CRONIC_OUT=$CRONIC_TMPDIR/out +CRONIC_ERR=$CRONIC_TMPDIR/err +CRONIC_REDUCED_ERR=$CRONIC_TMPDIR/err.reduced +CRONIC_TRACE=$CRONIC_TMPDIR/trace # Remove the temporary files however this script terminates, including when it -# is interrupted before it reaches the end. When DEBUG is true, the files are -# retained; don't "echo" their location, because that would be appended to -# stderr. +# is interrupted before it reaches the end. When CRONIC_DEBUG is true, the +# files are retained; don't "echo" their location, because that would be +# appended to stderr. # shellcheck disable=SC2329 # the function is invoked from the trap handlers below. cronic_cleanup() { - [ "$DEBUG" = "true" ] || rm -rf "$CRONIC_TMPDIR" + [ "$CRONIC_DEBUG" = "true" ] || rm -rf "$CRONIC_TMPDIR" } trap cronic_cleanup EXIT @@ -144,65 +149,66 @@ set +e # `command` because the wrapped command runs in this shell, where a function of # this script, such as `cronic_usage`, would otherwise run in place of a # program of the same name. -command -- "$@" > "$OUT" 2> "$TRACE" -RESULT=$? +command -- "$@" > "$CRONIC_OUT" 2> "$CRONIC_TRACE" +CRONIC_RESULT=$? set -e -PATTERN="^${PS4:0:1}\\+${PS4:1}" -if grep -q "$PATTERN" "$TRACE"; then +CRONIC_PATTERN="^${PS4:0:1}\\+${PS4:1}" +if grep -q "$CRONIC_PATTERN" "$CRONIC_TRACE"; then # `|| true` because `grep -v` exits with status 1 when it selects no lines, # which happens whenever stderr consists only of trace lines. Without it, # `set -e` would abort the script here, before it produces its report, # exits with the wrapped command's status, and removes its temporary files. - grep -v "$PATTERN" "$TRACE" > "$ERR" || true + grep -v "$CRONIC_PATTERN" "$CRONIC_TRACE" > "$CRONIC_ERR" || true else - ERR=$TRACE + CRONIC_ERR=$CRONIC_TRACE fi # Certain Unix programs send output to stderr even when a command is successful! # For example, git does this, and the POSIX standard requires it for make. -# The full stderr output is shown, but file REDUCED_ERR is checked for emptiness below. +# The full stderr output is shown, but file CRONIC_REDUCED_ERR is checked for +# emptiness below. # TODO: Expand this. -cat "$ERR" \ +cat "$CRONIC_ERR" \ | grep -v "^Everything up-to-date$" \ | grep -v '^make\(\[[0-9]\+\]\)\?: \(Entering\|Leaving\) directory '"'"'.*'"'"'$' \ - > "$REDUCED_ERR" || true + > "$CRONIC_REDUCED_ERR" || true -if [ $RESULT -ne "$EXPECTED" ] || { [ $PERMITSTDERR -eq 0 ] && [ -s "$REDUCED_ERR" ]; }; then +if [ $CRONIC_RESULT -ne "$CRONIC_EXPECTED" ] || { [ $CRONIC_PERMITSTDERR -eq 0 ] && [ -s "$CRONIC_REDUCED_ERR" ]; }; then echo "Cronic detected failure or error output for the command:" echo "$@" echo - if [[ $EXPECTED -eq 0 ]]; then - if [[ $RESULT -eq 0 ]]; then - echo "RESULT CODE: $RESULT (success)" + if [[ $CRONIC_EXPECTED -eq 0 ]]; then + if [[ $CRONIC_RESULT -eq 0 ]]; then + echo "RESULT CODE: $CRONIC_RESULT (success)" else - echo "RESULT CODE: $RESULT (failure)" + echo "RESULT CODE: $CRONIC_RESULT (failure)" fi else - echo "RESULT CODE: $RESULT (expected: $EXPECTED)" + echo "RESULT CODE: $CRONIC_RESULT (expected: $CRONIC_EXPECTED)" fi echo echo "ERROR OUTPUT:" - cat "$ERR" + cat "$CRONIC_ERR" echo - if [ "$DEBUG" != "true" ]; then - if ! cmp --silent "$ERR" "$REDUCED_ERR"; then + if [ "$CRONIC_DEBUG" != "true" ]; then + if ! cmp --silent "$CRONIC_ERR" "$CRONIC_REDUCED_ERR"; then echo "REDUCED ERROR OUTPUT:" - cat "$REDUCED_ERR" + cat "$CRONIC_REDUCED_ERR" echo fi fi echo "STANDARD OUTPUT:" - cat "$OUT" - if [ "$TRACE" != "$ERR" ]; then + cat "$CRONIC_OUT" + if [ "$CRONIC_TRACE" != "$CRONIC_ERR" ]; then echo echo "TRACE-ERROR OUTPUT:" - cat "$TRACE" + cat "$CRONIC_TRACE" fi - if [ $DUPLICATE_STDERR -eq 1 ]; then + if [ $CRONIC_DUPLICATE_STDERR -eq 1 ]; then echo echo "ERROR OUTPUT:" - cat "$ERR" + cat "$CRONIC_ERR" fi echo echo "END OF CRONIC OUTPUT." @@ -210,4 +216,4 @@ fi # The temporary files are removed by the EXIT trap. -exit $RESULT +exit $CRONIC_RESULT diff --git a/tests/cronic-test/cronic-test.sh b/tests/cronic-test/cronic-test.sh index 35cc622..af823fc 100755 --- a/tests/cronic-test/cronic-test.sh +++ b/tests/cronic-test/cronic-test.sh @@ -245,4 +245,18 @@ for name in $function_names; do done PATH="$saved_path" +# `cronic` does not change a variable that the caller exported, even one whose +# name is a plain word such as `DEBUG` or `OUT`. +cat > "$work/check-variables" << 'EOF' +#!/bin/sh +[ "$DEBUG" = "caller's DEBUG" ] && [ "$OUT" = "caller's OUT" ] \ + && [ "$RESULT" = "caller's RESULT" ] || exit 3 +EOF +chmod +x "$work/check-variables" +DEBUG="caller's DEBUG" +OUT="caller's OUT" +RESULT="caller's RESULT" +export DEBUG OUT RESULT +check "caller's exported variables" 0 silent "$work/check-variables" + exit "$status" From dfeec8bcc944b0baa0dc5010db3ee1c333cee039 Mon Sep 17 00:00:00 2001 From: Michael Ernst Date: Thu, 8 Oct 2026 20:20:08 -0700 Subject: [PATCH 19/28] Filter trace lines only when PS4 is literal, and escape its metacharacters --- cronic | 18 +++++++++-- tests/cronic-test/cronic-test.sh | 51 ++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 2 deletions(-) diff --git a/cronic b/cronic index 70f35d7..b40c197 100755 --- a/cronic +++ b/cronic @@ -14,6 +14,7 @@ # Extended by Michael Ernst to add the --expected-status, --permit-stderr, and # --duplicate-stderr options, which may be given in any order. # The options are documented in function `cronic_help`; run `cronic --help`. +# Trace lines are filtered only if PS4 contains no `$`, backquote, or backslash. # Exit if a command exits with a non-zero status, and treat unset variables # as an error. @@ -148,8 +149,21 @@ command -- "$@" > "$OUT" 2> "$TRACE" RESULT=$? set -e -PATTERN="^${PS4:0:1}\\+${PS4:1}" -if grep -q "$PATTERN" "$TRACE"; then +# A trace line starts with PS4 after expansion, with PS4's first character +# repeated once per level of nesting. If PS4 contains a `$`, backquote, or +# backslash, then its expansion can differ from line to line, so no line is +# treated as a trace line: any pattern for such a PS4 could also match, and +# hide, a real error. +# The `sed` command escapes PS4's basic-regular-expression metacharacters, then +# makes the first (possibly escaped) character repeatable. +PATTERN= +case ${PS4-} in + '' | *[\$\`\\]*) ;; + *) + PATTERN=^$(printf '%s\n' "$PS4" | sed 's/[*.[^$]/\\&/g; s/^\\\{0,1\}./&\\+/') + ;; +esac +if [ -n "$PATTERN" ] && grep -q "$PATTERN" "$TRACE"; then # `|| true` because `grep -v` exits with status 1 when it selects no lines, # which happens whenever stderr consists only of trace lines. Without it, # `set -e` would abort the script here, before it produces its report, diff --git a/tests/cronic-test/cronic-test.sh b/tests/cronic-test/cronic-test.sh index 35cc622..3fd63e8 100755 --- a/tests/cronic-test/cronic-test.sh +++ b/tests/cronic-test/cronic-test.sh @@ -70,6 +70,27 @@ exit "$1" EOF chmod +x "$work/make-noise-and-stderr" +# A command that writes, to stderr, lines that start with `+` but are not trace +# lines. +cat > "$work/plus-stderr" << 'EOF' +#!/bin/sh +echo "+++ b/file" 1>&2 +echo "++ conflict" 1>&2 +EOF +chmod +x "$work/plus-stderr" + +# Commands whose error output resembles trace lines for a PS4 with expansions. +cat > "$work/plus-colon-stderr" << 'EOF' +#!/bin/sh +echo "+++ conflict: in file.c" 1>&2 +EOF +chmod +x "$work/plus-colon-stderr" +cat > "$work/plus-space-stderr" << 'EOF' +#!/bin/sh +echo "+ error happened" 1>&2 +EOF +chmod +x "$work/plus-space-stderr" + # temp_files: prints `cronic`'s temporary files, in a canonical order. temp_files() { find "$TMPDIR" -mindepth 1 -maxdepth 1 2> /dev/null | sort @@ -245,4 +266,34 @@ for name in $function_names; do done PATH="$saved_path" +# Trace lines are recognized when PS4 contains regular-expression +# metacharacters or quotes. +PS4='[trace] ' +export PS4 +check "PS4 with metacharacters, trace-only stderr" 0 silent "$work/trace-only" 0 +check "PS4 with metacharacters, trace lines and real stderr" 0 \ + "report:^a real error$" "$work/trace-and-stderr" 0 +# Quotes in PS4 are literal. +PS4="+\"it's\" " +check "PS4 with quotes, trace-only stderr" 0 silent "$work/trace-only" 0 +# When PS4 contains an expansion or escape, no line is treated as a trace line, +# so trace lines are reported, and so is an error line that resembles one. +# shellcheck disable=SC2016 # The expansion is for the traced command to do. +PS4='+${LINENO}: ' +check "PS4 with expansions, trace-only stderr" 0 \ + "report:^+3: echo 'the standard output'$" "$work/trace-only" 0 +check "PS4 with expansions, error line that starts with +" 0 \ + "report:^+++ b/file$" "$work/plus-stderr" +# shellcheck disable=SC2016 # The expansion is for the traced command to do. +PS4='+${BASH_SOURCE}:${LINENO}: ' +check "PS4 with several expansions, error line that resembles a trace line" 0 \ + "report:^+++ conflict: in file.c$" "$work/plus-colon-stderr" +PS4='+\D{%H}: ' +check "PS4 with an escape, trace-only stderr" 0 report "$work/trace-only" 0 +# shellcheck disable=SC2016 # The expansion is for the traced command to do. +PS4='+$LINENO' +check "PS4 ending with an expansion, error line that starts with +" 0 \ + "report:^+ error happened$" "$work/plus-space-stderr" +unset PS4 + exit "$status" From 8e9b449218a709eaa27472547f30fbe55a3b8b69 Mon Sep 17 00:00:00 2001 From: Michael Ernst Date: Thu, 8 Oct 2026 20:25:33 -0700 Subject: [PATCH 20/28] Remove the PS4 and temporary-file-test changes, which are in #133 and #131 --- cronic | 18 +---- tests/cronic-test/cronic-test.sh | 52 -------------- tests/cronic-test/temp-file-test.sh | 102 +++++++++++++--------------- 3 files changed, 48 insertions(+), 124 deletions(-) diff --git a/cronic b/cronic index 23e974a..40ac745 100755 --- a/cronic +++ b/cronic @@ -14,7 +14,6 @@ # Extended by Michael Ernst to add the --expected-status, --permit-stderr, and # --duplicate-stderr options, which may be given in any order. # The options are documented in function `cronic_help`; run `cronic --help`. -# Trace lines are filtered only if PS4 contains no `$`, backquote, or backslash. # COMMAND runs in a subshell, with the caller's variables and shell options # rather than this script's. # This script runs under bash 3.2, which is /bin/bash on macOS, so it does not @@ -234,21 +233,8 @@ set +e CRONIC_RESULT=$? set -e -# A trace line starts with PS4 after expansion, with PS4's first character -# repeated once per level of nesting. If PS4 contains a `$`, backquote, or -# backslash, then its expansion can differ from line to line, so no line is -# treated as a trace line: any pattern for such a PS4 could also match, and -# hide, a real error. -# The `sed` command escapes PS4's basic-regular-expression metacharacters, then -# makes the first (possibly escaped) character repeatable. -CRONIC_PATTERN= -case ${PS4-} in - '' | *[\$\`\\]*) ;; - *) - CRONIC_PATTERN=^$(printf '%s\n' "$PS4" | sed 's/[*.[^$]/\\&/g; s/^\\\{0,1\}./&\\+/') - ;; -esac -if [ -n "$CRONIC_PATTERN" ] && grep -q "$CRONIC_PATTERN" "$CRONIC_TRACE"; then +CRONIC_PATTERN="^${PS4:0:1}\\+${PS4:1}" +if grep -q "$CRONIC_PATTERN" "$CRONIC_TRACE"; then # `|| true` because `grep -v` exits with status 1 when it selects no lines, # which happens whenever stderr consists only of trace lines. Without it, # `set -e` would abort the script here, before it produces its report, diff --git a/tests/cronic-test/cronic-test.sh b/tests/cronic-test/cronic-test.sh index cb52265..e71ea75 100755 --- a/tests/cronic-test/cronic-test.sh +++ b/tests/cronic-test/cronic-test.sh @@ -82,27 +82,6 @@ exit "$1" EOF chmod +x "$work/make-noise-and-stderr" -# A command that writes, to stderr, lines that start with `+` but are not trace -# lines. -cat > "$work/plus-stderr" << 'EOF' -#!/bin/sh -echo "+++ b/file" 1>&2 -echo "++ conflict" 1>&2 -EOF -chmod +x "$work/plus-stderr" - -# Commands whose error output resembles trace lines for a PS4 with expansions. -cat > "$work/plus-colon-stderr" << 'EOF' -#!/bin/sh -echo "+++ conflict: in file.c" 1>&2 -EOF -chmod +x "$work/plus-colon-stderr" -cat > "$work/plus-space-stderr" << 'EOF' -#!/bin/sh -echo "+ error happened" 1>&2 -EOF -chmod +x "$work/plus-space-stderr" - # A bash command that reads an unset variable. cat > "$work/read-unset" << 'EOF' #!/bin/bash @@ -331,37 +310,6 @@ for name in $function_names; do done PATH="$saved_path" - -# Trace lines are recognized when PS4 contains regular-expression -# metacharacters or quotes. -PS4='[trace] ' -export PS4 -check "PS4 with metacharacters, trace-only stderr" 0 silent "$work/trace-only" 0 -check "PS4 with metacharacters, trace lines and real stderr" 0 \ - "report:^a real error$" "$work/trace-and-stderr" 0 -# Quotes in PS4 are literal. -PS4="+\"it's\" " -check "PS4 with quotes, trace-only stderr" 0 silent "$work/trace-only" 0 -# When PS4 contains an expansion or escape, no line is treated as a trace line, -# so trace lines are reported, and so is an error line that resembles one. -# shellcheck disable=SC2016 # The expansion is for the traced command to do. -PS4='+${LINENO}: ' -check "PS4 with expansions, trace-only stderr" 0 \ - "report:^+3: echo 'the standard output'$" "$work/trace-only" 0 -check "PS4 with expansions, error line that starts with +" 0 \ - "report:^+++ b/file$" "$work/plus-stderr" -# shellcheck disable=SC2016 # The expansion is for the traced command to do. -PS4='+${BASH_SOURCE}:${LINENO}: ' -check "PS4 with several expansions, error line that resembles a trace line" 0 \ - "report:^+++ conflict: in file.c$" "$work/plus-colon-stderr" -PS4='+\D{%H}: ' -check "PS4 with an escape, trace-only stderr" 0 report "$work/trace-only" 0 -# shellcheck disable=SC2016 # The expansion is for the traced command to do. -PS4='+$LINENO' -check "PS4 ending with an expansion, error line that starts with +" 0 \ - "report:^+ error happened$" "$work/plus-space-stderr" -unset PS4 - # The command may be a shell builtin. check "builtin command" 0 silent : # A builtin that would end the shell does not prevent the report. diff --git a/tests/cronic-test/temp-file-test.sh b/tests/cronic-test/temp-file-test.sh index f135101..2212266 100755 --- a/tests/cronic-test/temp-file-test.sh +++ b/tests/cronic-test/temp-file-test.sh @@ -2,12 +2,11 @@ # Tests how `cronic` creates and removes its temporary files. # -# Temporary file names built from the process id, such as `/tmp/cronic.out.$$`, -# would be unsafe: process ids are guessable and reused, so on a multi-user -# machine another user could pre-create those paths as symlinks and have the -# wrapped command's output written through them. Without a signal handler, a -# run that was interrupted before its final cleanup would leave its temporary -# files behind. +# `cronic` used to write to `/tmp/cronic.out.$$` and three sibling paths. +# Process ids are guessable and reused, so on a multi-user machine another user +# could pre-create those paths as symlinks and have the wrapped command's +# output written through them. `cronic` also had no signal handler, so a run +# that was interrupted before its final `rm` left all four files behind. set -eu @@ -42,75 +41,66 @@ temp_files() { find "$TMPDIR" -mindepth 1 -maxdepth 1 2> /dev/null | sort } -# Waits for the test to create the "release" file, so that the test can act -# while `cronic` is running the command rather than before or after. -cat > "$work/wait-for-release" << 'EOF' -#!/bin/sh -touch "$1" -while [ ! -e "$2" ]; do - sleep 0.1 -done -EOF -chmod +x "$work/wait-for-release" - -# wait_for_start FILE: waits until the wrapped command has created FILE. -wait_for_start() { - waited=0 - while [ ! -e "$1" ]; do - if [ "$waited" -ge 100 ]; then - echo "the wrapped command did not start" - exit 1 - fi - sleep 0.1 - waited=$((waited + 1)) - done -} - ### The temporary file names are not derived from the process id. -# A temporary file name built from `$$` would be predictable. `$$` is -# `cronic`'s process id even within its subshells, so `cronic`'s process id is -# the only one to check. While the wrapped command runs, look for that id in -# the names under the $TMPDIR that this test sets, and for the predictable names -# under /tmp. The test fails if `cronic` has no temporary files at that time, -# so that it cannot pass vacuously. -"$CRONIC" "$work/wait-for-release" "$work/started-1" "$work/release-1" \ - > "$work/output" 2>&1 & -cronic_pid=$! -wait_for_start "$work/started-1" -during="$(temp_files)" -predictable="$( - find "$TMPDIR" -name "*$cronic_pid*" - for file in "/tmp/cronic.out.$cronic_pid" "/tmp/cronic.err.$cronic_pid" \ - "/tmp/cronic.err.reduced.$cronic_pid" "/tmp/cronic.trace.$cronic_pid"; do +# The wrapped command's parent is `cronic` itself, so $PPID is the process id +# that the old names were built from. Check both the hard-coded /tmp that the +# old names used and the $TMPDIR that this test sets, so that reintroducing the +# predictable names under either directory is caught. +cat > "$work/report-ppid" << 'EOF' +#!/bin/sh +for dir in "/tmp" "${TMPDIR:-/tmp}"; do + for file in "$dir/cronic.out.$PPID" "$dir/cronic.err.$PPID" \ + "$dir/cronic.err.reduced.$PPID" "$dir/cronic.trace.$PPID"; do if [ -e "$file" ]; then - echo "$file" + echo "$file" >> "$1" fi done -)" -touch "$work/release-1" -wait "$cronic_pid" || fail "nonzero exit status: $(cat "$work/output")" -if [ -z "$during" ]; then - fail "cronic had no temporary files while the wrapped command ran" -elif [ -n "$predictable" ]; then +done +EOF +chmod +x "$work/report-ppid" + +"$CRONIC" "$work/report-ppid" "$work/predictable" > "$work/output" 2>&1 \ + || fail "nonzero exit status: $(cat "$work/output")" +if [ -e "$work/predictable" ]; then fail "temporary file names are predictable from the process id:" - echo "$predictable" + cat "$work/predictable" else pass "temporary file names are not predictable from the process id" fi ### An interrupted run leaves no temporary files behind. +# Waits for the test to create the "release" file, so that the signal arrives +# while `cronic` is running the command rather than before or after. +cat > "$work/wait-for-release" << 'EOF' +#!/bin/sh +touch "$1" +while [ ! -e "$2" ]; do + sleep 0.1 +done +EOF +chmod +x "$work/wait-for-release" + before="$(temp_files)" -"$CRONIC" "$work/wait-for-release" "$work/started-2" "$work/release-2" \ +"$CRONIC" "$work/wait-for-release" "$work/started" "$work/release" \ > "$work/output" 2>&1 & cronic_pid=$! -wait_for_start "$work/started-2" + +waited=0 +while [ ! -e "$work/started" ]; do + if [ "$waited" -ge 100 ]; then + echo "the wrapped command did not start" + exit 1 + fi + sleep 0.1 + waited=$((waited + 1)) +done # `cronic` is waiting for the wrapped command, so it handles the signal only # once that command has finished; release the command after signaling. kill -TERM "$cronic_pid" -touch "$work/release-2" +touch "$work/release" cronic_status=0 # Redirect stderr to discard the shell's "Terminated" job message, which would # otherwise look like a failure in the test output. From 73b1f464d93eac4b54e237616acf2ccb3a4519c0 Mon Sep 17 00:00:00 2001 From: Michael Ernst Date: Thu, 8 Oct 2026 20:32:24 -0700 Subject: [PATCH 21/28] Locate cronic's temporary files by their contents, not by the process id The wrapped command writes a unique marker, and the test checks that every file containing it is in a private `mktemp -d` directory under $TMPDIR. This detects any predictable naming scheme, avoids false matches of short process ids, reports unsafe files before the vacuity check, tolerates find errors, and lets the wrapped command exit if the test exits early. Co-Authored-By: Claude Opus 5.5 --- tests/cronic-test/temp-file-test.sh | 93 ++++++++++++++++++++--------- 1 file changed, 66 insertions(+), 27 deletions(-) diff --git a/tests/cronic-test/temp-file-test.sh b/tests/cronic-test/temp-file-test.sh index f135101..a15f955 100755 --- a/tests/cronic-test/temp-file-test.sh +++ b/tests/cronic-test/temp-file-test.sh @@ -43,11 +43,19 @@ temp_files() { } # Waits for the test to create the "release" file, so that the test can act -# while `cronic` is running the command rather than before or after. +# while `cronic` is running the command rather than before or after. If a third +# argument is given, writes it to stdout and stderr first, so that the test can +# find the files that `cronic` redirected them to. Gives up if the release +# file's directory disappears, as it does when the test exits early; otherwise +# this script and `cronic` would never terminate. cat > "$work/wait-for-release" << 'EOF' #!/bin/sh +if [ $# -ge 3 ]; then + echo "$3" + echo "$3" >&2 +fi touch "$1" -while [ ! -e "$2" ]; do +while [ ! -e "$2" ] && [ -d "$(dirname -- "$2")" ]; do sleep 0.1 done EOF @@ -66,37 +74,68 @@ wait_for_start() { done } -### The temporary file names are not derived from the process id. +### The temporary files are in a private directory with an unpredictable name. + +# A temporary file name built from `$$`, `$RANDOM`, the time, or the like would +# be predictable. Rather than guess how a name might be predictable, check +# where the wrapped command's output actually goes: the command writes a +# string that is unique to this test run, and the test looks for the files that +# contain it. Each such file must be directly within a directory that `mktemp +# -d` created in $TMPDIR: named `cronic.XXXXXX` and accessible only by its +# owner. Searching /tmp as well catches a `cronic` that ignores $TMPDIR. The +# test also fails if it finds no such files, so that it cannot pass vacuously. +marker="cronic temp-file-test marker $work" + +# marker_files: prints the files that contain the marker. Files directly +# within /tmp, or within a subdirectory of it, are searched, except for this +# test's own files. Errors, such as unreadable files, are ignored. +marker_files() { + { + find "$TMPDIR" -type f -exec grep -lF -- "$marker" {} + 2> /dev/null || true + find /tmp -maxdepth 2 -path "$work" -prune \ + -o -type f -user "$(id -u)" -exec grep -lF -- "$marker" {} + \ + 2> /dev/null || true + } | sort -u +} + +# in_private_dir FILE: succeeds if FILE is directly within a directory in +# $TMPDIR that is named like `mktemp`'s `cronic.XXXXXX` template and that is +# accessible only by its owner. +in_private_dir() { + dir="$(dirname -- "$1")" + [ "$(dirname -- "$dir")" = "$TMPDIR" ] || return 1 + case "$(basename -- "$dir")" in + cronic.??????) ;; + *) return 1 ;; + esac + [ -n "$(find "$dir" -prune -perm 700)" ] +} -# A temporary file name built from `$$` would be predictable. `$$` is -# `cronic`'s process id even within its subshells, so `cronic`'s process id is -# the only one to check. While the wrapped command runs, look for that id in -# the names under the $TMPDIR that this test sets, and for the predictable names -# under /tmp. The test fails if `cronic` has no temporary files at that time, -# so that it cannot pass vacuously. "$CRONIC" "$work/wait-for-release" "$work/started-1" "$work/release-1" \ - > "$work/output" 2>&1 & + "$marker" > "$work/output" 2>&1 & cronic_pid=$! wait_for_start "$work/started-1" -during="$(temp_files)" -predictable="$( - find "$TMPDIR" -name "*$cronic_pid*" - for file in "/tmp/cronic.out.$cronic_pid" "/tmp/cronic.err.$cronic_pid" \ - "/tmp/cronic.err.reduced.$cronic_pid" "/tmp/cronic.trace.$cronic_pid"; do - if [ -e "$file" ]; then - echo "$file" - fi - done -)" +found="$(marker_files)" +unsafe="" +while IFS= read -r file; do + if [ -n "$file" ] && ! in_private_dir "$file"; then + unsafe="$unsafe$file +" + fi +done << EOF +$found +EOF touch "$work/release-1" wait "$cronic_pid" || fail "nonzero exit status: $(cat "$work/output")" -if [ -z "$during" ]; then - fail "cronic had no temporary files while the wrapped command ran" -elif [ -n "$predictable" ]; then - fail "temporary file names are predictable from the process id:" - echo "$predictable" -else - pass "temporary file names are not predictable from the process id" +if [ -n "$unsafe" ]; then + fail "temporary files are not in a private directory created by mktemp:" + printf '%s' "$unsafe" +fi +if [ -z "$found" ]; then + fail "found no temporary files of cronic while the wrapped command ran" +fi +if [ -n "$found" ] && [ -z "$unsafe" ]; then + pass "temporary files are in a private directory created by mktemp" fi ### An interrupted run leaves no temporary files behind. From 931c4bfbb84591724ff461162d4323f9a26ab052 Mon Sep 17 00:00:00 2001 From: Michael Ernst Date: Thu, 8 Oct 2026 20:32:54 -0700 Subject: [PATCH 22/28] Use lowercase names for `cronic`'s variables, and test every variable it assigns Environment variables are uppercase by convention, so lowercase names cannot overwrite a caller's exported variable (including `CRONIC_*` ones). The test now derives the variable names from `cronic` itself, and unsets the exported variables after the check. --- cronic | 95 ++++++++++++++++---------------- tests/cronic-test/cronic-test.sh | 38 ++++++++++--- 2 files changed, 77 insertions(+), 56 deletions(-) diff --git a/cronic b/cronic index 87c8811..80f4975 100755 --- a/cronic +++ b/cronic @@ -19,10 +19,11 @@ # as an error. set -eu -# The names of this script's functions start with "cronic_", and the names of -# its variables start with "CRONIC_", so that they are unlikely to be the names -# of the caller's exported variables. An assignment to a variable that the -# caller exported would change the value that the wrapped command sees. +# The names of this script's variables are lowercase, because an assignment to +# a variable that the caller exported would change the value that the wrapped +# command sees, and by convention exported (environment) variables are +# uppercase. The names of this script's functions and variables also start +# with "cronic_", to further reduce the chance of a clash. # Prints the usage line. cronic_usage_line() { @@ -52,23 +53,23 @@ cronic_help() { exit 0 } -# Sets CRONIC_EXPECTED to its argument, or reports a usage error if the +# Sets cronic_expected to its argument, or reports a usage error if the # argument is not a valid exit status. cronic_set_expected() { - CRONIC_EXPECTED=$1 + cronic_expected=$1 # No leading zeros, because `[[ ... -eq ... ]]` would read the value as # octal. At most 255, because no exit status is larger. - if ! [[ $CRONIC_EXPECTED =~ ^(0|[1-9][0-9]{0,2})$ ]] || [ "$CRONIC_EXPECTED" -gt 255 ]; then - echo "$0: --expected-status requires an integer from 0 to 255, got \"$CRONIC_EXPECTED\"" >&2 + if ! [[ $cronic_expected =~ ^(0|[1-9][0-9]{0,2})$ ]] || [ "$cronic_expected" -gt 255 ]; then + echo "$0: --expected-status requires an integer from 0 to 255, got \"$cronic_expected\"" >&2 cronic_usage fi } # The options are parsed before the temporary directory is created, so that # `--help` and a usage error neither create nor depend on it. -CRONIC_EXPECTED=0 -CRONIC_PERMITSTDERR=0 -CRONIC_DUPLICATE_STDERR=0 +cronic_expected=0 +cronic_permitstderr=0 +cronic_duplicate_stderr=0 while [ $# -gt 0 ]; do case $1 in --expected-status) @@ -86,11 +87,11 @@ while [ $# -gt 0 ]; do ;; --permit-stderr) shift - CRONIC_PERMITSTDERR=1 + cronic_permitstderr=1 ;; --duplicate-stderr) shift - CRONIC_DUPLICATE_STDERR=1 + cronic_duplicate_stderr=1 ;; -h | --help) cronic_help @@ -110,26 +111,26 @@ while [ $# -gt 0 ]; do done [ $# -gt 0 ] || cronic_usage -CRONIC_DEBUG=false -# CRONIC_DEBUG=true +cronic_debug=false +# cronic_debug=true # A private directory, rather than predictable `/tmp/cronic.*.$$` names: on a # shared machine, process ids are guessable and reused, so another user could # pre-create those paths as symlinks and have the wrapped command's output # written through them. -CRONIC_TMPDIR=$(mktemp -d "${TMPDIR:-/tmp}/cronic.XXXXXX") || exit 1 -CRONIC_OUT=$CRONIC_TMPDIR/out -CRONIC_ERR=$CRONIC_TMPDIR/err -CRONIC_REDUCED_ERR=$CRONIC_TMPDIR/err.reduced -CRONIC_TRACE=$CRONIC_TMPDIR/trace +cronic_tmpdir=$(mktemp -d "${TMPDIR:-/tmp}/cronic.XXXXXX") || exit 1 +cronic_out=$cronic_tmpdir/out +cronic_err=$cronic_tmpdir/err +cronic_reduced_err=$cronic_tmpdir/err.reduced +cronic_trace=$cronic_tmpdir/trace # Remove the temporary files however this script terminates, including when it -# is interrupted before it reaches the end. When CRONIC_DEBUG is true, the +# is interrupted before it reaches the end. When cronic_debug is true, the # files are retained; don't "echo" their location, because that would be # appended to stderr. # shellcheck disable=SC2329 # the function is invoked from the trap handlers below. cronic_cleanup() { - [ "$CRONIC_DEBUG" = "true" ] || rm -rf "$CRONIC_TMPDIR" + [ "$cronic_debug" = "true" ] || rm -rf "$cronic_tmpdir" } trap cronic_cleanup EXIT @@ -149,66 +150,66 @@ set +e # `command` because the wrapped command runs in this shell, where a function of # this script, such as `cronic_usage`, would otherwise run in place of a # program of the same name. -command -- "$@" > "$CRONIC_OUT" 2> "$CRONIC_TRACE" -CRONIC_RESULT=$? +command -- "$@" > "$cronic_out" 2> "$cronic_trace" +cronic_result=$? set -e -CRONIC_PATTERN="^${PS4:0:1}\\+${PS4:1}" -if grep -q "$CRONIC_PATTERN" "$CRONIC_TRACE"; then +cronic_pattern="^${PS4:0:1}\\+${PS4:1}" +if grep -q "$cronic_pattern" "$cronic_trace"; then # `|| true` because `grep -v` exits with status 1 when it selects no lines, # which happens whenever stderr consists only of trace lines. Without it, # `set -e` would abort the script here, before it produces its report, # exits with the wrapped command's status, and removes its temporary files. - grep -v "$CRONIC_PATTERN" "$CRONIC_TRACE" > "$CRONIC_ERR" || true + grep -v "$cronic_pattern" "$cronic_trace" > "$cronic_err" || true else - CRONIC_ERR=$CRONIC_TRACE + cronic_err=$cronic_trace fi # Certain Unix programs send output to stderr even when a command is successful! # For example, git does this, and the POSIX standard requires it for make. -# The full stderr output is shown, but file CRONIC_REDUCED_ERR is checked for +# The full stderr output is shown, but file cronic_reduced_err is checked for # emptiness below. # TODO: Expand this. -cat "$CRONIC_ERR" \ +cat "$cronic_err" \ | grep -v "^Everything up-to-date$" \ | grep -v '^make\(\[[0-9]\+\]\)\?: \(Entering\|Leaving\) directory '"'"'.*'"'"'$' \ - > "$CRONIC_REDUCED_ERR" || true + > "$cronic_reduced_err" || true -if [ $CRONIC_RESULT -ne "$CRONIC_EXPECTED" ] || { [ $CRONIC_PERMITSTDERR -eq 0 ] && [ -s "$CRONIC_REDUCED_ERR" ]; }; then +if [ $cronic_result -ne "$cronic_expected" ] || { [ $cronic_permitstderr -eq 0 ] && [ -s "$cronic_reduced_err" ]; }; then echo "Cronic detected failure or error output for the command:" echo "$@" echo - if [[ $CRONIC_EXPECTED -eq 0 ]]; then - if [[ $CRONIC_RESULT -eq 0 ]]; then - echo "RESULT CODE: $CRONIC_RESULT (success)" + if [[ $cronic_expected -eq 0 ]]; then + if [[ $cronic_result -eq 0 ]]; then + echo "RESULT CODE: $cronic_result (success)" else - echo "RESULT CODE: $CRONIC_RESULT (failure)" + echo "RESULT CODE: $cronic_result (failure)" fi else - echo "RESULT CODE: $CRONIC_RESULT (expected: $CRONIC_EXPECTED)" + echo "RESULT CODE: $cronic_result (expected: $cronic_expected)" fi echo echo "ERROR OUTPUT:" - cat "$CRONIC_ERR" + cat "$cronic_err" echo - if [ "$CRONIC_DEBUG" != "true" ]; then - if ! cmp --silent "$CRONIC_ERR" "$CRONIC_REDUCED_ERR"; then + if [ "$cronic_debug" != "true" ]; then + if ! cmp --silent "$cronic_err" "$cronic_reduced_err"; then echo "REDUCED ERROR OUTPUT:" - cat "$CRONIC_REDUCED_ERR" + cat "$cronic_reduced_err" echo fi fi echo "STANDARD OUTPUT:" - cat "$CRONIC_OUT" - if [ "$CRONIC_TRACE" != "$CRONIC_ERR" ]; then + cat "$cronic_out" + if [ "$cronic_trace" != "$cronic_err" ]; then echo echo "TRACE-ERROR OUTPUT:" - cat "$CRONIC_TRACE" + cat "$cronic_trace" fi - if [ $CRONIC_DUPLICATE_STDERR -eq 1 ]; then + if [ $cronic_duplicate_stderr -eq 1 ]; then echo echo "ERROR OUTPUT:" - cat "$CRONIC_ERR" + cat "$cronic_err" fi echo echo "END OF CRONIC OUTPUT." @@ -216,4 +217,4 @@ fi # The temporary files are removed by the EXIT trap. -exit $CRONIC_RESULT +exit $cronic_result diff --git a/tests/cronic-test/cronic-test.sh b/tests/cronic-test/cronic-test.sh index af823fc..7ddedd5 100755 --- a/tests/cronic-test/cronic-test.sh +++ b/tests/cronic-test/cronic-test.sh @@ -245,18 +245,38 @@ for name in $function_names; do done PATH="$saved_path" -# `cronic` does not change a variable that the caller exported, even one whose -# name is a plain word such as `DEBUG` or `OUT`. +# `cronic` does not change a variable that the caller exported. For each +# variable that `cronic` assigns, the caller exports that name in uppercase, +# both with and without any "cronic_" prefix (for example, `CRONIC_OUT` and +# `OUT`). `TMPDIR` is omitted, because `cronic` reads it. +variable_names=$(sed -n 's/^ *\([A-Za-z_][A-Za-z0-9_]*\)=.*/\1/p' "$CRONIC" | sort -u) +if [ -z "$variable_names" ]; then + echo "FAIL: found no variables in $CRONIC" + status=1 +fi +exported_names= +for name in $variable_names; do + upper=$(echo "$name" | tr '[:lower:]' '[:upper:]') + for exported in "$upper" "${upper#CRONIC_}"; do + [ "$exported" = TMPDIR ] && continue + exported_names="$exported_names $exported" + done +done cat > "$work/check-variables" << 'EOF' #!/bin/sh -[ "$DEBUG" = "caller's DEBUG" ] && [ "$OUT" = "caller's OUT" ] \ - && [ "$RESULT" = "caller's RESULT" ] || exit 3 +for name; do + eval "value=\${$name-}" + [ "$value" = "caller's $name" ] || exit 3 +done EOF chmod +x "$work/check-variables" -DEBUG="caller's DEBUG" -OUT="caller's OUT" -RESULT="caller's RESULT" -export DEBUG OUT RESULT -check "caller's exported variables" 0 silent "$work/check-variables" +for name in $exported_names; do + eval "$name=\"caller's \$name\"" + export "${name?}" +done +# shellcheck disable=SC2086 # each name is a separate argument. +check "caller's exported variables" 0 silent "$work/check-variables" $exported_names +# shellcheck disable=SC2086 # each name is a separate argument. +unset $exported_names exit "$status" From 391b40091642ab86bde5168a7c53cddbf6768bab Mon Sep 17 00:00:00 2001 From: Michael Ernst Date: Thu, 8 Oct 2026 20:39:54 -0700 Subject: [PATCH 23/28] Brevity --- cronic | 6 ------ 1 file changed, 6 deletions(-) diff --git a/cronic b/cronic index 80f4975..7b224bc 100755 --- a/cronic +++ b/cronic @@ -19,12 +19,6 @@ # as an error. set -eu -# The names of this script's variables are lowercase, because an assignment to -# a variable that the caller exported would change the value that the wrapped -# command sees, and by convention exported (environment) variables are -# uppercase. The names of this script's functions and variables also start -# with "cronic_", to further reduce the chance of a clash. - # Prints the usage line. cronic_usage_line() { echo "Usage: $0 [--expected-status N] [--permit-stderr] [--duplicate-stderr] [--] COMMAND [ARG...]" From 0c3b54069adc7b755e2ce16d680a7901d1eef2b2 Mon Sep 17 00:00:00 2001 From: Michael Ernst Date: Thu, 8 Oct 2026 20:42:59 -0700 Subject: [PATCH 24/28] Clarify what the exported-variables test of `cronic` checks --- tests/cronic-test/cronic-test.sh | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/tests/cronic-test/cronic-test.sh b/tests/cronic-test/cronic-test.sh index 7ddedd5..42048f3 100755 --- a/tests/cronic-test/cronic-test.sh +++ b/tests/cronic-test/cronic-test.sh @@ -245,10 +245,13 @@ for name in $function_names; do done PATH="$saved_path" -# `cronic` does not change a variable that the caller exported. For each -# variable that `cronic` assigns, the caller exports that name in uppercase, -# both with and without any "cronic_" prefix (for example, `CRONIC_OUT` and -# `OUT`). `TMPDIR` is omitted, because `cronic` reads it. +# The wrapped command sees the caller's exported variables unchanged. For +# each variable that `cronic` assigns, the caller exports that name in +# uppercase, both with and without any "cronic_" prefix (for example, +# `CRONIC_OUT` and `OUT`). `TMPDIR` is omitted, because `cronic` reads it. +# The lowercase names that `cronic` assigns, such as `cronic_out`, are not +# exported: `cronic` does overwrite them, but environment variables +# conventionally have uppercase names. variable_names=$(sed -n 's/^ *\([A-Za-z_][A-Za-z0-9_]*\)=.*/\1/p' "$CRONIC" | sort -u) if [ -z "$variable_names" ]; then echo "FAIL: found no variables in $CRONIC" From 63cea8cda5a0855589813593a2ed36728140e232 Mon Sep 17 00:00:00 2001 From: Michael Ernst Date: Thu, 8 Oct 2026 20:48:27 -0700 Subject: [PATCH 25/28] Separate bash execution traces from error output via BASH_XTRACEFD Bash 4.1 or later writes its trace to a separate file, which the report shows under "TRACE OUTPUT". For other shells, trace lines are still removed from stderr by PS4; a multibyte first character of PS4 is repeated as a whole, and a PS4 containing a newline is not used for filtering. --- cronic | 81 ++++++++++++++++++++++-------- tests/cronic-test/cronic-test.sh | 85 +++++++++++++++++++++++--------- 2 files changed, 123 insertions(+), 43 deletions(-) diff --git a/cronic b/cronic index b96ac5a..b26d786 100755 --- a/cronic +++ b/cronic @@ -6,15 +6,14 @@ # In brief: # Cronic is a small shim shell script for wrapping cron jobs so that cron # only sends email when an error has occurred. Cronic defines an error as -# any non-trace error output or a non-zero result code. Cronic filters -# Bash execution traces (or anything matching PS4) from the error output, -# so jobs can be run with execution tracing to aid forensic -# debugging. Apart from its options, Cronic simply executes its -# arguments. +# any non-trace error output or a non-zero result code. Cronic separates +# execution traces from the error output, so jobs can be run with execution +# tracing to aid forensic debugging. Apart from its options, Cronic simply +# executes its arguments. # Extended by Michael Ernst to add the --expected-status, --permit-stderr, and # --duplicate-stderr options, which may be given in any order. -# The options are documented in function `cronic_help`; run `cronic --help`. -# Trace lines are filtered only if PS4 contains no `$`, backquote, or backslash. +# The options and the treatment of execution traces are documented in function +# `cronic_help`; run `cronic --help`. # Exit if a command exits with a non-zero status, and treat unset variables # as an error. @@ -45,6 +44,12 @@ cronic_help() { echo " is less likely to be overlooked" echo " -- end the options, so that COMMAND may start with \"-\"" echo " -h, --help print this message" + echo "Execution traces (from \"set -x\") are not treated as error output." + echo "Bash 4.1 or later writes them to a separate file, via BASH_XTRACEFD; they" + echo "appear in the report under \"TRACE OUTPUT\". Other shells write them to" + echo "stderr, from which lines that start with PS4 are removed. That removal" + echo "is skipped if PS4 contains \"\$\", a backquote, a backslash, or a newline," + echo "because such a PS4 can expand differently on each line." exit 0 } @@ -118,6 +123,7 @@ cronic_out=$cronic_tmpdir/out cronic_err=$cronic_tmpdir/err cronic_reduced_err=$cronic_tmpdir/err.reduced cronic_trace=$cronic_tmpdir/trace +cronic_xtrace=$cronic_tmpdir/xtrace # Remove the temporary files however this script terminates, including when it # is interrupted before it reaches the end. When cronic_debug is true, the @@ -141,6 +147,17 @@ trap 'cronic_cleanup; trap - HUP EXIT; kill -s HUP $$' HUP trap 'cronic_cleanup; trap - INT EXIT; kill -s INT $$' INT trap 'cronic_cleanup; trap - TERM EXIT; kill -s TERM $$' TERM +# A bash command writes its execution trace to the file descriptor named by +# BASH_XTRACEFD, rather than to stderr, so the trace is separated from the +# error output exactly, whatever PS4 is. The descriptor is allocated by bash +# (as 10 or higher), so that it does not collide with a descriptor, such as 3, +# that the wrapped command uses for its own purposes. Bash before 4.1 supports +# neither BASH_XTRACEFD nor descriptor allocation. +if ((BASH_VERSINFO[0] > 4 || (BASH_VERSINFO[0] == 4 && BASH_VERSINFO[1] >= 1))); then + exec {cronic_xtrace_fd}> "$cronic_xtrace" + export BASH_XTRACEFD=$cronic_xtrace_fd +fi + set +e # `command` because the wrapped command runs in this shell, where a function of # this script, such as `cronic_usage`, would otherwise run in place of a @@ -149,20 +166,37 @@ command -- "$@" > "$cronic_out" 2> "$cronic_trace" cronic_result=$? set -e -# A trace line starts with PS4 after expansion, with PS4's first character -# repeated once per level of nesting. If PS4 contains a `$`, backquote, or -# backslash, then its expansion can differ from line to line, so no line is -# treated as a trace line: any pattern for such a PS4 could also match, and -# hide, a real error. -# The `sed` command escapes PS4's basic-regular-expression metacharacters, then -# makes the first (possibly escaped) character repeatable. -cronic_pattern= -case ${PS4-} in - '' | *[\$\`\\]*) ;; - *) - cronic_pattern=^$(printf '%s\n' "$PS4" | sed 's/[*.[^$]/\\&/g; s/^\\\{0,1\}./&\\+/') - ;; -esac +# Unsetting BASH_XTRACEFD also closes its file descriptor. +unset BASH_XTRACEFD + +# Prints a basic regular expression that matches its argument literally. +# The argument must not contain a backslash. +cronic_bre_quote() { + printf '%s\n' "$1" | LC_ALL=C sed 's/[*.[^]/\\&/g' +} + +# Prints a basic regular expression that matches the trace lines, for PS4, that +# a shell other than bash 4.1 or later writes to stderr. Such a line starts +# with PS4 after expansion, with PS4's first character repeated once per level +# of nesting. If PS4 contains a `$`, backquote, or backslash, then its +# expansion can differ from line to line, so nothing is printed: any pattern +# for such a PS4 could also match, and hide, a real error. Nothing is printed +# if PS4 contains a newline, either, because `grep` would treat each line of +# the pattern as a separate pattern. +cronic_trace_pattern() { + case ${PS4-} in + '' | *[\$\`\\]* | *$'\n'*) return ;; + esac + # In the C locale, a character is a byte. The first character is a UTF-8 + # lead byte and its continuation bytes, or else a single byte, so that a + # multibyte character is repeated as a whole, whatever the locale. + local LC_ALL=C + [[ $PS4 =~ ^([$'\xc0'-$'\xff'][$'\x80'-$'\xbf']*|.) ]] + local first=${BASH_REMATCH[0]} + printf '^\\(%s\\)\\+%s\n' "$(cronic_bre_quote "$first")" "$(cronic_bre_quote "${PS4#"$first"}")" +} + +cronic_pattern=$(cronic_trace_pattern) if [ -n "$cronic_pattern" ] && grep -q "$cronic_pattern" "$cronic_trace"; then # `|| true` because `grep -v` exits with status 1 when it selects no lines, # which happens whenever stderr consists only of trace lines. Without it, @@ -214,6 +248,11 @@ if [ $cronic_result -ne "$cronic_expected" ] || { [ $cronic_permitstderr -eq 0 ] echo "TRACE-ERROR OUTPUT:" cat "$cronic_trace" fi + if [ -s "$cronic_xtrace" ]; then + echo + echo "TRACE OUTPUT:" + cat "$cronic_xtrace" + fi if [ $cronic_duplicate_stderr -eq 1 ]; then echo echo "ERROR OUTPUT:" diff --git a/tests/cronic-test/cronic-test.sh b/tests/cronic-test/cronic-test.sh index 8449e42..7b62509 100755 --- a/tests/cronic-test/cronic-test.sh +++ b/tests/cronic-test/cronic-test.sh @@ -79,17 +79,13 @@ echo "++ conflict" 1>&2 EOF chmod +x "$work/plus-stderr" -# Commands whose error output resembles trace lines for a PS4 with expansions. -cat > "$work/plus-colon-stderr" << 'EOF' +# A command that writes each of its arguments, as a line, to stderr. It stands +# for a shell other than bash, which writes its trace lines to stderr. +cat > "$work/stderr-lines" << 'EOF' #!/bin/sh -echo "+++ conflict: in file.c" 1>&2 +printf '%s\n' "$@" 1>&2 EOF -chmod +x "$work/plus-colon-stderr" -cat > "$work/plus-space-stderr" << 'EOF' -#!/bin/sh -echo "+ error happened" 1>&2 -EOF -chmod +x "$work/plus-space-stderr" +chmod +x "$work/stderr-lines" # temp_files: prints `cronic`'s temporary files, in a canonical order. temp_files() { @@ -303,34 +299,79 @@ check "caller's exported variables" 0 silent "$work/check-variables" $exported_n # shellcheck disable=SC2086 # each name is a separate argument. unset $exported_names +# Bash writes its trace lines to a separate file, so they are not error +# output, whatever PS4 is. +export PS4 +# shellcheck disable=SC2016 # The expansion is for the traced command to do. +PS4='+${LINENO}: ' +check "bash, PS4 with expansions, trace-only stderr" 0 silent \ + "$work/trace-only" 0 +check "bash, PS4 with expansions, trace in the report" 3 \ + "report:echo 'the standard output'$" "$work/trace-only" 3 +check "bash, PS4 with expansions, trace lines and real stderr" 0 \ + "report:^a real error$" "$work/trace-and-stderr" 0 +PS4='+\D{%H}: ' +check "bash, PS4 with an escape, trace-only stderr" 0 silent \ + "$work/trace-only" 0 +PS4='[trace] ' +check "bash, PS4 with metacharacters, trace-only stderr" 0 silent \ + "$work/trace-only" 0 + +# The tests below concern trace lines that a shell other than bash writes to +# stderr; they are recognized by PS4. Bash running as root does not import +# PS4 from the environment, so `cronic` would see the default PS4, and these +# tests are skipped. +if [ "$(id -u)" -eq 0 ]; then + echo "SKIP: tests of PS4 in stderr, because bash ignores PS4 when run as root" + unset PS4 + exit "$status" +fi + # Trace lines are recognized when PS4 contains regular-expression -# metacharacters or quotes. +# metacharacters or quotes, and the metacharacters do not match other lines. PS4='[trace] ' -export PS4 -check "PS4 with metacharacters, trace-only stderr" 0 silent "$work/trace-only" 0 +check "PS4 with metacharacters, trace-only stderr" 0 silent \ + "$work/stderr-lines" "[trace] echo hi" "[[trace] echo nested" check "PS4 with metacharacters, trace lines and real stderr" 0 \ - "report:^a real error$" "$work/trace-and-stderr" 0 -# Quotes in PS4 are literal. + "report:^a real error$" \ + "$work/stderr-lines" "[trace] echo hi" "a real error" +PS4='+.* ' +check "PS4 with metacharacters, error line that they would match" 0 \ + "report:^+++ b/file$" "$work/stderr-lines" "+++ b/file" PS4="+\"it's\" " -check "PS4 with quotes, trace-only stderr" 0 silent "$work/trace-only" 0 -# When PS4 contains an expansion or escape, no line is treated as a trace line, -# so trace lines are reported, and so is an error line that resembles one. +check "PS4 with quotes, trace-only stderr" 0 silent \ + "$work/stderr-lines" "+\"it's\" echo hi" "++\"it's\" echo nested" +# A multibyte first character is repeated as a whole, even in the C locale. +PS4='→ ' +LC_ALL=C +export LC_ALL +check "PS4 with a multibyte first character, trace-only stderr" 0 silent \ + "$work/stderr-lines" "→ echo hi" "→→ echo nested" +unset LC_ALL +# When PS4 contains an expansion, an escape, or a newline, no line is treated +# as a trace line, so trace lines are reported, and so is an error line that +# resembles one. # shellcheck disable=SC2016 # The expansion is for the traced command to do. PS4='+${LINENO}: ' check "PS4 with expansions, trace-only stderr" 0 \ - "report:^+3: echo 'the standard output'$" "$work/trace-only" 0 + "report:^+3: echo hi$" "$work/stderr-lines" "+3: echo hi" check "PS4 with expansions, error line that starts with +" 0 \ - "report:^+++ b/file$" "$work/plus-stderr" + "report:^+++ b/file$" "$work/stderr-lines" "+++ b/file" # shellcheck disable=SC2016 # The expansion is for the traced command to do. PS4='+${BASH_SOURCE}:${LINENO}: ' check "PS4 with several expansions, error line that resembles a trace line" 0 \ - "report:^+++ conflict: in file.c$" "$work/plus-colon-stderr" + "report:^+++ conflict: in file.c$" "$work/stderr-lines" "+++ conflict: in file.c" PS4='+\D{%H}: ' -check "PS4 with an escape, trace-only stderr" 0 report "$work/trace-only" 0 +check "PS4 with an escape, trace-only stderr" 0 \ + "report:^+12: echo hi$" "$work/stderr-lines" "+12: echo hi" # shellcheck disable=SC2016 # The expansion is for the traced command to do. PS4='+$LINENO' check "PS4 ending with an expansion, error line that starts with +" 0 \ - "report:^+ error happened$" "$work/plus-space-stderr" + "report:^+ error happened$" "$work/stderr-lines" "+ error happened" +PS4='a +b ' +check "PS4 with a newline, error line that contains its second line" 0 \ + "report:^error: bad b input$" "$work/stderr-lines" "error: bad b input" unset PS4 exit "$status" From 7476e3b92ed981ae3b2c45b2d3d487ca651d2cb0 Mon Sep 17 00:00:00 2001 From: Michael Ernst Date: Thu, 8 Oct 2026 21:08:10 -0700 Subject: [PATCH 26/28] Restore only the caller's options; reserve cronic_ names; call builtins via builtin --- cronic | 117 +++++++++++------------ tests/cronic-test/cronic-test.sh | 159 ++++++++++++++++--------------- 2 files changed, 135 insertions(+), 141 deletions(-) diff --git a/cronic b/cronic index dd82bb2..5b0918a 100755 --- a/cronic +++ b/cronic @@ -14,21 +14,22 @@ # --duplicate-stderr options, which may be given in any order. # The options and the treatment of execution traces are documented in function # `cronic_help`; run `cronic --help`. -# COMMAND runs in a subshell, with the caller's variables and shell options -# rather than this script's. -# This script runs under bash 3.2, which is /bin/bash on macOS, so it does not -# use later features such as `${name@a}`. +# COMMAND runs in a subshell, with the caller's shell options rather than this +# script's. Names that start with `cronic_` are reserved for this script: +# COMMAND does not see a variable or function with such a name, even one that +# the caller exported. +# This script runs under bash 3.2, which is /bin/bash on macOS, so it uses a +# feature of a later bash only after checking the version. -# The caller's state, as commands that restore it, before this script changes -# any of it for itself. The wrapped command runs with the caller's state -# rather than this script's; see where it is run, below. -# * The caller's exported variables and functions whose names start with -# `cronic_`, which this script's own variables and functions would otherwise -# hide. -# * The caller's `set -o` shell options. `set +o` reports `-e` and verbose as -# off, because bash turns them off in a command substitution, so `$-` -# supplies them. This script changes no `shopt` option, so the command -# inherits the caller's `shopt` options without any restoring. +# The caller's shell options, as a command that restores them, before this +# script changes them for itself. This script changes only errexit, nounset, +# verbose, and xtrace, so the command inherits the caller's other `set -o` +# options, and its `shopt` options, without any restoring; an option that this +# script changes must be added here. `$-` reports the options without the +# cost of a command substitution, such as `$(set +o)`, in which bash would also +# report errexit as off. The options are +# restored in one command, so that restoring verbose and xtrace neither echoes +# nor traces the restoring of the others. # If the caller exported SHELLOPTS, then this script does not trace itself or # echo its input: that output would go to this script's stderr, and cron would # send email for every run. An exported `verbose` nonetheless echoes the lines @@ -36,45 +37,27 @@ # tell whether xtrace came from SHELLOPTS or from the command line, so # `bash -x cronic` traces this script only if SHELLOPTS is not exported. # The redirection discards the trace of these commands. +# Builtins are called via `builtin`, here and where the caller's options are +# restored, so that a function that the caller exported with the name of a +# builtin, such as `set` or `unset`, does not run in its place. { - # `compgen -e` lists exported variables, and fails if there are none, in which - # case `declare -p` with no names would print every variable. The `if` makes - # the status 0, as `errexit` requires. `declare -F` marks an exported - # function with `x`. The command substitution keeps `names`, `flags`, and - # `name` out of this script. - cronic_caller_variables=$( - if names=$(compgen -e cronic_); then - # shellcheck disable=SC2086 # Each name is one word. - declare -p $names - fi - declare -F | while read -r _ flags name; do - case "$flags $name" in - -*x*' 'cronic_*) - declare -f "$name" - echo "export -f $name" - ;; - esac - done - ) - # `$-` supplies errexit, verbose, and xtrace. They come last, in one - # command, so that restoring verbose and xtrace neither echoes nor traces the - # restoring of any other option. - cronic_caller_options=$(set +o) - cronic_caller_set='set' - for cronic_option in e:errexit v:verbose x:xtrace; do - cronic_caller_options=${cronic_caller_options/$'\n'set [-+]o ${cronic_option#*:}/} + cronic_caller_options='builtin set' + for cronic_option in e:errexit u:nounset v:verbose x:xtrace; do case $- in - *"${cronic_option%%:*}"*) cronic_caller_set+=" -o ${cronic_option#*:}" ;; - *) cronic_caller_set+=" +o ${cronic_option#*:}" ;; + *"${cronic_option%%:*}"*) cronic_caller_options+=" -o ${cronic_option#*:}" ;; + *) cronic_caller_options+=" +o ${cronic_option#*:}" ;; esac done - cronic_caller_options+=$'\n'$cronic_caller_set - if [[ $(declare -p SHELLOPTS) =~ ^declare\ -[a-zA-Z]*x ]]; then set +vx; fi + # The filter makes `compgen` match SHELLOPTS exactly, not every exported + # variable whose name starts with SHELLOPTS. + if builtin compgen -e -X '!SHELLOPTS' SHELLOPTS > /dev/null; then + builtin set +vx + fi } 2> /dev/null # Exit if a command exits with a non-zero status, and treat unset variables # as an error. -set -eu +builtin set -eu # Prints the usage line. cronic_usage_line() { @@ -101,6 +84,8 @@ cronic_help() { echo " is less likely to be overlooked" echo " -- end the options, so that COMMAND may start with \"-\"" echo " -h, --help print this message" + echo "Names that start with \"cronic_\" are reserved: COMMAND does not see a" + echo "variable or function with such a name, even one that the caller exported." echo "Execution traces (from \"set -x\") are not treated as error output." echo "Bash 4.1 or later writes them to a separate file, via BASH_XTRACEFD; they" echo "appear in the report under \"TRACE OUTPUT\". Other shells write them to" @@ -231,27 +216,35 @@ fi # when `-x` is set, the trace of running the command goes into the command's # trace output rather than to this script's stderr. # The subshell unsets every variable whose name starts with `cronic_` and every -# function whose name starts with `cronic_`, so that a builtin or shell function -# that is the command does not see this script's variables and functions, and -# so that a function does not take precedence over a program of the same name. -# Then the subshell restores the caller's variables and options. The subshell -# first turns off this script's xtrace and verbose, so that the restoring is not -# traced or echoed; any error in the restoring goes to the command's error -# output. -set +e +# function that this script defined, so that a builtin or shell function that +# is the command does not see this script's variables and functions, and so +# that a function does not take precedence over a program of the same name. +# This script's functions are listed, rather than found by +# `compgen -A function cronic_`, to avoid the cost of a command substitution; +# the test suite checks that the list is complete. A command substitution +# finds any other such function, such as one that the caller exported, but +# only when the status of `compgen` shows that there is one. +# Then the subshell restores the caller's options. The subshell first turns +# off this script's xtrace and verbose, so that the restoring is not traced or +# echoed; any error in the restoring goes to the command's error output. +builtin set +e ( - { set +vx; } 2> /dev/null - if [ -n "$cronic_xtrace_fd" ]; then - export BASH_XTRACEFD=$cronic_xtrace_fd + { builtin set +vx; } 2> /dev/null + if [[ -n $cronic_xtrace_fd ]]; then + builtin export BASH_XTRACEFD=$cronic_xtrace_fd + fi + builtin unset -f cronic_usage_line cronic_usage cronic_help \ + cronic_set_expected cronic_cleanup + if builtin compgen -A function cronic_ > /dev/null; then + # shellcheck disable=SC2046 # Each name is one word. + builtin unset -f $(builtin compgen -A function cronic_) fi - eval "unset -v \"\${!cronic_@}\" - unset -f \$(compgen -A function cronic_) - $cronic_caller_variables + builtin eval "builtin unset -v \"\${!cronic_@}\" $cronic_caller_options" "$@" ) > "$cronic_out" 2> "$cronic_trace" cronic_result=$? -set -e +builtin set -e if [ -n "$cronic_xtrace_fd" ]; then exec {cronic_xtrace_fd}>&- @@ -260,7 +253,7 @@ fi # Prints a basic regular expression that matches its argument literally. # The argument must not contain a backslash. cronic_bre_quote() { - printf '%s\n' "$1" | LC_ALL=C sed 's/[*.[^]/\\&/g' + builtin printf '%s\n' "$1" | LC_ALL=C sed 's/[*.[^]/\\&/g' } # Prints a basic regular expression that matches the trace lines, for PS4, that @@ -281,7 +274,7 @@ cronic_trace_pattern() { local LC_ALL=C [[ $PS4 =~ ^([$'\xc0'-$'\xff'][$'\x80'-$'\xbf']*|.) ]] local first=${BASH_REMATCH[0]} - printf '^\\(%s\\)\\+%s\n' "$(cronic_bre_quote "$first")" "$(cronic_bre_quote "${PS4#"$first"}")" + builtin printf '^\\(%s\\)\\+%s\n' "$(cronic_bre_quote "$first")" "$(cronic_bre_quote "${PS4#"$first"}")" } cronic_pattern=$(cronic_trace_pattern) diff --git a/tests/cronic-test/cronic-test.sh b/tests/cronic-test/cronic-test.sh index 20d6c2c..0d7f832 100755 --- a/tests/cronic-test/cronic-test.sh +++ b/tests/cronic-test/cronic-test.sh @@ -116,22 +116,13 @@ exit "$1" EOF chmod +x "$work/ignore-failure" -# Runs `cronic` with SHELLOPTS set to its first argument and exported. -cat > "$work/cronic-with-shellopts" << 'EOF' -#!/bin/sh -shellopts="$1" -shift -exec env SHELLOPTS="$shellopts" "$REAL_CRONIC" "$@" -EOF -chmod +x "$work/cronic-with-shellopts" - # Runs `cronic` with SHELLOPTS set to its first argument and exported, and # with these exported shell functions: # * `fail_then_continue` runs `false` and then prints a line. # * `fail_in_pipeline` runs a pipeline whose first command fails. # * `report_cronic_variables` writes to stderr any variable or function of # `cronic`'s that it can see. -cat > "$work/cronic-with-function" << 'EOF2' +cat > "$work/cronic-with-shellopts" << 'EOF2' #!/bin/bash fail_then_continue() { false @@ -150,13 +141,62 @@ shellopts="$1" shift exec env SHELLOPTS="$shellopts" "$REAL_CRONIC" "$@" EOF2 -chmod +x "$work/cronic-with-function" +chmod +x "$work/cronic-with-shellopts" + +# Runs `cronic` with exported shell functions named like the builtins that +# `cronic` uses to restore the caller's options. Each function writes to +# stderr and then runs the builtin. +cat > "$work/cronic-with-builtin-functions" << 'EOF' +#!/bin/bash +for name in compgen eval export printf set unset; do + builtin eval "$name() { + echo \"the function $name ran\" >&2 + builtin $name \"\$@\" + }" + builtin export -f "$name" +done +exec "$REAL_CRONIC" "$@" +EOF +chmod +x "$work/cronic-with-builtin-functions" # temp_files: prints `cronic`'s temporary files, in a canonical order. temp_files() { find "$TMPDIR" -mindepth 1 -maxdepth 1 2> /dev/null | sort } +# check_trace_section DESCRIPTION GOAL-FILE COMMAND...: runs COMMAND, which +# runs `cronic`, and checks that it exits with status 0, leaves no temporary +# files, and produces a report whose "TRACE OUTPUT" section, through the +# following empty line, is the contents of GOAL-FILE. COMMAND's stderr is left +# in "$work/stderr". +check_trace_section() { + description="$1" + goal="$2" + shift 2 + + before="$(temp_files)" + actual_status=0 + "$@" > "$work/output" 2> "$work/stderr" || actual_status=$? + after="$(temp_files)" + if [ "$actual_status" != 0 ]; then + echo "FAIL: $description: exit status $actual_status, expected 0" + status=1 + fi + if [ "$before" != "$after" ]; then + echo "FAIL: $description: temporary files were left behind:" + echo "$after" + status=1 + fi + sed -n '/^TRACE OUTPUT:$/,/^$/p' "$work/output" > "$work/trace-section" + if cmp -s "$goal" "$work/trace-section"; then + echo "PASS: $description" + else + echo "FAIL: $description: got:" + cat "$work/output" + status=1 + fi +} + # check DESCRIPTION EXPECTED-STATUS EXPECTED-OUTPUT COMMAND...: runs `cronic` # on COMMAND and checks its exit status, whether it printed a report, and that # it left no temporary files behind. EXPECTED-OUTPUT is "silent", "report", @@ -370,26 +410,6 @@ check "exported SHELLOPTS with xtrace, trace-only stderr" 0 silent \ # command, followed by the command's own trace, with nothing that `cronic` did # to set up the command's options. Bash writes the trace via BASH_XTRACEFD, so # it appears under "TRACE OUTPUT". -before="$(temp_files)" -xtrace_status=0 -"$CRONIC" "$default_shellopts:xtrace" "$work/trace-and-stderr" 0 \ - > "$work/output" 2> "$work/stderr" || xtrace_status=$? -after="$(temp_files)" -if [ "$xtrace_status" != 0 ]; then - echo "FAIL: exported SHELLOPTS with xtrace: exit status $xtrace_status, expected 0" - status=1 -fi -if [ -s "$work/stderr" ]; then - echo "FAIL: exported SHELLOPTS with xtrace: cronic wrote to stderr:" - cat "$work/stderr" - status=1 -fi -if [ "$before" != "$after" ]; then - echo "FAIL: exported SHELLOPTS with xtrace: temporary files were left behind:" - echo "$after" - status=1 -fi -sed -n '/^TRACE OUTPUT:$/,/^$/p' "$work/output" > "$work/trace-section" cat > "$work/trace-section.goal" << EOF TRACE OUTPUT: + $work/trace-and-stderr 0 @@ -398,11 +418,12 @@ TRACE OUTPUT: + exit 0 EOF -if cmp -s "$work/trace-section.goal" "$work/trace-section"; then - echo "PASS: exported SHELLOPTS with xtrace, trace output" -else - echo "FAIL: exported SHELLOPTS with xtrace, trace output: got:" - cat "$work/output" +check_trace_section "exported SHELLOPTS with xtrace, trace output" \ + "$work/trace-section.goal" \ + "$CRONIC" "$default_shellopts:xtrace" "$work/trace-and-stderr" 0 +if [ -s "$work/stderr" ]; then + echo "FAIL: exported SHELLOPTS with xtrace: cronic wrote to stderr:" + cat "$work/stderr" status=1 fi @@ -422,7 +443,6 @@ fi # The caller's `-e` applies within an exported shell function that is the # command, and the caller's lack of `-e` does too. -CRONIC="$work/cronic-with-function" check "exported SHELLOPTS with errexit, exported function ignores a failure" 1 \ report "$default_shellopts:errexit" fail_then_continue check "exported SHELLOPTS, exported function ignores a failure" 0 silent \ @@ -482,43 +502,25 @@ check "caller's exported variables" 0 silent "$work/check-variables" $exported_n # shellcheck disable=SC2086 # each name is a separate argument. unset $exported_names -# The command sees a variable that the caller exported even if its name is that -# of one of `cronic`'s own variables, though that is unconventional. -cat > "$work/check-cronic-variables" << 'EOF' -#!/bin/sh -[ "$cronic_debug" = "caller's cronic_debug" ] \ - && [ "$cronic_tmpdir" = "caller's cronic_tmpdir" ] || exit 3 -EOF -chmod +x "$work/check-cronic-variables" -cronic_debug="caller's cronic_debug" -cronic_tmpdir="caller's cronic_tmpdir" -export cronic_debug cronic_tmpdir -check "caller's exported variables named like cronic's" 0 silent \ - "$work/check-cronic-variables" -unset cronic_debug cronic_tmpdir - -# `cronic` does not remove a function that the caller exported, whether its -# name is one of `cronic`'s own functions or merely starts like them. The -# function is visible both to the command and to the command's children. -cat > "$work/cronic-with-cronic-functions" << 'EOF' +# Names that start with `cronic_` are reserved: the command does not see a +# variable or function with such a name that the caller exported. +cat > "$work/cronic-with-cronic-names" << 'EOF' #!/bin/bash cronic_mine() { exit 3 } -cronic_usage() { - exit 4 -} -export -f cronic_mine cronic_usage -exec "$1" "${@:2}" +cronic_debug="caller's cronic_debug" +export -f cronic_mine +export cronic_debug +exec "$REAL_CRONIC" "$@" EOF -chmod +x "$work/cronic-with-cronic-functions" -CRONIC="$work/cronic-with-cronic-functions" -check "caller's exported function named cronic_mine" 3 silent \ - "$REAL_CRONIC" --expected-status 3 cronic_mine -check "caller's exported function named cronic_mine, in a child" 3 silent \ - "$REAL_CRONIC" --expected-status 3 bash -c cronic_mine -check "caller's exported function named cronic_usage" 4 silent \ - "$REAL_CRONIC" --expected-status 4 cronic_usage +chmod +x "$work/cronic-with-cronic-names" +CRONIC="$work/cronic-with-cronic-names" +# shellcheck disable=SC2016 # The expansion is for `sh` to do. +check "caller's exported cronic_ variable is not seen" 0 silent \ + sh -c '[ -z "${cronic_debug+set}" ]' +check "caller's exported cronic_ function is not seen" 0 silent \ + bash -c '! declare -F cronic_mine' CRONIC="$REAL_CRONIC" # `bash -x cronic`, which does not export SHELLOPTS, traces `cronic` itself. @@ -536,9 +538,6 @@ fi # Under `bash -x cronic`, the report's trace section contains nothing that # `cronic` did to set up the command. The command does not inherit xtrace, so # its own `set -x` is not traced. -bash -x "$CRONIC" "$work/trace-and-stderr" 0 > "$work/output" 2> /dev/null \ - || true -sed -n '/^TRACE OUTPUT:$/,/^$/p' "$work/output" > "$work/trace-section" cat > "$work/bash-x-trace-section.goal" << EOF TRACE OUTPUT: + $work/trace-and-stderr 0 @@ -546,13 +545,15 @@ TRACE OUTPUT: + exit 0 EOF -if cmp -s "$work/bash-x-trace-section.goal" "$work/trace-section"; then - echo "PASS: bash -x cronic, trace output" -else - echo "FAIL: bash -x cronic, trace output: got:" - cat "$work/output" - status=1 -fi +check_trace_section "bash -x cronic, trace output" \ + "$work/bash-x-trace-section.goal" \ + bash -x "$CRONIC" "$work/trace-and-stderr" 0 + +# A function that the caller exported with the name of a builtin does not run +# in place of the builtin when `cronic` restores the caller's options. +CRONIC="$work/cronic-with-builtin-functions" +check "caller's exported functions named like builtins" 0 silent true +CRONIC="$REAL_CRONIC" # Bash writes its trace lines to a separate file, so they are not error # output, whatever PS4 is. From d5e1141ee85435d671212aed534b58f5b44f1fdb Mon Sep 17 00:00:00 2001 From: Michael Ernst Date: Thu, 8 Oct 2026 21:13:20 -0700 Subject: [PATCH 27/28] Unset the caller's cronic_ functions up front; read names whole; drop builtin prefixes --- cronic | 59 ++++++++++++++++++-------------- tests/cronic-test/cronic-test.sh | 22 ------------ 2 files changed, 33 insertions(+), 48 deletions(-) diff --git a/cronic b/cronic index 5b0918a..17764b1 100755 --- a/cronic +++ b/cronic @@ -18,6 +18,9 @@ # script's. Names that start with `cronic_` are reserved for this script: # COMMAND does not see a variable or function with such a name, even one that # the caller exported. +# This script assumes that the caller has not exported a function with the name +# of a builtin or program that this script uses, such as `set` or `cat`; such a +# function would run in place of the builtin or program. # This script runs under bash 3.2, which is /bin/bash on macOS, so it uses a # feature of a later bash only after checking the version. @@ -27,9 +30,9 @@ # options, and its `shopt` options, without any restoring; an option that this # script changes must be added here. `$-` reports the options without the # cost of a command substitution, such as `$(set +o)`, in which bash would also -# report errexit as off. The options are -# restored in one command, so that restoring verbose and xtrace neither echoes -# nor traces the restoring of the others. +# report errexit as off. The options are restored in one command, so that +# restoring verbose and xtrace neither echoes nor traces the restoring of the +# others. # If the caller exported SHELLOPTS, then this script does not trace itself or # echo its input: that output would go to this script's stderr, and cron would # send email for every run. An exported `verbose` nonetheless echoes the lines @@ -37,11 +40,8 @@ # tell whether xtrace came from SHELLOPTS or from the command line, so # `bash -x cronic` traces this script only if SHELLOPTS is not exported. # The redirection discards the trace of these commands. -# Builtins are called via `builtin`, here and where the caller's options are -# restored, so that a function that the caller exported with the name of a -# builtin, such as `set` or `unset`, does not run in its place. { - cronic_caller_options='builtin set' + cronic_caller_options='set' for cronic_option in e:errexit u:nounset v:verbose x:xtrace; do case $- in *"${cronic_option%%:*}"*) cronic_caller_options+=" -o ${cronic_option#*:}" ;; @@ -50,14 +50,26 @@ done # The filter makes `compgen` match SHELLOPTS exactly, not every exported # variable whose name starts with SHELLOPTS. - if builtin compgen -e -X '!SHELLOPTS' SHELLOPTS > /dev/null; then - builtin set +vx + if compgen -e -X '!SHELLOPTS' SHELLOPTS > /dev/null; then + set +vx + fi + # Unset every function whose name starts with `cronic_`, all of which the + # caller exported, because this script has not yet defined any. Then the + # only such functions are this script's, which the subshell that runs the + # command unsets by name. Testing the status of `compgen` first avoids the + # cost of a process substitution when there is no such function. Each name + # is read whole, rather than split from an unquoted expansion, so that a name + # that contains a glob character is not expanded to file names. + if compgen -A function cronic_ > /dev/null; then + while read -r cronic_name; do + unset -f "$cronic_name" + done < <(compgen -A function cronic_) fi } 2> /dev/null # Exit if a command exits with a non-zero status, and treat unset variables # as an error. -builtin set -eu +set -eu # Prints the usage line. cronic_usage_line() { @@ -220,31 +232,26 @@ fi # is the command does not see this script's variables and functions, and so # that a function does not take precedence over a program of the same name. # This script's functions are listed, rather than found by -# `compgen -A function cronic_`, to avoid the cost of a command substitution; -# the test suite checks that the list is complete. A command substitution -# finds any other such function, such as one that the caller exported, but -# only when the status of `compgen` shows that there is one. +# `compgen -A function cronic_`, to avoid the cost of a command substitution. +# The list must name every function that this script defines before this +# point; the test suite fails if the command can see one that it omits. # Then the subshell restores the caller's options. The subshell first turns # off this script's xtrace and verbose, so that the restoring is not traced or # echoed; any error in the restoring goes to the command's error output. -builtin set +e +set +e ( - { builtin set +vx; } 2> /dev/null + { set +vx; } 2> /dev/null if [[ -n $cronic_xtrace_fd ]]; then - builtin export BASH_XTRACEFD=$cronic_xtrace_fd + export BASH_XTRACEFD=$cronic_xtrace_fd fi - builtin unset -f cronic_usage_line cronic_usage cronic_help \ + unset -f cronic_usage_line cronic_usage cronic_help \ cronic_set_expected cronic_cleanup - if builtin compgen -A function cronic_ > /dev/null; then - # shellcheck disable=SC2046 # Each name is one word. - builtin unset -f $(builtin compgen -A function cronic_) - fi - builtin eval "builtin unset -v \"\${!cronic_@}\" + eval "unset -v \"\${!cronic_@}\" $cronic_caller_options" "$@" ) > "$cronic_out" 2> "$cronic_trace" cronic_result=$? -builtin set -e +set -e if [ -n "$cronic_xtrace_fd" ]; then exec {cronic_xtrace_fd}>&- @@ -253,7 +260,7 @@ fi # Prints a basic regular expression that matches its argument literally. # The argument must not contain a backslash. cronic_bre_quote() { - builtin printf '%s\n' "$1" | LC_ALL=C sed 's/[*.[^]/\\&/g' + printf '%s\n' "$1" | LC_ALL=C sed 's/[*.[^]/\\&/g' } # Prints a basic regular expression that matches the trace lines, for PS4, that @@ -274,7 +281,7 @@ cronic_trace_pattern() { local LC_ALL=C [[ $PS4 =~ ^([$'\xc0'-$'\xff'][$'\x80'-$'\xbf']*|.) ]] local first=${BASH_REMATCH[0]} - builtin printf '^\\(%s\\)\\+%s\n' "$(cronic_bre_quote "$first")" "$(cronic_bre_quote "${PS4#"$first"}")" + printf '^\\(%s\\)\\+%s\n' "$(cronic_bre_quote "$first")" "$(cronic_bre_quote "${PS4#"$first"}")" } cronic_pattern=$(cronic_trace_pattern) diff --git a/tests/cronic-test/cronic-test.sh b/tests/cronic-test/cronic-test.sh index 0d7f832..8ac4ed3 100755 --- a/tests/cronic-test/cronic-test.sh +++ b/tests/cronic-test/cronic-test.sh @@ -143,22 +143,6 @@ exec env SHELLOPTS="$shellopts" "$REAL_CRONIC" "$@" EOF2 chmod +x "$work/cronic-with-shellopts" -# Runs `cronic` with exported shell functions named like the builtins that -# `cronic` uses to restore the caller's options. Each function writes to -# stderr and then runs the builtin. -cat > "$work/cronic-with-builtin-functions" << 'EOF' -#!/bin/bash -for name in compgen eval export printf set unset; do - builtin eval "$name() { - echo \"the function $name ran\" >&2 - builtin $name \"\$@\" - }" - builtin export -f "$name" -done -exec "$REAL_CRONIC" "$@" -EOF -chmod +x "$work/cronic-with-builtin-functions" - # temp_files: prints `cronic`'s temporary files, in a canonical order. temp_files() { find "$TMPDIR" -mindepth 1 -maxdepth 1 2> /dev/null | sort @@ -549,12 +533,6 @@ check_trace_section "bash -x cronic, trace output" \ "$work/bash-x-trace-section.goal" \ bash -x "$CRONIC" "$work/trace-and-stderr" 0 -# A function that the caller exported with the name of a builtin does not run -# in place of the builtin when `cronic` restores the caller's options. -CRONIC="$work/cronic-with-builtin-functions" -check "caller's exported functions named like builtins" 0 silent true -CRONIC="$REAL_CRONIC" - # Bash writes its trace lines to a separate file, so they are not error # output, whatever PS4 is. export PS4 From 23c71c54892669b576221bc80d6a8eba5c07345c Mon Sep 17 00:00:00 2001 From: Michael Ernst Date: Fri, 9 Oct 2026 10:07:24 -0700 Subject: [PATCH 28/28] Simplify option restoring; turn off verbose first; avoid process substitution; share test helper Record the caller's options as `${-//[^euvx]/}`, and turn off an exported verbose and xtrace before any comment, so that cronic echoes only its first lines. Read the caller's cronic_ function names from a here-string, which bash before 5.1 accepts in posix mode. Document the assumption that compgen exists. In the test, re-run without SHELLOPTS via `sh "$0"`, detect it exactly with printenv, factor out run_cronic, check cronic's stderr under verbose, and drop a duplicate test. --- cronic | 85 +++++++++++---------- tests/cronic-test/cronic-test.sh | 122 +++++++++++++++---------------- 2 files changed, 103 insertions(+), 104 deletions(-) diff --git a/cronic b/cronic index 17764b1..2377c5f 100755 --- a/cronic +++ b/cronic @@ -1,4 +1,8 @@ #!/bin/bash +{ + cronic_caller_flags=${-//[^euvx]/} + if compgen -e -X '!SHELLOPTS' SHELLOPTS > /dev/null; then set +vx; fi +} 2> /dev/null # Cronic v2 - cron job report wrapper # Copyright 2007 Chuck Houpt @@ -21,49 +25,47 @@ # This script assumes that the caller has not exported a function with the name # of a builtin or program that this script uses, such as `set` or `cat`; such a # function would run in place of the builtin or program. +# This script assumes that bash has the `compgen` builtin, which is absent only +# if bash was built with `--disable-progcomp`. Without it, an exported +# SHELLOPTS could make this script trace itself to stderr, and the command could +# see a `cronic_` function that the caller exported. # This script runs under bash 3.2, which is /bin/bash on macOS, so it uses a # feature of a later bash only after checking the version. -# The caller's shell options, as a command that restores them, before this -# script changes them for itself. This script changes only errexit, nounset, -# verbose, and xtrace, so the command inherits the caller's other `set -o` -# options, and its `shopt` options, without any restoring; an option that this -# script changes must be added here. `$-` reports the options without the -# cost of a command substitution, such as `$(set +o)`, in which bash would also -# report errexit as off. The options are restored in one command, so that -# restoring verbose and xtrace neither echoes nor traces the restoring of the -# others. -# If the caller exported SHELLOPTS, then this script does not trace itself or -# echo its input: that output would go to this script's stderr, and cron would -# send email for every run. An exported `verbose` nonetheless echoes the lines -# of this script that bash reads before it reaches the `set +vx`. Bash cannot -# tell whether xtrace came from SHELLOPTS or from the command line, so -# `bash -x cronic` traces this script only if SHELLOPTS is not exported. -# The redirection discards the trace of these commands. +# The first command sets cronic_caller_flags to the caller's shell options, as +# the letters that `$-` uses for them, before this script changes them for +# itself. This script changes only errexit, nounset, verbose, and xtrace, so +# the command inherits the caller's other `set -o` options, and its `shopt` +# options, without any restoring; an option that this script changes must be +# added to the pattern. `$-` reports the options without the cost of a command +# substitution, such as `$(set +o)`, in which bash would also report errexit as +# off. +# If the caller exported SHELLOPTS, then the first command also stops this +# script from tracing itself or echoing its input: that output would go to +# this script's stderr, and cron would send email for every run. An exported +# `verbose` nonetheless echoes every line that bash reads before it reaches the +# `set +vx`, which is why that command comes first, before any comment. Bash +# cannot tell whether xtrace came from SHELLOPTS or from the command line, so +# `bash -x cronic` traces this script only if SHELLOPTS is not exported. The +# filter makes `compgen` match SHELLOPTS exactly, not every exported variable +# whose name starts with SHELLOPTS. The redirection discards the trace of the +# first command. + +# Unset every function whose name starts with `cronic_`, all of which the caller +# exported, because this script has not yet defined any. Then the only such +# functions are this script's, which the subshell that runs the command unsets +# by name. Testing the status of `compgen` first avoids the cost of a command +# substitution when there is no such function. Each name is read whole, rather +# than split from an unquoted expansion, so that a name that contains a glob +# character is not expanded to file names. The names are read from a +# here-string rather than from a process substitution, which bash before 5.1 +# rejects in posix mode, as an exported SHELLOPTS can set. The redirection +# discards the trace of these commands. { - cronic_caller_options='set' - for cronic_option in e:errexit u:nounset v:verbose x:xtrace; do - case $- in - *"${cronic_option%%:*}"*) cronic_caller_options+=" -o ${cronic_option#*:}" ;; - *) cronic_caller_options+=" +o ${cronic_option#*:}" ;; - esac - done - # The filter makes `compgen` match SHELLOPTS exactly, not every exported - # variable whose name starts with SHELLOPTS. - if compgen -e -X '!SHELLOPTS' SHELLOPTS > /dev/null; then - set +vx - fi - # Unset every function whose name starts with `cronic_`, all of which the - # caller exported, because this script has not yet defined any. Then the - # only such functions are this script's, which the subshell that runs the - # command unsets by name. Testing the status of `compgen` first avoids the - # cost of a process substitution when there is no such function. Each name - # is read whole, rather than split from an unquoted expansion, so that a name - # that contains a glob character is not expanded to file names. if compgen -A function cronic_ > /dev/null; then while read -r cronic_name; do unset -f "$cronic_name" - done < <(compgen -A function cronic_) + done <<< "$(compgen -A function cronic_)" fi } 2> /dev/null @@ -237,17 +239,22 @@ fi # point; the test suite fails if the command can see one that it omits. # Then the subshell restores the caller's options. The subshell first turns # off this script's xtrace and verbose, so that the restoring is not traced or -# echoed; any error in the restoring goes to the command's error output. +# echoed; any error in the restoring goes to the command's error output. The +# subshell then needs to turn off only this script's nounset (errexit is off +# already), in the same command that turns on the caller's options, so that +# restoring verbose and xtrace neither echoes nor traces the restoring of the +# others. The `:+` avoids a bare `set -`, which would turn off verbose and +# xtrace. set +e ( { set +vx; } 2> /dev/null - if [[ -n $cronic_xtrace_fd ]]; then + if [ -n "$cronic_xtrace_fd" ]; then export BASH_XTRACEFD=$cronic_xtrace_fd fi unset -f cronic_usage_line cronic_usage cronic_help \ cronic_set_expected cronic_cleanup eval "unset -v \"\${!cronic_@}\" - $cronic_caller_options" + set +u${cronic_caller_flags:+ -$cronic_caller_flags}" "$@" ) > "$cronic_out" 2> "$cronic_trace" cronic_result=$? diff --git a/tests/cronic-test/cronic-test.sh b/tests/cronic-test/cronic-test.sh index 8ac4ed3..415ace9 100755 --- a/tests/cronic-test/cronic-test.sh +++ b/tests/cronic-test/cronic-test.sh @@ -19,9 +19,10 @@ unset BASH_ENV # Do not pass this script's own options, such as `-e` and `-u`, to `cronic` and # to the commands below. If SHELLOPTS is exported, then run this script again # without it, because bash makes SHELLOPTS read-only, so it cannot be unset. -if env | grep -q '^SHELLOPTS='; then - env -u SHELLOPTS "$0" "$@" - exit +# `printenv` matches the name exactly, unlike a search of the output of `env`, +# which can match a line within the value of another variable. +if printenv SHELLOPTS > /dev/null; then + exec env -u SHELLOPTS sh "$0" "$@" fi SCRIPT_DIR="$(CDPATH='' cd -- "$(dirname -- "$0")" && pwd -P)" @@ -132,8 +133,7 @@ fail_in_pipeline() { false | true } report_cronic_variables() { - set | grep '^cronic_' >&2 - declare -F | grep ' cronic_' >&2 + compgen -v -A function cronic_ >&2 return 0 } export -f fail_then_continue fail_in_pipeline report_cronic_variables @@ -148,29 +148,45 @@ temp_files() { find "$TMPDIR" -mindepth 1 -maxdepth 1 2> /dev/null | sort } -# check_trace_section DESCRIPTION GOAL-FILE COMMAND...: runs COMMAND, which -# runs `cronic`, and checks that it exits with status 0, leaves no temporary -# files, and produces a report whose "TRACE OUTPUT" section, through the -# following empty line, is the contents of GOAL-FILE. COMMAND's stderr is left -# in "$work/stderr". -check_trace_section() { +# run_cronic DESCRIPTION EXPECTED-STATUS COMMAND...: runs COMMAND, which runs +# `cronic`, with its standard output in "$work/output" and its error output in +# "$work/stderr". If COMMAND exits with a status other than EXPECTED-STATUS, +# or leaves temporary files behind, then reports a failure and returns 1. +run_cronic() { description="$1" - goal="$2" + expected_status="$2" shift 2 before="$(temp_files)" actual_status=0 "$@" > "$work/output" 2> "$work/stderr" || actual_status=$? after="$(temp_files)" - if [ "$actual_status" != 0 ]; then - echo "FAIL: $description: exit status $actual_status, expected 0" + + if [ "$actual_status" != "$expected_status" ]; then + echo "FAIL: $description: exit status $actual_status, expected $expected_status" + cat "$work/output" "$work/stderr" status=1 + return 1 fi if [ "$before" != "$after" ]; then echo "FAIL: $description: temporary files were left behind:" echo "$after" status=1 + return 1 fi +} + +# check_trace_section DESCRIPTION GOAL-FILE COMMAND...: runs COMMAND, which +# runs `cronic`, and checks that it exits with status 0, leaves no temporary +# files, and produces a report whose "TRACE OUTPUT" section, through the +# following empty line, is the contents of GOAL-FILE. COMMAND's stderr is left +# in "$work/stderr". +check_trace_section() { + description="$1" + goal="$2" + shift 2 + + run_cronic "$description" 0 "$@" || return 0 sed -n '/^TRACE OUTPUT:$/,/^$/p' "$work/output" > "$work/trace-section" if cmp -s "$goal" "$work/trace-section"; then echo "PASS: $description" @@ -193,17 +209,10 @@ check() { expected_output="$3" shift 3 - before="$(temp_files)" - actual_status=0 - "$CRONIC" "$@" > "$work/output" 2>&1 || actual_status=$? - after="$(temp_files)" - - if [ "$actual_status" != "$expected_status" ]; then - echo "FAIL: $description: exit status $actual_status, expected $expected_status" - cat "$work/output" - status=1 - return - fi + run_cronic "$description" "$expected_status" "$CRONIC" "$@" || return 0 + # The checks below examine both outputs, because `cronic` writes its usage + # messages to stderr. + cat "$work/stderr" >> "$work/output" case "$expected_output" in silent) @@ -235,13 +244,6 @@ check() { ;; esac - if [ "$before" != "$after" ]; then - echo "FAIL: $description: temporary files were left behind:" - echo "$after" - status=1 - return - fi - echo "PASS: $description" } @@ -357,20 +359,6 @@ check "builtin command" 0 silent : check "exit builtin" 3 report exit 3 check "exec builtin" 3 report exec "$work/trace-only" 3 -# A program whose name is that of a function of `cronic`'s runs rather than the -# function. -cat > "$work/bin/cronic_cleanup" << 'EOF' -#!/bin/sh -echo "the program named cronic_cleanup ran" 1>&2 -exit 4 -EOF -chmod +x "$work/bin/cronic_cleanup" -saved_path="$PATH" -PATH="$work/bin:$PATH" -check "program named cronic_cleanup" 4 \ - "report:^the program named cronic_cleanup ran$" cronic_cleanup -PATH="$saved_path" - # When SHELLOPTS is exported, `cronic`'s own `-e` and `-u` options do not reach # the wrapped command, but the caller's options do. REAL_CRONIC="$CRONIC" @@ -413,16 +401,20 @@ fi # With verbose exported, a successful command produces no report: `cronic` # does not echo, into the command's error output, the commands that restore the -# caller's options. (`cronic` does echo its own first lines to its stderr.) -verbose_status=0 -"$CRONIC" "$default_shellopts:verbose" true > "$work/output" 2> /dev/null \ - || verbose_status=$? -if [ "$verbose_status" = 0 ] && [ ! -s "$work/output" ]; then - echo "PASS: exported SHELLOPTS with verbose" -else - echo "FAIL: exported SHELLOPTS with verbose: exit status $verbose_status, output:" - cat "$work/output" - status=1 +# caller's options. `cronic` echoes to its stderr only its lines through its +# first command, which bash reads before `cronic` turns verbose off. +sed '/^} 2> \/dev\/null$/q' "$REAL_CRONIC" > "$work/verbose-stderr.goal" +if run_cronic "exported SHELLOPTS with verbose" 0 \ + "$CRONIC" "$default_shellopts:verbose" true; then + if [ ! -s "$work/output" ] && cmp -s "$work/verbose-stderr.goal" "$work/stderr"; then + echo "PASS: exported SHELLOPTS with verbose" + else + echo "FAIL: exported SHELLOPTS with verbose: output:" + cat "$work/output" + echo "stderr:" + cat "$work/stderr" + status=1 + fi fi # The caller's `-e` applies within an exported shell function that is the @@ -508,15 +500,15 @@ check "caller's exported cronic_ function is not seen" 0 silent \ CRONIC="$REAL_CRONIC" # `bash -x cronic`, which does not export SHELLOPTS, traces `cronic` itself. -bash_x_status=0 -bash -x "$CRONIC" "$work/trace-only" 0 > "$work/output" 2> "$work/stderr" \ - || bash_x_status=$? -if [ "$bash_x_status" = 0 ] && grep -q "cronic_debug=false" "$work/stderr"; then - echo "PASS: bash -x traces cronic" -else - echo "FAIL: bash -x traces cronic: exit status $bash_x_status, stderr:" - cat "$work/stderr" - status=1 +if run_cronic "bash -x traces cronic" 0 \ + bash -x "$CRONIC" "$work/trace-only" 0; then + if grep -q "cronic_debug=false" "$work/stderr"; then + echo "PASS: bash -x traces cronic" + else + echo "FAIL: bash -x traces cronic: stderr:" + cat "$work/stderr" + status=1 + fi fi # Under `bash -x cronic`, the report's trace section contains nothing that