From eab79b0ba29c9635d31137e429720ac355ebb69d Mon Sep 17 00:00:00 2001 From: Ramesh Padmanabhaiah Date: Fri, 3 Jul 2026 12:54:40 -0700 Subject: [PATCH] Harden cleanup temp file handling --- lib/bash/file/lib_file.sh | 65 +++++++++++++++++++++---------- lib/bash/file/tests/lib_file.bats | 38 +++++++++++++++--- lib/bash/git/lib_git.sh | 2 +- lib/bash/git/tests/lib_git.bats | 40 ++++++++++++++----- lib/bash/std/lib_std.sh | 9 +++-- lib/bash/std/tests/lib_std.bats | 46 ++++++++++++++++++++++ lib/bash/tests/test_helper.sh | 12 ++++++ 7 files changed, 172 insertions(+), 40 deletions(-) diff --git a/lib/bash/file/lib_file.sh b/lib/bash/file/lib_file.sh index 4b99583..0f5d750 100644 --- a/lib/bash/file/lib_file.sh +++ b/lib/bash/file/lib_file.sh @@ -31,6 +31,32 @@ __preserve_file_mode__() { chmod "$source_mode" "$target_file" } +__file_remove_temp_paths__() { + local path + + for path; do + [[ -n "$path" ]] && rm -f -- "$path" + done + return 0 +} + +__file_make_target_temp__() { + local result_name="$1" target_file="$2" + local target_dir target_base temp_dir temp_path + + target_dir="$(dirname -- "$target_file")" + target_base="$(basename -- "$target_file")" + temp_dir="$(cd -- "$target_dir" && pwd -P)" || return 1 + + temp_path="$(mktemp "$temp_dir/$target_base.XXXXXX" 2>/dev/null)" || return 1 + if ! std_register_cleanup_path "$temp_path"; then + rm -f -- "$temp_path" + return 1 + fi + + printf -v "$result_name" '%s' "$temp_path" +} + __file_section_markers_ordered__() { local target_file="$1" beginning_marker="$2" end_marker="$3" @@ -165,24 +191,22 @@ update_file_section() { local current_content_file="" new_content_file="" temp_file if [[ "$remove_section" == false ]]; then - new_content_file=$(mktemp "${TMPDIR:-/tmp}/base-file-section-new.XXXXXX") - if [[ ! -f "$new_content_file" ]]; then + if ! std_make_temp_file new_content_file base-file-section-new; then log_error "Failed to create temporary content file for '$target_file'." return 1 fi if ! printf '%s' "$new_content_string" > "$new_content_file"; then log_error "Failed to write replacement content for '$target_file'." - rm -f "$new_content_file" + __file_remove_temp_paths__ "$new_content_file" return 1 fi fi if [[ "$section_exists" == true && "$remove_section" == false ]]; then - current_content_file=$(mktemp "${TMPDIR:-/tmp}/base-file-section-current.XXXXXX") - if [[ ! -f "$current_content_file" ]]; then + if ! std_make_temp_file current_content_file base-file-section-current; then log_error "Failed to create temporary current content file for '$target_file'." - rm -f "$new_content_file" + __file_remove_temp_paths__ "$new_content_file" return 1 fi @@ -204,28 +228,27 @@ update_file_section() { } ' "$target_file" > "$current_content_file"; then log_error "Failed to read existing section in '$target_file'." - rm -f "$current_content_file" "$new_content_file" + __file_remove_temp_paths__ "$current_content_file" "$new_content_file" return 1 fi if cmp -s "$current_content_file" "$new_content_file"; then log_debug "Section already up to date in '$target_file'." - rm -f "$current_content_file" "$new_content_file" + __file_remove_temp_paths__ "$current_content_file" "$new_content_file" return 0 fi - rm -f "$current_content_file" + __file_remove_temp_paths__ "$current_content_file" fi log_info "Updating '$target_file'" - temp_file=$(mktemp "${target_file}.XXXXXX") - if [[ ! -f "$temp_file" ]]; then + if ! __file_make_target_temp__ temp_file "$target_file"; then log_error "Failed to create temporary file for '$target_file'." - rm -f "$new_content_file" + __file_remove_temp_paths__ "$new_content_file" return 1 fi if ! __preserve_file_mode__ "$target_file" "$temp_file"; then log_error "Failed to preserve permissions for '$target_file'." - rm -f "$temp_file" "$new_content_file" + __file_remove_temp_paths__ "$temp_file" "$new_content_file" return 1 fi @@ -248,7 +271,7 @@ update_file_section() { } } ' "$target_file" > "$temp_file" && mv -f "$temp_file" "$target_file"; then - rm -f "$new_content_file" + __file_remove_temp_paths__ "$new_content_file" return 0 fi else @@ -274,26 +297,26 @@ update_file_section() { print $0 } ' "$target_file" > "$temp_file" && mv -f "$temp_file" "$target_file"; then - rm -f "$new_content_file" + __file_remove_temp_paths__ "$new_content_file" return 0 fi fi log_error "Failed to process sections in '$target_file'." - rm -f "$temp_file" "$new_content_file" + __file_remove_temp_paths__ "$temp_file" "$new_content_file" return 1 else # Markers not found in the file if ! cp "$target_file" "$temp_file"; then log_error "Failed to copy '$target_file' to '$temp_file'." - rm -f "$temp_file" "$new_content_file" + __file_remove_temp_paths__ "$temp_file" "$new_content_file" return 1 fi if [[ -s "$temp_file" ]] && [[ $(tail -c 1 "$temp_file" 2>/dev/null | wc -l) -eq 0 ]]; then if ! printf '\n' >> "$temp_file"; then log_error "Failed to add trailing newline to '$temp_file'." - rm -f "$temp_file" "$new_content_file" + __file_remove_temp_paths__ "$temp_file" "$new_content_file" return 1 fi fi @@ -304,17 +327,17 @@ update_file_section() { printf '%s\n' "$end_marker" } >> "$temp_file"; then log_error "Failed to add new section to '$target_file'." - rm -f "$temp_file" "$new_content_file" + __file_remove_temp_paths__ "$temp_file" "$new_content_file" return 1 fi if ! mv -f "$temp_file" "$target_file"; then log_error "Failed to replace '$target_file' with '$temp_file'." - rm -f "$temp_file" "$new_content_file" + __file_remove_temp_paths__ "$temp_file" "$new_content_file" return 1 fi - rm -f "$new_content_file" + __file_remove_temp_paths__ "$new_content_file" return 0 fi } diff --git a/lib/bash/file/tests/lib_file.bats b/lib/bash/file/tests/lib_file.bats index e085866..8464b4e 100644 --- a/lib/bash/file/tests/lib_file.bats +++ b/lib/bash/file/tests/lib_file.bats @@ -147,6 +147,32 @@ EOF [ "$(cat "$target")" = $'before\n# BEGIN\nfirst\nsecond\nthird\n# END\nafter' ] } +@test "update_file_section registers internal temp files for cleanup" { + local registration_file="$TEST_TMPDIR/registered-cleanup-paths.txt" + local target="$TEST_TMPDIR/config.txt" + cat <<'EOF' > "$target" +before +# BEGIN +old +# END +after +EOF + + eval "$(declare -f std_register_cleanup_path | sed '1s/std_register_cleanup_path/__orig_std_register_cleanup_path/')" + std_register_cleanup_path() { + printf '%s\n' "$@" >> "$registration_file" + __orig_std_register_cleanup_path "$@" + } + + update_file_section "$target" "# BEGIN" "# END" "new" + unset -f std_register_cleanup_path __orig_std_register_cleanup_path + + [ "$(cat "$target")" = $'before\n# BEGIN\nnew\n# END\nafter' ] + [[ "$(cat "$registration_file")" == *"base-file-section-new."* ]] + [[ "$(cat "$registration_file")" == *"base-file-section-current."* ]] + [[ "$(cat "$registration_file")" == *"config.txt."* ]] +} + @test "update_file_section skips unchanged existing section" { local before_inode local target="$TEST_TMPDIR/config.txt" @@ -160,7 +186,7 @@ after EOF before_inode="$(file_inode "$target")" - bats_run update_file_section "$target" "# BEGIN" "# END" "same" "content" + capture_command update_file_section "$target" "# BEGIN" "# END" "same" "content" [ "$status" -eq 0 ] [[ "$output" != *"Updating '$target'"* ]] @@ -168,7 +194,7 @@ EOF [ "$(cat "$target")" = $'before\n# BEGIN\nsame\ncontent\n# END\nafter' ] set_log_level DEBUG - bats_run update_file_section "$target" "# BEGIN" "# END" "same" "content" + capture_command update_file_section "$target" "# BEGIN" "# END" "same" "content" [ "$status" -eq 0 ] [[ "$output" == *"Section already up to date in '$target'."* ]] @@ -201,7 +227,7 @@ EOF [ "$(cat "$target")" = $'before\n# BEGIN\nsecret\nvalue\n# END\nafter' ] } -@test "update_file_section removes a marked block with -r" { +@test "update_file_section removes a marked block with remove option" { local target="$TEST_TMPDIR/config.txt" cat <<'EOF' > "$target" before @@ -216,7 +242,7 @@ EOF [ "$(cat "$target")" = $'before\nafter' ] } -@test "update_file_section removes only the first matching marked block with -r" { +@test "update_file_section removes only the first matching marked block with remove option" { local target="$TEST_TMPDIR/config.txt" cat <<'EOF' > "$target" before @@ -287,7 +313,7 @@ EOF @test "update_file_section is a no-op for a missing target file" { local target="$TEST_TMPDIR/missing.txt" - bats_run update_file_section "$target" "# BEGIN" "# END" "value" + capture_command update_file_section "$target" "# BEGIN" "# END" "value" [ "$status" -eq 0 ] [ ! -e "$target" ] @@ -312,7 +338,7 @@ EOF return 1 } - bats_run update_file_section "$target" "# BEGIN" "# END" "value" + capture_command update_file_section "$target" "# BEGIN" "# END" "value" unset -f cp [ "$status" -eq 1 ] diff --git a/lib/bash/git/lib_git.sh b/lib/bash/git/lib_git.sh index 4dbf6ac..46dcb2b 100644 --- a/lib/bash/git/lib_git.sh +++ b/lib/bash/git/lib_git.sh @@ -158,7 +158,7 @@ git_update_repo() { return 1 fi - git_log="$(mktemp "${TMPDIR:-/tmp}/git_log.XXXXXX")" || { + std_make_temp_file git_log git_log || { log_error "Unable to create temporary git log file." return 1 } diff --git a/lib/bash/git/tests/lib_git.bats b/lib/bash/git/tests/lib_git.bats index b4db3c3..87b0c19 100644 --- a/lib/bash/git/tests/lib_git.bats +++ b/lib/bash/git/tests/lib_git.bats @@ -115,7 +115,7 @@ setup() { } @test "git_update_repo usage names the current function" { - bats_run git_update_repo + capture_command git_update_repo [ "$status" -eq 1 ] [[ "$output" == *"Usage: git_update_repo /path/to/repo [allowed_dirty_path] [expected_branch]"* ]] @@ -131,7 +131,7 @@ setup() { printf 'local change\n' > "$repo/data.txt" set_log_level DEBUG - bats_run git_update_repo "$repo" + capture_command git_update_repo "$repo" [ "$status" -eq 0 ] [[ "$output" == *"has local changes; skipping auto-update"* ]] @@ -145,7 +145,7 @@ setup() { commit_all "$repo" "Initial commit" set_log_level DEBUG - bats_run git_update_repo "$repo" "" release + capture_command git_update_repo "$repo" "" release [ "$status" -eq 0 ] [[ "$output" == *"not 'release'. Skipping update"* ]] @@ -160,7 +160,7 @@ setup() { commit_all "$repo" "Initial commit" before_head="$(git -C "$repo" rev-parse HEAD)" - bats_run git_update_repo "$repo" "" main + capture_command git_update_repo "$repo" "" main [ "$status" -eq 1 ] [[ "$output" == *"git pull failed on repo '$repo'"* ]] @@ -178,7 +178,7 @@ setup() { before_head="$(git -C "$repo" rev-parse HEAD)" git -C "$repo" remote set-url origin "$TEST_TMPDIR/missing-remote.git" - bats_run git_update_repo "$repo" "" main + capture_command git_update_repo "$repo" "" main [ "$status" -eq 1 ] [[ "$output" == *"git pull failed on repo '$repo'"* ]] @@ -204,7 +204,7 @@ setup() { git -C "$other" commit --amend -m "Rewrite remote history" >/dev/null 2>&1 git -C "$other" push --force origin main >/dev/null 2>&1 - bats_run git_update_repo "$repo" "" main + capture_command git_update_repo "$repo" "" main [ "$status" -eq 1 ] [[ "$output" == *"git pull failed on repo '$repo'"* ]] @@ -230,7 +230,7 @@ setup() { git -C "$other" push origin main >/dev/null 2>&1 printf 'local untracked\n' > "$repo/local-notes.md" - bats_run git_update_repo "$repo" "" main + capture_command git_update_repo "$repo" "" main [ "$status" -eq 1 ] [[ "$output" == *"git pull failed on repo '$repo'"* ]] @@ -249,7 +249,7 @@ setup() { printf 'local change\n' > "$repo/data.txt" set_log_level DEBUG - bats_run git_update_repo "$repo" + capture_command git_update_repo "$repo" [ "$status" -eq 0 ] [[ "$output" == *"has local changes; skipping auto-update"* ]] @@ -397,7 +397,7 @@ setup() { printf 'local change\n' > "$repo/data.txt" trap 'printf "outer return trap\n"' RETURN - TMPDIR="$temp_dir" bats_run git_update_repo "$repo" + TMPDIR="$temp_dir" capture_command git_update_repo "$repo" return_trap="$(trap -p RETURN)" trap - RETURN @@ -406,6 +406,28 @@ setup() { ! compgen -G "$temp_dir/git_log.*" >/dev/null } +@test "git_update_repo registers temp log for cleanup" { + local repo="$TEST_TMPDIR/repo" + local registration_file="$TEST_TMPDIR/registered-cleanup-paths.txt" + + init_git_repo "$repo" + printf 'base\n' > "$repo/data.txt" + commit_all "$repo" "Initial commit" + printf 'local change\n' > "$repo/data.txt" + + eval "$(declare -f std_register_cleanup_path | sed '1s/std_register_cleanup_path/__orig_std_register_cleanup_path/')" + std_register_cleanup_path() { + printf '%s\n' "$@" >> "$registration_file" + __orig_std_register_cleanup_path "$@" + } + + capture_command git_update_repo "$repo" + unset -f std_register_cleanup_path __orig_std_register_cleanup_path + + [ "$status" -eq 0 ] + [[ "$(cat "$registration_file")" == *"git_log."* ]] +} + @test "_git_update_repo_finish removes temp log after success" { local git_log="$TEST_TMPDIR/git.log" diff --git a/lib/bash/std/lib_std.sh b/lib/bash/std/lib_std.sh index a93fd18..3d2a6ee 100644 --- a/lib/bash/std/lib_std.sh +++ b/lib/bash/std/lib_std.sh @@ -1033,7 +1033,7 @@ __std_run_with_timeout_fallback__() { local timeout_marker command_pid timer_pid command_status local kill_grace_seconds=1 - timeout_marker="$(mktemp "${TMPDIR:-/tmp}/base-bash-libs-timeout.XXXXXXXXXX" 2>/dev/null)" || return 127 + std_make_temp_file timeout_marker base-bash-libs-timeout || return 127 "$@" & command_pid=$! @@ -1229,10 +1229,13 @@ safe_truncate() { __std_get_exit_trap_command__() { local result_name="$1" trap_spec="" + local trap_prefix="trap -- '" trap_suffix="' EXIT" trap_spec="$(trap -p EXIT || true)" - if [[ "$trap_spec" =~ ^trap\ --\ \'(.*)\'\ EXIT$ ]]; then - printf -v "$result_name" '%s' "${BASH_REMATCH[1]}" + if [[ "$trap_spec" == "$trap_prefix"*"$trap_suffix" ]]; then + trap_spec="${trap_spec#"$trap_prefix"}" + trap_spec="${trap_spec%"$trap_suffix"}" + printf -v "$result_name" '%s' "$trap_spec" else printf -v "$result_name" '%s' "" fi diff --git a/lib/bash/std/tests/lib_std.bats b/lib/bash/std/tests/lib_std.bats index 88e1156..6cfd7af 100644 --- a/lib/bash/std/tests/lib_std.bats +++ b/lib/bash/std/tests/lib_std.bats @@ -972,6 +972,31 @@ EOF [ ! -e "$marker_file" ] } +@test "std_run fallback timeout marker is registered for cleanup" { + local fake_bin="$TEST_TMPDIR/no-timeout-bin" + local registration_file="$TEST_TMPDIR/registered-cleanup-paths.txt" + local output_file="$TEST_TMPDIR/timeout-fallback-output.txt" + local old_path="$PATH" + + mkdir -p "$fake_bin" + ln -s "$(command -v mktemp)" "$fake_bin/mktemp" + ln -s "$(command -v rm)" "$fake_bin/rm" + ln -s "$(command -v sleep)" "$fake_bin/sleep" + + eval "$(declare -f std_register_cleanup_path | sed '1s/std_register_cleanup_path/__orig_std_register_cleanup_path/')" + std_register_cleanup_path() { + printf '%s\n' "$@" >> "$registration_file" + __orig_std_register_cleanup_path "$@" + } + + PATH="$fake_bin" std_run --no-exit --quiet --timeout 5 /bin/echo fallback > "$output_file" + PATH="$old_path" + unset -f std_register_cleanup_path __orig_std_register_cleanup_path + + [ "$(cat "$output_file")" = "fallback" ] + [[ "$(cat "$registration_file")" == *"base-bash-libs-timeout."* ]] +} + @test "std_run --max-attempts retries until the command succeeds" { local counter_file="$TEST_TMPDIR/retry-count.txt" local script="$TEST_TMPDIR/retry-eventual-success.sh" @@ -1296,6 +1321,27 @@ EOF [ "$(cat "$log_file")" = $'existing\ncleanup-one\ncleanup-two' ] } +@test "cleanup hooks preserve multi-line existing EXIT traps" { + local script="$TEST_TMPDIR/cleanup-multiline-trap.sh" + local log_file="$TEST_TMPDIR/cleanup-multiline-trap.log" + + create_script "$script" <> "$log_file" +printf "existing-two\n" >> "$log_file" +' EXIT +cleanup_one() { printf "cleanup-one\n" >> "$log_file"; } +std_register_cleanup_hook cleanup_one +EOF + + bats_run bash "$script" + + [ "$status" -eq 0 ] + [ "$(cat "$log_file")" = $'existing-one\nexisting-two\ncleanup-one' ] +} + @test "cleanup hook registration ignores duplicates and supports removal" { local script="$TEST_TMPDIR/cleanup-hook-remove.sh" local log_file="$TEST_TMPDIR/cleanup-hook-remove.log" diff --git a/lib/bash/tests/test_helper.sh b/lib/bash/tests/test_helper.sh index 5b27a15..a161505 100644 --- a/lib/bash/tests/test_helper.sh +++ b/lib/bash/tests/test_helper.sh @@ -52,6 +52,18 @@ setup_test_tmpdir() { mkdir -p "$TEST_TMPDIR" } +capture_command() { + local capture_file="${TEST_TMPDIR:?}/captured-command.out" + + set +e + "$@" >"$capture_file" 2>&1 + status=$? + set -e + + output="$(cat "$capture_file")" + IFS=$'\n' read -r -d '' -a lines < "$capture_file" || true +} + init_git_repo() { local repo_dir="$1" local default_branch="${2:-main}"