From 5e73d5129361a6d9effdfe86210e5c46e8bf3159 Mon Sep 17 00:00:00 2001 From: Michael Ernst Date: Thu, 8 Oct 2026 20:20:05 -0700 Subject: [PATCH 1/2] 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 73b1f464d93eac4b54e237616acf2ccb3a4519c0 Mon Sep 17 00:00:00 2001 From: Michael Ernst Date: Thu, 8 Oct 2026 20:32:24 -0700 Subject: [PATCH 2/2] 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.