Bash & systemd Reliability — truncate races, jq quoting, Restart=on-failure

AdminInterface/www/includes/footer.php

@@ -138,14 +138,23 @@ function updateBatteryIcon() {
 </html>
 
 <?php
+	// R3-B-5: previous code launched restart.sh / shutdown.sh in a
+	// detached background shell with no inter-request locking — a
+	// double-click on the reboot button (or two admin tabs both
+	// pressing Reboot in quick succession) spawned two restart.sh
+	// processes simultaneously. Their cleanup steps fight (kill of
+	// pm2 from one terminates spawn from the other, etc.) and the
+	// box can end up in a half-rebooted state. Wrap in `flock -n`
+	// against a per-action lockfile so a second invocation while
+	// the first is still running becomes a no-op.
 	if( $reboot == 1 )
 		{
-		$command='sudo su - -c "sleep 5; /usr/local/bin/mupibox/./restart.sh &" &';
+		$command='( flock -n 9 || exit 0; sleep 5; sudo /usr/local/bin/mupibox/./restart.sh ) 9>/tmp/.mupibox.reboot.lock &';
 		exec($command);
 		}
 	if( $shutdown == 1 )
 		{
-		$command='sudo su - -c "sleep 5; /usr/local/bin/mupibox/./shutdown.sh &" &';
+		$command='( flock -n 9 || exit 0; sleep 5; sudo /usr/local/bin/mupibox/./shutdown.sh ) 9>/tmp/.mupibox.shutdown.lock &';
 		exec($command);
 		}
 ?>

AdminInterface/www/service.php

@@ -3,13 +3,26 @@
 	$dataonline = json_decode($onlinejson, true);
 	include ('includes/header.php');
 
+	// LOW-3: every apt-get path here can collide with another concurrent
+	// service-toggle (admin clicks "enable VNC" while "enable Samba" is
+	// still running). dpkg has its own exclusive lock at
+	// /var/lib/dpkg/lock-frontend, so the second request fails with a
+	// confusing error in stderr but the PHP page just says "VNC enabled"
+	// — the user has no idea it didn't actually run. Serialise apt-touching
+	// commands behind a flock so the second one waits up to 120s for the
+	// first to finish, and log if the wait expires.
+	$APT_LOCK = '/tmp/.mupibox.apt.lock';
+	$aptWrap = function ($cmd) use ($APT_LOCK) {
+		return "flock -w 120 " . escapeshellarg($APT_LOCK) . " bash -c " . escapeshellarg($cmd);
+	};
+
 	if( $_POST['change_vnc'] == "stop & disable" )
 		{
 		exec("sudo systemctl stop mupi_vnc.service");
 		exec("sudo systemctl stop mupi_novnc.service");
 		exec("sudo systemctl disable mupi_vnc.service");
 		exec("sudo systemctl disable mupi_novnc.service");
-		exec("sudo apt-get remove x11vnc websockify -y");
+		exec($aptWrap("sudo apt-get remove x11vnc websockify -y"));
 		exec("sudo pkill websockify");		
 		exec("sudo rm -R /usr/share/novnc");
 		exec("sudo su - -c \"/usr/bin/cat <<< $(/usr/bin/jq --arg v \"0\" '.tweaks.vnc = $v' /etc/mupibox/mupiboxconfig.json) >  /etc/mupibox/mupiboxconfig.json\"");
@@ -18,7 +31,7 @@
 		}
 	else if( $_POST['change_vnc'] == "enable & start" )
 		{
-		exec("sudo apt-get install x11vnc websockify -y");
+		exec($aptWrap("sudo apt-get install x11vnc websockify -y"));
 		exec("sudo git clone https://github.com/novnc/noVNC.git /usr/share/novnc");
 		exec("sudo chown -R dietpi:dietpi /usr/share/novnc");
 		exec("sudo systemctl enable mupi_vnc.service");
@@ -33,29 +46,29 @@
 	if( $_POST['change_samba'] == "enable & start" )
 		{
 		$command = "sudo apt-get install samba -y && sudo wget https://raw.githubusercontent.com/splitti/MuPiBox/main/config/templates/smb.conf -O /etc/samba/smb.conf && sudo systemctl enable smbd.service && sudo systemctl start smbd.service";
-		exec($command, $output, $result );
+		exec($aptWrap($command), $output, $result );
 		$change=1;
 		$CHANGE_TXT=$CHANGE_TXT."<li>Samba enabled</li>";
 		}
 	else if( $_POST['change_samba'] == "stop & disable" )
 		{
 		$command = "sudo systemctl stop smbd.service && sudo systemctl disable smbd.service && sudo apt-get remove samba -y";
-		exec($command, $output, $result );
+		exec($aptWrap($command), $output, $result );
 		$change=1;
 		$CHANGE_TXT=$CHANGE_TXT."<li>Samba disabled</li>";
 		}
 
 	if( $_POST['change_ftp'] == "enable & start" )
 		{
 		$command = " sudo apt-get install proftpd -y && sudo apt-get install samba -y && sudo wget https://raw.githubusercontent.com/splitti/MuPiBox/main/config/templates/proftpd.conf -O /etc/proftpd/proftpd.conf && sudo systemctl restart proftpd";
-		exec($command, $output, $result );
+		exec($aptWrap($command), $output, $result );
 		$change=1;
 		$CHANGE_TXT=$CHANGE_TXT."<li>FTP enabled</li>";
 		}
 	else if( $_POST['change_ftp'] == "stop & disable" )
 		{
 		$command = "sudo systemctl stop proftpd.service && sudo systemctl disable proftpd.service && sudo apt-get remove proftpd -y";
-		exec($command, $output, $result );
+		exec($aptWrap($command), $output, $result );
 		$change=1;
 		$CHANGE_TXT=$CHANGE_TXT."<li>FTP disabled</li>";
 		}

config/services/mupi_change_checker.service

@@ -3,6 +3,8 @@ Description=Check for changes in media directory
 
 [Service]
 ExecStart=/usr/local/bin/mupibox/./change_checker.sh
+Restart=on-failure
+RestartSec=10
 
 [Install]
 WantedBy=basic.target

config/services/mupi_check_internet.service

@@ -5,6 +5,8 @@ Description=Checks Internet Connection
 User=dietpi
 Group=dietpi
 ExecStart=/usr/local/bin/mupibox/./check_network.sh
+Restart=on-failure
+RestartSec=10
 
 [Install]
 WantedBy=multi-user.target

config/services/mupi_idle_shutdown.service

@@ -3,6 +3,8 @@ Description=Idle Shutdown Timer
 
 [Service]
 ExecStart=/usr/local/bin/mupibox/./idle_shutdown.sh
+Restart=on-failure
+RestartSec=10
 
 [Install]
 WantedBy=basic.target

config/services/mupi_telegram.service

@@ -5,6 +5,8 @@ After=network-online.target
 
 [Service]
 ExecStart=/usr/local/bin/mupibox/./telegram_start.sh
+Restart=on-failure
+RestartSec=10
 
 [Install]
 WantedBy=default.target

scripts/librespot/librespot-start.sh

@@ -16,7 +16,13 @@ if ! $cachestate ; then
     export LIBRESPOT_NAME_DISABLE_AUDIO_CACHE=1
 fi
 username=$( jq -r .username ${LIBRESPOT_CACHE}/credentials.json )
-if [[ ! -z "$var" ]]; then
+# LOW-1: previous test was `[[ ! -z "$var" ]]` — `$var` was never set
+# anywhere in this script, so the test was always false (or always true
+# depending on shell semantics for unset-vs-empty), and LIBRESPOT_USERNAME
+# never got exported even when credentials.json had a real username.
+# Test the actual username variable, plus filter "null" since jq -r emits
+# that literal for missing keys.
+if [[ -n "$username" && "$username" != "null" ]]; then
   export LIBRESPOT_USERNAME=${username}
 fi
 

scripts/mupibox/add_index.sh

@@ -18,6 +18,14 @@ if [ -f "${DATA_LOCK}" ]; then
     exit
 else
 	touch ${DATA_LOCK}
+	# HIGH-15: previous code had no trap, so a perl/jq crash, a SIGTERM
+	# from systemd, or an out-of-disk during the temp writes left
+	# DATA_LOCK behind. Subsequent invocations would see the stale lock
+	# and exit, blocking m3u_generator's index pass forever — observable
+	# as "new audiobook folder shows up but never gets an index, so
+	# resume can't address it." Trap the common termination signals so
+	# the lock is removed even when the script dies mid-write.
+	trap 'rm -f "${DATA_LOCK}"' EXIT INT TERM HUP
 
 	/usr/bin/cat ${DATA} | grep -v '"index":' > ${TMP_DATA}
     /usr/bin/perl -pe 'BEGIN{$k=-1};s/{/$& . "\n        \"index\": " .  ++$k . ","/e' ${TMP_DATA} > ${DATA}
@@ -26,5 +34,6 @@ else
     /usr/bin/mv ${TMP_DATA} ${DATA}
     /usr/bin/chown dietpi:dietpi ${DATA}
 	echo "Index is finished"
-	rm ${DATA_LOCK}
+	# Lock removal happens via the EXIT trap above; the explicit rm here
+	# is now redundant but kept to preserve the original log-line ordering.
 fi

scripts/mupibox/add_wifi.sh

@@ -22,8 +22,16 @@ while true
 do
 	if test -f "${MUPIWIFI}"
 	then
-		SSID="$(/usr/bin/jq -r .[].ssid ${MUPIWIFI})"
-		PSK="$(/usr/bin/jq -r .[].pw ${MUPIWIFI})"
+		# HIGH-11: previous filter was `jq -r .[].ssid` which iterates and
+		# emits N newline-separated SSIDs when the array has N entries.
+		# wpa_passphrase / the "network={ ssid=…" emit below then receive
+		# a multi-line string and write garbage into wpa_supplicant.conf.
+		# The frontend always queues exactly one entry per save event, so
+		# pin to index 0; if there are leftover entries they're picked up
+		# on the next loop iteration after `sudo rm ${MUPIWIFI}` clears
+		# the file.
+		SSID="$(/usr/bin/jq -r '.[0].ssid // empty' ${MUPIWIFI})"
+		PSK="$(/usr/bin/jq -r '.[0].pw // empty' ${MUPIWIFI})"
 		if [ "${SSID}" = "" ]
 		then
 			sudo rm ${MUPIWIFI}
@@ -44,11 +52,23 @@ do
 			restart_network			
 		elif [ "${PSK}" = "" ]
 		then
-			echo 'network={' | sudo tee -a ${WPACONF}
-			echo '	ssid="'${SSID}'"' | sudo tee -a ${WPACONF}
-			echo '	scan_ssid=1' | sudo tee -a ${WPACONF}
-			echo '}' | sudo tee -a ${WPACONF}
-			restart_network
+			# HIGH-12: previous code spliced ${SSID} unquoted into a
+			# single-quoted echo, so an SSID containing `"` or a newline
+			# could escape the quoted string and inject arbitrary blocks
+			# into wpa_supplicant.conf (root-owned, security-relevant).
+			# Use wpa_passphrase's open-network shape via printf with %s,
+			# which can't be reinterpreted by the shell, then escape any
+			# embedded `"` for the JSON-style ssid field.
+			# Reject SSIDs containing characters wpa_supplicant.conf can't
+			# represent (newlines / NUL) outright.
+			if [[ "${SSID}" == *$'\n'* || "${SSID}" == *$'\0'* ]]; then
+				echo "add_wifi.sh: SSID rejected (newline / NUL in name)" >&2
+			else
+				_ESCAPED_SSID="${SSID//\\/\\\\}"
+				_ESCAPED_SSID="${_ESCAPED_SSID//\"/\\\"}"
+				printf 'network={\n\tssid="%s"\n\tscan_ssid=1\n}\n' "${_ESCAPED_SSID}" | sudo tee -a ${WPACONF} >/dev/null
+				restart_network
+			fi
 		else
 			WIFI_RESULT=$(sudo -i wpa_passphrase "${SSID}" "${PSK}") 
 			IFS=$'\n'

scripts/mupibox/albumstop.sh

@@ -5,14 +5,26 @@
 ALBUMSTOP_FILE="/home/dietpi/.mupibox/Sonos-Kids-Controller-master/server/config/albumstop.json"
 
 
+# Atomic-update pattern (HIGH-8): write jq output to a same-dir tempfile,
+# then mv it onto the target. The previous `cat <<< $(jq …) > FILE`
+# truncated FILE before jq finished — jq errors or partial output left
+# the JSON empty and bricked the next boot. With write-then-rename, jq
+# either succeeds and the rename atomically swaps in the new content, or
+# it fails and the original file stays intact (we rm the half-written
+# tempfile so /tmp doesn't fill up).
 if [ ! -f ${ALBUMSTOP_FILE} ]; then
-        sudo echo -n "{}" ${ALBUMSTOP_FILE}
-        chown dietpi:dietpi ${ALBUMSTOP_FILE}
-        /usr/bin/cat <<< $(/usr/bin/jq -n --arg v "Off" '.albumStop = $v' ${ALBUMSTOP_FILE}) >  ${ALBUMSTOP_FILE}
+        # HIGH-14 (Phase-3) + Phase-5 follow-up: ALBUMSTOP_FILE lives
+        # under /home/dietpi/.../config/ which is dietpi-writable; drop
+        # the sudo+tee that produced a root-owned file the script
+        # couldn't subsequently replace.
+        echo -n "{}" > "${ALBUMSTOP_FILE}"
+        _TMP="${ALBUMSTOP_FILE}.tmp.$$"
+        /usr/bin/jq -n --arg v "Off" '.albumStop = $v' > "${_TMP}" && mv "${_TMP}" "${ALBUMSTOP_FILE}" || rm -f "${_TMP}"
         sleep 10
         sudo bash /usr/local/bin/mupibox/shutdown.sh
 else
-        /usr/bin/cat <<< $(/usr/bin/jq --arg v "Off" '.albumStop = $v' ${ALBUMSTOP_FILE}) >  ${ALBUMSTOP_FILE}
+        _TMP="${ALBUMSTOP_FILE}.tmp.$$"
+        /usr/bin/jq --arg v "Off" '.albumStop = $v' "${ALBUMSTOP_FILE}" > "${_TMP}" && mv "${_TMP}" "${ALBUMSTOP_FILE}" || rm -f "${_TMP}"
         sleep 10
         sudo bash /usr/local/bin/mupibox/shutdown.sh
 fi

scripts/mupibox/albumstop_activator.sh

@@ -4,10 +4,14 @@
 
 ALBUMSTOP_FILE="/home/dietpi/.mupibox/Sonos-Kids-Controller-master/server/config/albumstop.json"
 
+# Atomic-update pattern (HIGH-8): same as albumstop.sh.
 if [ ! -f ${ALBUMSTOP_FILE} ]; then
-        sudo echo -n "{}" ${ALBUMSTOP_FILE}
-        chown dietpi:dietpi ${ALBUMSTOP_FILE}
-        /usr/bin/cat <<< $(/usr/bin/jq -n --arg v "On" '.albumStop = $v' ${ALBUMSTOP_FILE}) >  ${ALBUMSTOP_FILE}
+        # HIGH-14 (Phase-3) + Phase-5 follow-up: same as albumstop.sh —
+        # drop sudo, dietpi can write the destination directly.
+        echo -n "{}" > "${ALBUMSTOP_FILE}"
+        _TMP="${ALBUMSTOP_FILE}.tmp.$$"
+        /usr/bin/jq -n --arg v "On" '.albumStop = $v' > "${_TMP}" && mv "${_TMP}" "${ALBUMSTOP_FILE}" || rm -f "${_TMP}"
 else
-        /usr/bin/cat <<< $(/usr/bin/jq --arg v "On" '.albumStop = $v' ${ALBUMSTOP_FILE}) >  ${ALBUMSTOP_FILE}
+        _TMP="${ALBUMSTOP_FILE}.tmp.$$"
+        /usr/bin/jq --arg v "On" '.albumStop = $v' "${ALBUMSTOP_FILE}" > "${_TMP}" && mv "${_TMP}" "${ALBUMSTOP_FILE}" || rm -f "${_TMP}"
 fi

scripts/mupibox/change_checker.sh

@@ -4,6 +4,14 @@
 
 CONFIG="/etc/mupibox/mupiboxconfig.json"
 CHECK_TIMER=$(/usr/bin/jq -r .mupibox.mediaCheckTimer ${CONFIG})
+# MED-22: a fresh image without `mediaCheckTimer` in mupiboxconfig.json
+# gets `null` from jq -r, so `sleep null` returns immediately and the
+# while loop pegs one CPU at 100% — observable on a fresh box as the
+# system fan ramping up before the first config save. Default to 60s
+# (same cadence m3u_generator.sh runs at on real boxes).
+if [ -z "${CHECK_TIMER}" ] || [ "${CHECK_TIMER}" = "null" ]; then
+  CHECK_TIMER=60
+fi
 
 while true
 do