kubernetes-sigs / kubernetes-sigs/logtools

logcheck: detect nil-unsafe fmt.Stringer pointers passed as structured log values

Open
#34 3 comments 0 reactions 1 assignee Claimed by @Mujib-Ahasan View on GitHub
priority/backlog triage/accepted
Dominant language
Go
Stars
18
Forks
20
Avg merge
5d 4h
Merged PRs (30d)
1

Description

### Feature request: flag nil-unsafe `fmt.Stringer` pointers passed as structured log values

**The bug class**: passing a `*T` value to a structured logging call (`logger.Info("msg", "key", ptr)`) where `T` implements `fmt.Stringer` with a value receiver. The logger stringifies the value by calling `String()` through the pointer; because the receiver is a value, a nil pointer panics unconditionally. klog recovers ([`StringerToString`](https://github.com/kubernetes/klog/blob/v2.140.0/internal/serialize/keyvalues.go#L224-L234)) and renders:

```
notAfter=""
```

The prototypical offender is `*metav1.Time` (embeds `time.Time`, whose [`String` has a value receiver](https://cs.opensource.google/go/go/+/go1.24.0:src/time/format.go;l=517)) — precisely the case `klog.SafePtr` was added for (kubernetes/klog#393, allowlisted in #27 here).

**Real-world impact**: in cert-manager this output was repeatedly mistaken for a controller crash and kept an issue alive for two years at priority/critical-urgent (cert-manager/cert-manager#6799, fixed by cert-manager/cert-manager#9117). A quick repo-wide prototype of this check found a second, latent instance in a different controller (cert-manager/cert-manager#9120) — the nil was logged in exactly the error-explaining branch where it could be nil.

**Proposed check** (fits the `parameters` family): for each value position of a structured logging call, if the static type is `*T` and `T` (the element type, i.e. the value method set) implements `fmt.Stringer`, report and suggest wrapping in `klog.SafePtr`. Checking the *element* type keeps precision high: it flags only the promoted/value-receiver case that always panics, not pointer-receiver `String` implementations that may nil-check (e.g. `*net.IPNet`). In cert-manager the prototype produced 3 findings: 1 real bug, 2 technically-never-nil pointers where `SafePtr` is harmless.

logcheck already knows how to locate key/value positions in these calls, which is the hard part — we first prototyped this with gocritic's ruleguard and needed one rule per (method × argument position) to work around matcher limitations, which is too fragile to maintain in-repo.

The same idea could extend to `error` values (`Error()` is also called through the pointer), possibly behind a flag.

Also filed with golangci-lint's loggercheck: https://github.com/timonwong/loggercheck/issues/109

*with claude fable-5*

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.