From 5b1e13ca3d4ff0ea85ace7fec390ab80b26434cf Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Sat, 12 Sep 2026 11:47:16 +0700 Subject: [PATCH] fleetd #519 review fix: move the parser comments with the code they explain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #523 extracted parse_policy_fields() but left about 25 lines of explanatory comments at the old parse site in main(). That is the same defect class as fleetd #500 itself — a stated fact that no longer matches the code next to it — in the very file whose ticket history is about it. Three blocks moved, no code touched: * "One parse pass" + the mapfile/process-substitution reasoning now sits above parse_policy_fields(), which is what it describes. * The arity-check block now sits inside the function, directly above `if (( ${#_FIELDS[@]} < 5 ))`. At the old site it said "the slice just below this" and "every line below this expects", both pointing at a function call rather than the check. Reworded to name main() and its slice explicitly. * The pipefail note said the parser failure was "handled below"; the handling is now above it, in the function. The call site keeps a three-line pointer saying where the reasoning went. Checked myself, on this revision: * suite exit 0, "PASS: probe member credentials guards" * bash -n rc=0 under /bin/bash 3.2.57 and bash 5.3.9 * two mutations killed, each proven applied two ways (mutant present AND original gone), restored byte-identical, green control after each: - dropping the empty-parse special case -> FAIL: empty parser output count - arity threshold 5 -> 0 -> FAIL: short parser output status --- scripts/probe-member-credentials.sh | 55 +++++++++++++++-------------- 1 file changed, 29 insertions(+), 26 deletions(-) diff --git a/scripts/probe-member-credentials.sh b/scripts/probe-member-credentials.sh index 5108cad..f65a8a5 100755 --- a/scripts/probe-member-credentials.sh +++ b/scripts/probe-member-credentials.sh @@ -64,6 +64,23 @@ # The two outputs side by side are the finding: any name whose hash matches between them is a # credential the member holds in full. # +# One parse pass: line 1 = present (true/false/null), line 2 = policy mode (possibly blank), +# lines 3-5 = knownCount/allowedCount/blockedCount, remaining lines = the known[] names. A single +# pass avoids re-parsing (and re-risking a truthiness bug) five separate times. +# +# This used to feed the parser straight into `mapfile -t _FIELDS < <(producer)`. That form cannot +# see the producer fail: `<` `<(...)` is a process substitution, not a pipeline, so `set -o +# pipefail` does not reach inside it, and mapfile's own exit status reports whether the BUILTIN +# ran, not whether the command substituted into it succeeded — a failing jq or python3 there still +# leaves mapfile at rc=0 with an empty array, read as a parse that genuinely found nothing (fleetd +# #500). Capturing the parser's output with command substitution first, and checking ITS exit +# status, reports the producer's real failure while the fact still exists — before it is handed to +# mapfile at all. +# +# mapfile then reads from that captured string with `<<<` (a herestring), not `< <(...)`: `<<<` +# materialises the whole string in memory first, where `< <(...)` would stream it. That only +# matters for a large producer; this one is a short credential-name policy response, so the +# tradeoff is irrelevant here — noted because it would not be for every producer. parse_policy_fields() { if command -v jq >/dev/null 2>&1; then _FIELDS_RAW="$(printf '%s' "$POLICY_JSON" | jq -r ' @@ -107,6 +124,14 @@ PY mapfile -t _FIELDS <<< "$_FIELDS_RAW" fi + # Arity check — the CORRECTNESS fix (fleetd #500). A parser that exits 0 can still return fewer + # than the 5 fixed fields (present, policy mode, 3 counts) that every fixed-field read in main() expects, + # whatever the reason: a producer that printed nothing, malformed JSON that jq/python3 still + # accepted, or a schema change upstream. main()'s slice (`_FIELDS[@]:5`) does not fire + # `set -u` on an unset OR a short array, and every fixed-field read there used a `:-` default, so + # without this check a short `_FIELDS` reaches the "0 known names" guard further down with the + # same look as a policy that genuinely has 0 names. Check the count here, at the one point the + # fact is still present, before the slice consumes it. if (( ${#_FIELDS[@]} < 5 )); then echo "refusing to run: the policy parser ($_PARSER_NAME) returned ${#_FIELDS[@]} field(s); at" \ "least 5 are required (present, policy mode, knownCount, allowedCount, blockedCount). The" \ @@ -118,7 +143,7 @@ PY main() { set -uo pipefail -# `pipefail` is not what catches the parser failure handled below (fleetd #500): in +# `pipefail` is not what catches the parser failure handled in parse_policy_fields() above (fleetd #500): in # `printf '%s' "$POLICY_JSON" | jq -r '...'`, jq is the LAST element of the pipe, so the pipeline's # own exit status is already jq's status, with or without pipefail. It is kept as insurance for if # a post-processing stage is ever appended after the parser (e.g. `| tail -n +2`) — at that point @@ -195,31 +220,9 @@ EOF exit 1 fi -# One parse pass: line 1 = present (true/false/null), line 2 = policy mode (possibly blank), -# lines 3-5 = knownCount/allowedCount/blockedCount, remaining lines = the known[] names. A single -# pass avoids re-parsing (and re-risking a truthiness bug) five separate times. -# -# This used to feed the parser straight into `mapfile -t _FIELDS < <(producer)`. That form cannot -# see the producer fail: `<` `<(...)` is a process substitution, not a pipeline, so `set -o -# pipefail` does not reach inside it, and mapfile's own exit status reports whether the BUILTIN -# ran, not whether the command substituted into it succeeded — a failing jq or python3 there still -# leaves mapfile at rc=0 with an empty array, read as a parse that genuinely found nothing (fleetd -# #500). Capturing the parser's output with command substitution first, and checking ITS exit -# status, reports the producer's real failure while the fact still exists — before it is handed to -# mapfile at all. -# -# mapfile then reads from that captured string with `<<<` (a herestring), not `< <(...)`: `<<<` -# materialises the whole string in memory first, where `< <(...)` would stream it. That only -# matters for a large producer; this one is a short credential-name policy response, so the -# tradeoff is irrelevant here — noted because it would not be for every producer. -# Arity check — the CORRECTNESS fix (fleetd #500). A parser that exits 0 can still return fewer -# than the 5 fixed fields (present, policy mode, 3 counts) that every line below this expects, -# whatever the reason: a producer that printed nothing, malformed JSON that jq/python3 still -# accepted, or a schema change upstream. The slice just below this (`_FIELDS[@]:5`) does not fire -# `set -u` on an unset OR a short array, and every fixed-field read above used a `:-` default, so -# without this check a short `_FIELDS` reaches the "0 known names" guard further down with the -# same look as a policy that genuinely has 0 names. Check the count here, at the one point the -# fact is still present, before the slice consumes it. +# Parse the policy in one pass and refuse on any of the three failure causes. The decision, the +# three refusals and the reasoning behind each live in parse_policy_fields() above — kept there +# with the code rather than here, so the explanation cannot drift away from what it explains. parse_policy_fields || exit $? PRESENT="${_FIELDS[0]:-null}"