From 2ce38ce9eb1d449085f2e4389e2d4005f74b8bd5 Mon Sep 17 00:00:00 2001 From: 28allday Date: Thu, 23 Apr 2026 20:52:41 +0100 Subject: [PATCH] Harden stop path, autostart, and state file Bug-check pass on top of the v2 rewrite. Five real issues fixed: * autostart_enable used to return the exit code of the trailing `log` call, so a failing `systemctl --user enable` was silently reported as success. Now returns 1 with a tui_err on real failure. * save_state wrote directly to STATE_FILE; a crash mid-write would leave a truncated file that load_state would partially parse. Switched to an atomic tmp + mv -f pattern. * load_state used `source "$STATE_FILE"` which is arbitrary code execution if a video path ever contained shell metacharacters. Replaced with a read-based KEY=VALUE parser that only honours LAST_VIDEO / LAST_TARGET / LAST_DIR. * stop_mpvpaper can be called twice in quick succession (TUI stop immediately followed by systemd's ExecStop). Wrapped the whole body in a `flock -n` on $STATE_DIR/.stop.lock so the second caller no-ops instead of racing against the first. * Watcher `cleanup` trap used `[ -n VAR ] && kill`, which short-circuits to non-zero when VAR is unset and aborts the trap before `exit 0` under set -e. Restructured to a proper if/|| true. --- motion-wallpaper-toggle | 72 ++++++++++++++++++++++++++++++++-------- motion-wallpaper-watcher | 18 +++++++++- 2 files changed, 76 insertions(+), 14 deletions(-) diff --git a/motion-wallpaper-toggle b/motion-wallpaper-toggle index fc2b2e4..a0ef25d 100644 --- a/motion-wallpaper-toggle +++ b/motion-wallpaper-toggle @@ -92,10 +92,19 @@ load_state() { LAST_VIDEO="" LAST_TARGET="" LAST_DIR="" - if [ -f "$STATE_FILE" ]; then - # shellcheck source=/dev/null - source "$STATE_FILE" - fi + [ -f "$STATE_FILE" ] || return 0 + # Parse KEY="VALUE" lines directly instead of `source`, so a maliciously + # crafted video path (e.g. containing `";rm -rf …`) can't execute. + local key val + while IFS='=' read -r key val; do + val="${val#\"}" + val="${val%\"}" + case "$key" in + LAST_VIDEO) LAST_VIDEO="$val" ;; + LAST_TARGET) LAST_TARGET="$val" ;; + LAST_DIR) LAST_DIR="$val" ;; + esac + done < "$STATE_FILE" } save_state() { @@ -103,11 +112,14 @@ save_state() { # so the filesystem browser re-opens where the user last landed. local dir="${3:-$(dirname "$1")}" umask 077 - cat > "$STATE_FILE" < "$tmp" </dev/null 2>&1 + if ! systemctl --user enable motion-wallpaper.service >/dev/null 2>&1; then + tui_err "Failed to enable autostart (systemctl error)." + return 1 + fi log "autostart enabled" + return 0 } autostart_disable() { @@ -281,6 +297,14 @@ start_watcher() { restore_static_wallpaper() { local omarchy_bg="$HOME/.config/omarchy/current/background" if [ -e "$omarchy_bg" ]; then + # Stop may be called twice in quick succession when the TUI stops the + # wallpaper and systemd's ExecStop fires right after. Skip if someone + # else already put swaybg back so we don't kill-and-respawn (causes a + # visible flicker). + if pgrep -x swaybg >/dev/null 2>&1 && ! pgrep -x mpvpaper >/dev/null 2>&1; then + log "swaybg already running, skipping restore" + return 0 + fi pkill -x hyprpaper 2>/dev/null || true pkill -x swaybg 2>/dev/null || true if command -v uwsm-app >/dev/null 2>&1; then @@ -338,13 +362,18 @@ start_mpvpaper_bg() { } stop_mpvpaper() { - kill_watcher - pkill -x mpvpaper 2>/dev/null || true - # mpv cleans up the IPC socket on exit, but if we killed mpvpaper hard - # the dangling socket can linger and confuse the next run. - rm -f "$MPV_IPC_SOCK" 2>/dev/null || true - restore_static_wallpaper - log "stopped" + # Serialize stops so the TUI-initiated path and systemd's ExecStop (which + # fires when the TUI kills mpvpaper) can't both be mid-restore at once. + # `flock -n` → second caller skips cleanly if the first still holds it. + ( + flock -n 9 || { log "stop already in progress, skipping"; exit 0; } + kill_watcher + pkill -x mpvpaper 2>/dev/null || true + # mpv cleans up its IPC socket on exit; a hard kill can leave it dangling. + rm -f "$MPV_IPC_SOCK" 2>/dev/null || true + restore_static_wallpaper + log "stopped" + ) 9>"$STATE_DIR/.stop.lock" } # ===== actions ================================================================ @@ -370,6 +399,14 @@ action_toggle() { stop_mpvpaper tui_ok "Stopped. Normal wallpaper restored." notify "Motion wallpaper stopped." + # If autostart is on, a plain stop will let the wallpaper return on + # next reboot. Offer to turn autostart off so "stop" means "stop". + if autostart_enabled; then + if gum confirm "Autostart is still enabled — also disable it so the wallpaper doesn't resume after reboot?"; then + autostart_disable + tui_ok "Autostart disabled." + fi + fi ;; "Change video") load_state @@ -388,6 +425,15 @@ action_toggle() { "Turn autostart OFF") autostart_disable tui_ok "Autostart disabled." + # Motion wallpaper is still running at this point. Offer to also stop + # it now so the label "turn off" matches visible behaviour. + if is_running; then + if gum confirm "Motion wallpaper is still running — stop it now too?"; then + stop_mpvpaper + tui_ok "Stopped. Normal wallpaper restored." + notify "Motion wallpaper stopped." + fi + fi ;; Cancel) exit 0 ;; esac diff --git a/motion-wallpaper-watcher b/motion-wallpaper-watcher index ba7e231..83fb736 100644 --- a/motion-wallpaper-watcher +++ b/motion-wallpaper-watcher @@ -47,9 +47,25 @@ resume_mpv() { send_mpv '{ "command": ["set_property", "pause", false] }'; } log "watching $HYPR_SOCK" +# Kill socat child on exit so it doesn't spam "Broken pipe" to the log when +# the watcher is killed by the main toggle script. +cleanup() { + # `[ ] && kill` short-circuits to non-zero when SOCAT_PID is unset, which + # under `set -e` would abort the trap before `exit 0` — wrap safely. + if [ -n "${SOCAT_PID:-}" ]; then + kill "$SOCAT_PID" 2>/dev/null || true + fi + exit 0 +} +trap cleanup EXIT INT TERM + # Hyprland socket2 emits newline-separated "EVENT>>DATA" lines. We only care # about the fullscreen state. `fullscreen>>1` = entered, `fullscreen>>0` = left. -socat -u "UNIX-CONNECT:$HYPR_SOCK" - | while IFS= read -r line; do +# socat stderr is dropped so EPIPE on shutdown doesn't bloat the log. +coproc SOCAT { socat -u "UNIX-CONNECT:$HYPR_SOCK" - 2>/dev/null; } +# Bash auto-exports SOCAT_PID from `coproc SOCAT`; used in cleanup trap. + +while IFS= read -r line <&"${SOCAT[0]}"; do case "$line" in fullscreen\>\>1) log "fullscreen entered — pause"