From a602054826b45e8add40f2ef6fd37aa4d66dca0f Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 15 Oct 2024 14:20:13 +0200 Subject: [PATCH] daemon: fix restoring containers with name matching an ID This patch fixes a bug in the daemon's restore step on startup, where a container with a name matching another container's ID would not be restored. `Daemon.registerName` is used during startup as part of the daemon's container restore code https://github.com/moby/moby/blob/97b1233a15c86afa627ffbb60748b530bc3d19bb/daemon/daemon.go#L331-L344 In that process, it first registers the containers names through [`Daemon.registerName()`][1], then registers the container's ID through [`Daemon.Register()`][1], which calls `Daemon.containers.Add()` under the hood. Restoring containers is done in a goroutine, and at this stage of the daemon's lifecycle, not all containers may be restored yet. However, `Daemon.registerName()` has some safeguard to prevent the same container from being restored _twice_ through [`Daemon.Exists()`][3]. If a duplicate is found, an error is logged, and the container is not restored (but kept on disk). While it's disputable if this logic is needed at all, perhaps a panic would be more appropriate (duplicate containers were stored on disk), there's also a flaw in the current implementation of this check. The [`Daemon.Exists()`][3] function uses [`Daemon.GetContainer()`][4] to look up the container. This function performs fuzzy matching on the given reference, first trying to match containers on their full ID, which _should_ not give a match at this stage, before falling back to matching containers by name and partial prefix. This last part can be problematic in situations where a container exists that uses the container to restore's ID as name. In such cases, the container will be considered "already present", and not restored. Create a container, then create a number of containers, each of which using the ID of the previous container as name. docker create --name one hello-world d54301b7560f3c3544acc2d9c9dd55a194d6db37c2af64fe83fa34238c7ce6ab docker create --name d54301b7560f3c3544acc2d9c9dd55a194d6db37c2af64fe83fa34238c7ce6ab hello-world 217c53b9826eb7875ca2620596864d039848470befeb5f963b3ebffe509e7a6d docker create --name 217c53b9826eb7875ca2620596864d039848470befeb5f963b3ebffe509e7a6d hello-world b125af485d6d1875b586b314f93af1b49d5baaa94cec4199ae4ef4c6da05e7e4 docker create --name b125af485d6d1875b586b314f93af1b49d5baaa94cec4199ae4ef4c6da05e7e4 hello-world The daemon should now have a number of containers where the ID and name conflict: docker ps -a --no-trunc --format 'table {{.ID}}\t{{.Names}}' CONTAINER ID NAMES f59e8e4044471c45d4c9841d11a2c586cbfa4703b1344035fd51a15e15899ea7 b125af485d6d1875b586b314f93af1b49d5baaa94cec4199ae4ef4c6da05e7e4 b125af485d6d1875b586b314f93af1b49d5baaa94cec4199ae4ef4c6da05e7e4 217c53b9826eb7875ca2620596864d039848470befeb5f963b3ebffe509e7a6d 217c53b9826eb7875ca2620596864d039848470befeb5f963b3ebffe509e7a6d d54301b7560f3c3544acc2d9c9dd55a194d6db37c2af64fe83fa34238c7ce6ab d54301b7560f3c3544acc2d9c9dd55a194d6db37c2af64fe83fa34238c7ce6ab one Restart the daemon. Depending on the order in which containers are restored, a conflict may happen, and the conflicting container will not be restored. Logs below are from the daemon with debug enabled; INFO[2024-10-15T11:13:38.770744797Z] Loading containers: start. DEBU[2024-10-15T11:13:38.771152214Z] processing event stream module=libcontainerd namespace=moby DEBU[2024-10-15T11:13:38.771599797Z] loaded container container=d54301b7560f3c3544acc2d9c9dd55a194d6db37c2af64fe83fa34238c7ce6ab paused=false running=false DEBU[2024-10-15T11:13:38.771637464Z] loaded container container=217c53b9826eb7875ca2620596864d039848470befeb5f963b3ebffe509e7a6d paused=false running=false DEBU[2024-10-15T11:13:38.771672714Z] loaded container container=bbe03a6554867810c2d7464ed3cb853865c755bae797b8d1f4caf60fb3f9fa04 paused=false running=false DEBU[2024-10-15T11:13:38.771765297Z] loaded container container=f59e8e4044471c45d4c9841d11a2c586cbfa4703b1344035fd51a15e15899ea7 paused=false running=false DEBU[2024-10-15T11:13:38.771780839Z] loaded container container=b125af485d6d1875b586b314f93af1b49d5baaa94cec4199ae4ef4c6da05e7e4 paused=false running=false ERRO[2024-10-15T11:13:38.772114505Z] failed to register container name: /217c53b9826eb7875ca2620596864d039848470befeb5f963b3ebffe509e7a6d container=b125af485d6d1875b586b314f93af1b49d5baaa94cec4199ae4ef4c6da05e7e4 error="container is already loaded" And the conflicting container (`217c53b9826eb7875ca2620596864d039848470befeb5f963b3ebffe509e7a6d`) is not present: docker ps -a --no-trunc --format 'table {{.ID}}\t{{.Names}}' CONTAINER ID NAMES f59e8e4044471c45d4c9841d11a2c586cbfa4703b1344035fd51a15e15899ea7 b125af485d6d1875b586b314f93af1b49d5baaa94cec4199ae4ef4c6da05e7e4 b125af485d6d1875b586b314f93af1b49d5baaa94cec4199ae4ef4c6da05e7e4 217c53b9826eb7875ca2620596864d039848470befeb5f963b3ebffe509e7a6d d54301b7560f3c3544acc2d9c9dd55a194d6db37c2af64fe83fa34238c7ce6ab one [1]: https://github.com/moby/moby/blob/97b1233a15c86afa627ffbb60748b530bc3d19bb/daemon/names.go#L22-L38 [2]: https://github.com/moby/moby/blob/97b1233a15c86afa627ffbb60748b530bc3d19bb/daemon/container.go#L106-L121 [3]: https://github.com/moby/moby/blob/97b1233a15c86afa627ffbb60748b530bc3d19bb/daemon/container.go#L71-L76 [4]: https://github.com/moby/moby/blob/97b1233a15c86afa627ffbb60748b530bc3d19bb/daemon/container.go#L30-L69 Signed-off-by: Sebastiaan van Stijn --- daemon/names.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/daemon/names.go b/daemon/names.go index 14cefc07fd9fd..9ce40e24a1d2b 100644 --- a/daemon/names.go +++ b/daemon/names.go @@ -23,7 +23,9 @@ func (daemon *Daemon) registerName(container *container.Container) error { if container.ID == "" { return fmt.Errorf("invalid empty id") } - if daemon.Exists(container.ID) { + if daemon.containers.Get(container.ID) != nil { + // TODO(thaJeztah): should this be a panic (duplicate IDs due to invalid state on disk?) + // TODO(thaJeztah): should this also check for container.ID being a prefix of another container's ID? (daemon.containersReplica.GetByPrefix); only should happen due to corruption / truncated ID. return fmt.Errorf("container is already loaded") } if container.Name == "" {