diff --git a/scripts/pulse-auto-update.sh b/scripts/pulse-auto-update.sh index 6d7ee23218..c39d6a0db9 100755 --- a/scripts/pulse-auto-update.sh +++ b/scripts/pulse-auto-update.sh @@ -391,8 +391,11 @@ perform_update() { # Whatever way this function exits, never leave Pulse stopped when it was # running before the update (#1630). Extended below once the installer - # tempfiles exist. - trap 'ensure_service_restarted "$service_name" "$service_was_active"' RETURN + # tempfiles exist. The trap clears itself as it runs: a RETURN trap is + # shell-global and would otherwise fire again when a later function returns, + # expanding these now out-of-scope locals under `set -u` and failing an + # otherwise successful update (#2128). + trap 'ensure_service_restarted "$service_name" "$service_was_active"; trap - RETURN' RETURN # Refuse to install a prerelease via the unattended updater. The stable # channel must never cross onto a tag like v6.0.0-rc.2, even if every @@ -440,7 +443,7 @@ perform_update() { installer_tmp=$(mktemp /tmp/pulse-update-installer.XXXXXX) signature_tmp=$(mktemp /tmp/pulse-update-installer.sig.XXXXXX) - trap 'rm -f "$installer_tmp" "$signature_tmp"; ensure_service_restarted "$service_name" "$service_was_active"' RETURN + trap 'rm -f "$installer_tmp" "$signature_tmp"; ensure_service_restarted "$service_name" "$service_was_active"; trap - RETURN' RETURN if ! curl -fsSL "$install_script_url" -o "$installer_tmp"; then log error "Failed to download installer from $install_script_url" diff --git a/scripts/tests/test-pulse-auto-update.sh b/scripts/tests/test-pulse-auto-update.sh index 4347487cc1..185b0f0eec 100755 --- a/scripts/tests/test-pulse-auto-update.sh +++ b/scripts/tests/test-pulse-auto-update.sh @@ -354,6 +354,77 @@ INSTALLER return "${status}" } +test_perform_update_return_trap_does_not_leak() { + # Regression for #2128: the RETURN trap installed inside perform_update is + # shell-global. If it is not cleared when perform_update returns, it fires + # again when a later function returns, expanding the now out-of-scope locals + # under `set -u`. That aborts an otherwise successful update with + # "installer_tmp: unbound variable" and marks pulse-update.service failed. + local tmpdir + tmpdir="$(mktemp -d)" + local status=0 + + INSTALL_DIR="${tmpdir}/opt/pulse" + CONFIG_DIR="${tmpdir}/etc/pulse" + mkdir -p "${INSTALL_DIR}/bin" "${CONFIG_DIR}" + printf 'v5.1.24\n' > "${INSTALL_DIR}/VERSION" + cat > "${INSTALL_DIR}/bin/pulse" <<'EOF' +#!/usr/bin/env bash +echo "v5.1.24" +EOF + chmod +x "${INSTALL_DIR}/bin/pulse" + + export INSTALL_DIR + export FAKE_NEW_VERSION="v5.1.25" + + is_prerelease_tag() { return 1; } + detect_service_name() { echo "pulse"; } + resolve_install_script_url() { echo "http://localhost/install.sh"; } + verify_release_signature() { return 0; } + get_current_version() { tr -d '\r\n' < "${INSTALL_DIR}/VERSION"; } + + # The fake installer bumps VERSION, simulating a successful install. + curl() { + local out="" prev="" + local arg + for arg in "$@"; do + if [[ "$prev" == "-o" ]]; then out="$arg"; fi + prev="$arg" + done + if [[ -n "$out" ]]; then + case "$out" in + *.sig.*) printf 'dummy-signature\n' > "$out" ;; + *) + cat > "$out" <<'INSTALLER' +#!/usr/bin/env bash +printf '%s\n' "${FAKE_NEW_VERSION}" > "${PULSE_INSTALL_DIR}/VERSION" +exit 0 +INSTALLER + ;; + esac + fi + return 0 + } + + systemctl() { return 0; } + sleep() { :; } + + if ! perform_update "v5.1.25"; then + echo "perform_update unexpectedly failed on a successful install" >&2 + status=1 + fi + + # Deliberately do NOT clear the RETURN trap here. The fix must clear it as it + # runs; this subsequent function return must not re-trigger it. + trap_leak_probe() { :; } + if ! trap_leak_probe; then + status=1 + fi + + rm -rf "${tmpdir}" + return "${status}" +} + main() { assert_success "wait_for_service_active retries until active" test_wait_for_service_active_succeeds_after_retry assert_success "wait_for_service_active times out when never active" test_wait_for_service_active_times_out_when_never_active @@ -363,6 +434,7 @@ main() { assert_success "ensure_service_restarted no-ops when service was inactive" test_ensure_service_restarted_noops_when_service_was_inactive assert_success "ensure_service_restarted starts a stopped service" test_ensure_service_restarted_starts_stopped_service assert_success "perform_update restarts service when installer fails" test_perform_update_restarts_service_when_installer_fails + assert_success "perform_update RETURN trap does not leak into later returns" test_perform_update_return_trap_does_not_leak if (( failures > 0 )); then echo "Total failures: ${failures}" >&2