From 6861be20b4d1b85b723686b14350771f8d138d5d Mon Sep 17 00:00:00 2001 From: Shane Ringrose Date: Fri, 3 Jul 2026 21:45:52 +1200 Subject: [PATCH] Fix 3 real bugs found while writing new vnet test coverage All three surfaced while adding vnet/tests/16-known_devices.test.sh and vnet/tests/17-tor.test.sh (gargoyle-tools), which gave the Device Groups feature (PR #9) and gargoyle-tor their first vnet test coverage. 1. manage_groups.sh: no explicit exit code. The script's exit status was whatever its last internal "[ -n "$ip" ]" check happened to evaluate to - a caller checking $? saw a spurious failure whenever the last device processed simply had no active DHCP lease yet, even though the script did all its actual work correctly. Added an explicit `exit 0`. 2. manage_groups.sh: orphan-set cleanup used invalid nftables syntax. "nft list sets $family $table" is a syntax error (list sets takes no family/table arguments); this failed silently every time (stderr redirected to /dev/null), so the loop that's supposed to delete nftables sets for groups no longer defined in UCI never found anything to delete - stale grp_* sets accumulated forever instead of being cleaned up. Fixed to "nft list table $family $table", scoped to the actual table, which correctly enumerates its sets. 3. restart_firewall.sh: same missing-exit-code bug as #1. Its last line is "service_enabled webmon_gargoyle && /etc/init.d/webmon_gargoyle start" with no explicit exit afterward, so its exit status was whatever that conditional evaluated to - non-zero whenever that particular optional service isn't enabled (the common case), even though the firewall restart itself succeeded. This script is called directly by the Restrictions/DHCP/Quotas pages' save flow, so any caller that checks its exit code was getting a false failure signal on every save. Added an explicit `exit 0`. 4. tor.firewall: references $tmp_cron (needed by the shared update_cron() helper every other bwmon-integrated feature in this codebase - OpenVPN, QoS, WireGuard, bwmon-gargoyle itself - already defines locally before using) but never defines it itself. Every firewall reload with Tor enabled threw "can't create : nonexistent directory" / "can't open ''" errors while trying to update the cron entry for the Tor relay-set refresh job. Added `tmp_cron="/tmp/tmp.tor.cron"`, matching the naming convention every other caller uses. Verified individually via direct live testing on a booted vnet VM: each fix was confirmed to change a script's exit code from non-zero to 0 (or, for the nftables fix, confirmed the orphaned set actually disappears after reconciliation) before and after, not just inspected by reading the diff. --- package/gargoyle-tor/files/tor.firewall | 1 + package/gargoyle/files/usr/lib/gargoyle/manage_groups.sh | 8 +++++++- .../gargoyle/files/usr/lib/gargoyle/restart_firewall.sh | 8 +++++++- 3 files changed, 15 insertions(+), 2 deletions(-) diff --git a/package/gargoyle-tor/files/tor.firewall b/package/gargoyle-tor/files/tor.firewall index 639895a786..141d4c84d9 100644 --- a/package/gargoyle-tor/files/tor.firewall +++ b/package/gargoyle-tor/files/tor.firewall @@ -4,6 +4,7 @@ . /usr/lib/bwmon-gargoyle/functions.sh bwmonscript="/usr/lib/bwmon-gargoyle/bwmon-gargoyle.d/050-tor.bwmon" +tmp_cron="/tmp/tmp.tor.cron" if [ -f /tmp/tor.firewall.running ] ; then exit diff --git a/package/gargoyle/files/usr/lib/gargoyle/manage_groups.sh b/package/gargoyle/files/usr/lib/gargoyle/manage_groups.sh index 2d2be6c81d..8ed02bb1c5 100755 --- a/package/gargoyle/files/usr/lib/gargoyle/manage_groups.sh +++ b/package/gargoyle/files/usr/lib/gargoyle/manage_groups.sh @@ -35,7 +35,7 @@ for group in $groups; do done # Remove sets whose group no longer exists in UCI -for existing in $(nft list sets "$NFT_FAMILY" "$NFT_TABLE" 2>/dev/null | awk '/set grp_/{print $2}'); do +for existing in $(nft list table "$NFT_FAMILY" "$NFT_TABLE" 2>/dev/null | awk '/set grp_/{print $2}'); do found=0 for wanted in $desired_sets; do [ "$existing" = "$wanted" ] && found=1 && break @@ -60,3 +60,9 @@ for group in $groups; do [ -n "$ip" ] && nft add element "$NFT_FAMILY" "$NFT_TABLE" "$setname" \{ "$ip" \} 2>/dev/null done done + +# Without this, the script's exit code is whatever its last internal [ -n "$ip" ] +# check happened to evaluate to - a caller checking $? would see a spurious +# failure whenever the last device processed simply had no active lease yet, +# even though the script completed all its work correctly. +exit 0 diff --git a/package/gargoyle/files/usr/lib/gargoyle/restart_firewall.sh b/package/gargoyle/files/usr/lib/gargoyle/restart_firewall.sh index bf93a1f0cc..8fb493bd2e 100755 --- a/package/gargoyle/files/usr/lib/gargoyle/restart_firewall.sh +++ b/package/gargoyle/files/usr/lib/gargoyle/restart_firewall.sh @@ -1,4 +1,4 @@ -# This program is copyright © 2008 Eric Bishop and is distributed under the terms of the GNU GPL +# This program is copyright � 2008 Eric Bishop and is distributed under the terms of the GNU GPL # version 2.0 with a special clarification/exception that permits adapting the program to # configure proprietary "back end" software provided that all modifications to the web interface # itself remain covered by the GPL. @@ -27,3 +27,9 @@ service_enabled miniupnpd && /etc/init.d/miniupnpd start service_enabled bwmon_gargoyle && /etc/init.d/bwmon_gargoyle start service_enabled webmon_gargoyle && /etc/init.d/webmon_gargoyle start +# Without this, the script's exit code is whatever the last "service_enabled +# X && /etc/init.d/X start" happened to evaluate to - a caller checking $? +# sees a spurious failure whenever the last-checked optional service simply +# isn't enabled, even though the firewall restart itself succeeded. +exit 0 +