Skip to content

[grug] Elder FP: shell command flagged as unchecked when if ! IS the check #771

Description

@quadseven

Why

Elder raised missing-error-handling on production/homedns/deploy/keepalived/check-dnsdist.sh:4
(quadseven/infra#1988), saying the dig result was unchecked. The whole file is:

#!/bin/bash
# keepalived track_script - 0 healthy, 1 fail
set -euo pipefail
if ! dig @127.0.0.1 google.com +time=1 +tries=1 +short >/dev/null 2>&1; then
  exit 1
fi
exit 0

The if ! is the exit-status check, and set -euo pipefail backs it. A
failed dig takes the exit 1 branch, which is exactly the keepalived
track_script contract. There is no unchecked path. This is a clean false
positive from pattern-matching "command invocation" without reading the
surrounding control flow.

Distinct from #763 (registry-vs-allowlist conflation) - that is a semantic FP,
this is a syntactic/control-flow one.

Why it matters more than one bad finding

FPs are expensive asymmetrically. A human who rebuts a finding spends real time,
and after two or three they start skimming - at which point the true positives in
the same batch get skimmed too. On this PR Elder produced 11 findings, 10 genuine
and 1 FP; that ratio is good, but the FP was the FIRST one I read closely, which
is the worst position for it.

What

Teach the shell rules control-flow awareness. A command should NOT be flagged as
unchecked when it appears in any of:

  • if cmd; then / if ! cmd; then / elif cmd; then
  • while cmd; do / until cmd; do
  • cmd && ... / cmd || ...
  • as the condition of a case on $?
  • with an explicit || true / || exit N / || return N
  • assigned via if out=$(cmd); then

And weight set -e / set -euo pipefail at file scope as a global signal that
bare invocations ARE checked by default - which inverts the rule for the whole
file.

The cheapest robust fix is probably delegating to shellcheck, which already
models all of this correctly, rather than growing bespoke regex rules. That
overlaps #681 (run OSS static analyzers and ground findings in their output) -
this issue is a concrete, high-value instance of why #681 pays off.

Acceptance criteria

  • The check-dnsdist.sh file above produces ZERO findings.
  • A genuinely unchecked dig ... on its own line in a file with no set -e
    still produces a finding (do not just make the rule deaf).
  • Regression fixtures for each control-flow form listed above.

Size: S

Refs #707

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions