Skip to content

read_manifest conflates an unreadable manifest with an empty one #33

Description

@twistedmelonman

Summary

read_manifest cannot distinguish "the manifest lists no domains" from "the
manifest could not be read". Both produce an empty array, and callers treat
that as "nothing is managed".

Mechanism

read_manifest() {
  if [[ -f "$MANIFEST_FILE" ]]; then
    grep -E '^[^#[:space:]]' "$MANIFEST_FILE" || true
  fi
}

grep exits 1 when it matches nothing and 2 on an actual error — unreadable
file, I/O failure. The || true swallows both identically, and the [[ -f ]]
test passes for a file that exists but cannot be read (bad permissions).

Impact

Two callers act on the empty result:

  • do_uninstall (~line 522): warns "No managed resolver files found" and exits
    cleanly, leaving managed resolver files in place. The user is told there was
    nothing to remove.
  • main (~line 610), computing MANIFEST_DOMAINS: the diff against
    DESIRED_DOMAINS sees zero managed domains, so nothing is scheduled for
    removal and stale managed files are orphaned.

Neither is a crash; both are silent wrong answers, which is the harder failure
to notice.

Suggested fix

Distinguish the exit codes, and fail loudly on a genuine read error — the
repo's own convention is fatal for unrecoverable state:

read_manifest() {
  [[ -f "$MANIFEST_FILE" ]] || return 0
  local out status
  out="$(grep -E '^[^#[:space:]]' "$MANIFEST_FILE")" && status=0 || status=$?
  (( status <= 1 )) || fatal "Cannot read manifest: ${MANIFEST_FILE}"
  [[ -n "$out" ]] && printf '%s\n' "$out"
  return 0
}

Callers would then need || fatal rather than || true, since the function
could fail meaningfully.

Provenance

Pre-existing; not introduced by #. Found while making the repo
shellcheck -S info clean for standards-check (dev-env#100). That PR added
|| true at the four mapfile < <(...) call sites to satisfy SC2312 — those
are inert, because process substitution never propagates status under set -e
and these callees already always exit 0. This issue is about the inner
|| true, which is load-bearing and does mask a real error.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions