Skip to content

fix(healthcheck): guard periodic checks against divide-by-zero when config is cleared#3400

Open
mvanhorn wants to merge 1 commit into
passteque:masterfrom
mvanhorn:fix/3398-healthcheck-divide-by-zero
Open

fix(healthcheck): guard periodic checks against divide-by-zero when config is cleared#3400
mvanhorn wants to merge 1 commit into
passteque:masterfrom
mvanhorn:fix/3398-healthcheck-divide-by-zero

Conversation

@mvanhorn

Copy link
Copy Markdown

Description

The healthcheck goroutine can panic with panic: runtime error: integer divide by zero (stack at internal/healthcheck/checker.go in fullPeriodicCheck).

fullPeriodicCheck indexed the shared config slice directly with c.tlsDialAddrs[try%len(c.tlsDialAddrs)]. Checker.Stop() clears the config by setting c.tlsDialAddrs = nil (and icmpTargetIPs / smallCheckType) — but did so without holding c.configMutex. A fullPeriodicCheck still in flight during or just after Stop() then evaluates try % len(c.tlsDialAddrs) with len == 0 and panics. It is also a data race: fullPeriodicCheck read c.tlsDialAddrs without the mutex while Stop/SetConfig wrote it. smallPeriodicCheck already copies c.icmpTargetIPs under the mutex, but its icmpTargetIPs[try%len(icmpTargetIPs)] had the same latent divide-by-zero when that copy is empty (Stop nils icmpTargetIPs too).

This change:

  • Stop() now takes c.configMutex while clearing the config, synchronizing the clear with the readers.
  • fullPeriodicCheck() copies c.tlsDialAddrs into a local slice under c.configMutex (mirroring smallPeriodicCheck) and returns early when the copy is empty — there is nothing to dial — instead of dividing by zero. The check closure indexes the local copy.
  • smallPeriodicCheck() returns early when the (already-copied) ICMP target slice is empty and the check type is not DNS, so the copied-slice modulo cannot divide by zero either. The DNS-fallback path is unchanged.

Behavior is identical for a normally-running checker with non-empty config; the guards only affect the teardown / empty-config window.

Verified with go test ./internal/healthcheck/..., go test -race ./internal/healthcheck/..., go build ./..., and gofmt -l (clean). A regression test exercises the periodic checks with empty target slices and asserts they no longer panic.

Issue

Closes #3398

Assertions

  • I am aware that any changes to settings should be reflected in the wiki — N/A, this change adds no settings and does not alter existing settings.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: panic: runtime error: integer divide by zero

1 participant