From 9b79e486464d4d505a9f81772a415a30b06016ba Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 11 Sep 2025 13:00:53 +0200 Subject: [PATCH] cli/command/container: prevent panic during stats on empty event Actor.ID This code was missing a check for the ID field before truncating it to a shorter length for presentation. This would result in a panic if an event would either have an empty ID field or a shorter length ID; panic: runtime error: slice bounds out of range [:12] with length 0 goroutine 82 [running]: github.com/docker/cli/cli/command/container.RunStats.func2({{0x0, 0x0}, {0x0, 0x0}, {0x0, 0x0}, {0x40001fcba0, 0x9}, {0x40001fcba9, 0x5}, ...}) /go/src/github.com/docker/cli/cli/command/container/stats.go:146 +0x1d0 created by github.com/docker/cli/cli/command/container.(*eventHandler).watch in goroutine 6 /go/src/github.com/docker/cli/cli/command/container/stats.go:363 +0x1c8 We need to look at this code in general; the truncated ID is passed to NewStats, which uses the ID to propagate the `Container` field in the `StatsEntry` struct. which is not used in the default format used by `docker stats` and, having the same content as the `ID` field on the same struct, doesn't make it very useful, other than being able to present it under a `CONTAINER` column (instead of `CONTAINER ID`); we should consider deprecating it; there may be some subtle things to look into here; the `Container` field originally held the container name. This was changed in [moby@ef915fd], which introduced separate `ID` and `Name` fields, renaming the old `Name` field to container. Looking at [`Stats.SetStatistics()`] and related code in [stats_helpers.go], the `Container` field is used as the "canonical" reference for the stats record; this allows the stats _data_ to be refreshed when a new stats sample arrives for the same container (also see [moby@929a77b], which moved locking to the `Stats` wrapper struct). This construct allows to account for intermediate states, where a stats sample was incomplete or could produce an error; in that case, the reference to the container for which the stats were sampled is kept to allow removing a container from the list once the container was removed. We should consider removing `Container` as a formatting option, and moving the `Container` field to the outer struct; this makes the outer struct responsible for keeping a reference to the container, allowing the `StatsEntry` as a whole to be replaced atomically. This patch only addresses the panic; - It changes the logic to preserve the container ID verbatim instead of truncating. This allows stats samples to be matched against the `Actor.ID` as-is. - Truncating the `Container` is moved to the presentation logic; currently this does not take `--no-trunc` into account to keep the existing behavior, but we can (should) consider adding this. - Logging is improved to use structured logs, and an extra check is added to prevent empty IDs from being added as watcher. [`Stats.SetStatistics()`]: https://github.com/docker/cli/blob/82281087e3e186c5a2eafa0d973e849ff84c357d/cli/command/container/formatter_stats.go#L88-L94 [moby@ef915fd]: https://github.com/moby/moby/commit/ef915fd036d9ea5263f9370dce490ef97ea0618d [moby@929a77b]: https://github.com/moby/moby/commit/929a77b814dfe9ab7a11bffc2d16eebd27bd903a [stats_helpers.go]: https://github.com/docker/cli/blob/82281087e3e186c5a2eafa0d973e849ff84c357d/cli/command/container/stats_helpers.go#L26-L51 Signed-off-by: Sebastiaan van Stijn --- cli/command/container/formatter_stats.go | 1 + cli/command/container/stats.go | 15 ++++++++++----- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/cli/command/container/formatter_stats.go b/cli/command/container/formatter_stats.go index 48371eedc..626c63142 100644 --- a/cli/command/container/formatter_stats.go +++ b/cli/command/container/formatter_stats.go @@ -167,6 +167,7 @@ func (c *statsContext) Container() string { } func (c *statsContext) Name() string { + // TODO(thaJeztah): make this explicitly trim the "/" prefix, not just any char. if len(c.s.Name) > 1 { return c.s.Name[1:] } diff --git a/cli/command/container/stats.go b/cli/command/container/stats.go index 00c8cf6b5..a0f90bb13 100644 --- a/cli/command/container/stats.go +++ b/cli/command/container/stats.go @@ -134,7 +134,7 @@ func RunStats(ctx context.Context, dockerCLI command.Cli, options *StatsOptions) eh := newEventHandler() if options.All { eh.setHandler(events.ActionCreate, func(e events.Message) { - s := NewStats(e.Actor.ID[:12]) + s := NewStats(e.Actor.ID) if cStats.add(s) { waitFirst.Add(1) go collect(ctx, s, apiClient, !options.NoStream, waitFirst) @@ -143,7 +143,7 @@ func RunStats(ctx context.Context, dockerCLI command.Cli, options *StatsOptions) } eh.setHandler(events.ActionStart, func(e events.Message) { - s := NewStats(e.Actor.ID[:12]) + s := NewStats(e.Actor.ID) if cStats.add(s) { waitFirst.Add(1) go collect(ctx, s, apiClient, !options.NoStream, waitFirst) @@ -152,7 +152,7 @@ func RunStats(ctx context.Context, dockerCLI command.Cli, options *StatsOptions) if !options.All { eh.setHandler(events.ActionDie, func(e events.Message) { - cStats.remove(e.Actor.ID[:12]) + cStats.remove(e.Actor.ID) }) } @@ -205,7 +205,7 @@ func RunStats(ctx context.Context, dockerCLI command.Cli, options *StatsOptions) return err } for _, ctr := range cs { - s := NewStats(ctr.ID[:12]) + s := NewStats(ctr.ID) if cStats.add(s) { waitFirst.Add(1) go collect(ctx, s, apiClient, !options.NoStream, waitFirst) @@ -358,7 +358,12 @@ func (eh *eventHandler) watch(c <-chan events.Message) { if !exists { continue } - logrus.Debugf("event handler: received event: %v", e) + if e.Actor.ID == "" { + logrus.WithField("event", e).Errorf("event handler: received %s event with empty ID", e.Action) + continue + } + + logrus.WithField("event", e).Debugf("event handler: received %s event for: %s", e.Action, e.Actor.ID) go h(e) } }