Repository: moby/moby (language: go)

Issue:
Goroutine panic and mutex leak in `getInspectData`
### Description

A goroutine panic can happen in `getInspectData` if the inspected container is being concurrently removed. Goroutine panics while holding container's mutex so subsequent operations that require the mutex (like inspecting the affected container) hang indefinitely. The container is stuck in "Removal In Progress" state.

AFAIU the scenario is that:
* goroutine a: `cleanupContainer` marks container as dead and releases the mutex ([here](https://github.com/moby/moby/blob/v27.4.1/daemon/delete.go#L134))
* goroutine b: `getInspectData` is called while the mutex is hold ([here](https://github.com/moby/moby/blob/v27.4.1/daemon/inspect.go#L60))
* goroutine b: `getInspectData` performs `container.RWLayer` `nil` check ([here](https://github.com/moby/moby/blob/v27.4.1/daemon/inspect.go#L192))
* goroutine a: `cleanupContainer` removes the layer and sets `container.RWLayer` to `nil` **without acquiring the mutex** ([here](https://github.com/moby/moby/blob/v27.4.1/daemon/delete.go#L144))
* goroutine b: `getInspectData` calls `container.RWLayer.Metadata()` trying to dereference nil, the goroutine panics, the mutex is leaked ([here](https://github.com/moby/moby/blob/v27.4.1/daemon/inspect.go#L199))

I found that a similar (the same?) issue was discovered before and fixed in https://github.com/moby/moby/pull/36242. The fix does not work in this scenario because `container.RWLayer` can be modified right after the `nil` check by `cleanupContainer`.

The fix I tried that works is to acquire the mutex for setting `container.RWLayer = nil` in `cleanupContainer`. Simply copying `container.RWLayer` to a local variable prior to the `nil` check does not work, as go seem to be optimizing out the local reference..., anyway, the mutex approach seem to be the right one. I'll provide a PR in a moment.

Panic stacktrace:
```
2025/01/05 02:35:53 http: panic serving @: runtime error: invalid memory address or nil pointer dereference
goroutine 487 [running]:
net/http.(*conn).serve.func1()
        /usr/local/go/src/net/http/server.go:1903 +0xbe
panic({0x61447a2a4820?, 0x61447bc57760?})
        /usr/local/go/src/runtime/panic.go:770 +0x132
github.com/docker/docker/layer.(*referencedRWLayer).Metadata(0xc00a366dc0?)
        <autogenerated>:1 +0x17
github.com/docker/docker/daemon.(*Daemon).getInspectData(0xc0001ef688, 0xc00058e008, 0xc00a158a08)
        /root/build-deb/engine/daemon/inspect.go:199 +0xf38
github.com/docker/docker/daemon.(*Daemon).ContainerInspectCurrent(0xc0001ef688, {0x61447a757060, 0xc00153d2f0}, {0xc00976a326?, 0xc0057aade0?}, 0x0)
        /root/build-deb/engine/daemon/inspect.go:60 +0xce
github.com/docker/docker/daemon.(*Daemon).ContainerInspect(0xc0001ef688, {0x61447a757060, 0xc00153d2f0}, {0xc00976a326, 0x40}, 0x0, {0xc00976a316?, 0x614477c896e5?})
        /root/build-deb/engine/daemon/inspect.go:32 +0xac
github.com/docker/docker/api/server/router/container.(*containerRouter).getContainersByName(0xc002143080, {0x61447a757060, 0xc00153d2f0}, {0x61447a74fe00, 0xc0018cc7e0}, 0xc00509d170?, 0xc00153cd80)
        /root/build-deb/engine/api/server/router/container/inspect.go:15 +0xcb
github.com/docker/docker/api/server/middleware.(*ExperimentalMiddleware).WrapHandler.ExperimentalMiddleware.WrapHandler.func1({0x61447a757060, 0xc00153d2f0}, {0x61447a74fe00, 0xc0018cc7e0}, 0xc006b58120, 0xc00153cd80)
        /root/build-deb/engine/api/server/middleware/experimental.go:26 +0xa3
github.com/docker/docker/api/server/middleware.(*VersionMiddleware).WrapHandler.VersionMiddleware.WrapHandler.func1({0x61447a757060, 0xc00153d290}, {0x61447a74fe00, 0xc0018cc7e0}, 0xc006b58120, 0xc00153cd80)
        /root/build-deb/engine/api/server/middleware/version.go:84 +0x2b3
github.com/docker/docker/pkg/authorization.(*Middleware).WrapHandler.func1({0x61447a757060, 0xc00153d290}, {0x61447a74fe00, 0xc0018cc7e0}, 0xc006b58120, 0xc00153cd80)
        /root/build-deb/engine/pkg/authorization/middleware.go:59 +0x34a
github.com/docker/docker/api/server.(*Server).makeHTTPHandler.func1({0x61447a74fe00, 0xc0018cc7e0}, 0xc0009cdb00)
        /root/build-deb/engine/api/server/server.go:56 +0x1ae
net/http.HandlerFunc.ServeHTTP(0x61447a757060?, {0x61447a74fe00?, 0xc0018cc7e0?}, 0x614479f47050?)
        /usr/local/go/src/net/http/server.go:2171 +0x29
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp.(*middleware).serveHTTP(0xc000bb4b00, {0x61447a7487f0, 0xc006b9c620}, 0xc0009ccd80, {0x61447a72ad80, 0xc002196180})
        /root/build-deb/engine/vendor/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/handler.go:229 +0x1243
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp.NewMiddleware.func1.1({0x61447a7487f0?, 0xc006b9c620?}, 0x614479f3f201?)
        /root/build-deb/engine/vendor/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/handler.go:81 +0x35
net/http.HandlerFunc.ServeHTTP(0x61447a757060?, {0x61447a7487f0?, 0xc006b9c620?}, 0x614479f3f238?)
        /usr/local/go/src/net/http/server.go:2171 +0x29
net/http.HandlerFunc.ServeHTTP(0xc000829e60?, {0x61447a7487f0?, 0xc006b9c620?}, 0x614477d751cf?)
        /usr/local/go/src/net/http/server.go:2171 +0x29
github.com/gorilla/mux.(*Router).ServeHTTP(0xc00039e9c0, {0x61447a7487f0, 0xc006b9c620}, 0xc009c93e60)
        /root/build-deb/engine/vendor/github.com/gorilla/mux/mux.go:212 +0x1e2
net/http.serverHandler.ServeHTTP({0xc000f58030?}, {0x61447a7487f0?, 0xc006b9c620?}, 0x6?)
        /usr/local/go/src/net/http/server.go:3142 +0x8e
net/http.(*conn).serve(0xc000bfa750, {0x61447a757060, 0xc000fccf90})
        /usr/local/go/src/net/http/server.go:2044 +0x5e8
created by net/http.(*Server).Serve in goroutine 611
        /usr/local/go/src/net/http/server.go:3290 +0x4b4
```

### Reproduce

To trigger this race condition reliably:

* run dockerd with an artificial delay right before `graphDriverData, err := container.RWLayer.Metadata()` (it's the use that is done right after `nil` check) in `getInspectData`, e.g. by inserting a loop (`time.Sleep` would also work, but obtained stack would be different from what I seen, as generated assembly would change a bit - without `<autogenerated>:1` frame)  
  ```
  (...)
  for i := 0; i < 1000000000; i++ {
  }
  graphDriverData, err := rwLayer.Metadata()
  (...)
  ```
* start a container: `docker run --rm -t -i --name test ubuntu:24.04`
* in background inspect it in a loop: `while true; do docker inspect test; done`
* exit the container

The panic happens reliably with exactly the same stack. Commands that require container lock, like `docker inspect test`, hung.


### Expected behavior

No panic and no mutex leak.


### docker version

```bash
Client: Docker Engine - Community
 Version:           27.4.1
 API version:       1.47
 Go version:        go1.22.10
 Git commit:        b9d17ea
 Built:             Tue Dec 17 15:45:46 2024
 OS/Arch:           linux/amd64
 Context:           default

Server: Docker Engine - Community
 Engine:
  Version:          27.4.1
  API version:      1.47 (minimum version 1.24)
  Go version:       go1.22.10
  Git commit:       c710b88
  Built:            Tue Dec 17 15:45:46 2024
  OS/Arch:          linux/amd64
  Experimental:     false
 containerd:
  Version:          1.7.24
  GitCommit:        88bf19b2105c8b17560993bee28a01ddc2f97182
 runc:
  Version:          1.2.2
  GitCommit:        v1.2.2-0-g7cb3632
 docker-init:
  Version:          0.19.0
  GitCommit:        de40ad0
```

### docker info

```bash
Client: Docker Engine - Community
 Version:    27.4.1
 Context:    default
 Debug Mode: false
 Plugins:
  buildx: Docker Buildx (Docker Inc.)
    Version:  v0.19.3
    Path:     /usr/libexec/docker/cli-plugins/docker-buildx
  compose: Docker Compose (Docker Inc.)
    Version:  v2.32.1
    Path:     /usr/libexec/docker/cli-plugins/docker-compose

Server:
 Containers: 10
  Running: 9
  Paused: 0
  Stopped: 1
 Images: 1093
 Server Version: 27.4.1
 Storage Driver: overlay2
  Backing Filesystem: extfs
  Supports d_type: true
  Using metacopy: false
  Native Overlay Diff: true
  userxattr: false
 Logging Driver: json-file
 Cgroup Driver: systemd
 Cgroup Version: 2
 Plugins:
  Volume: local
  Network: bridge host ipvlan macvlan null overlay
  Log: awslogs fluentd gcplogs gelf journald json-file local splunk syslog
 Swarm: inactive
 Runtimes: io.containerd.runc.v2 runc
 Default Runtime: runc
 Init Binary: docker-init
 containerd version: 88bf19b2105c8b17560993bee28a01ddc2f97182
 runc version: v1.2.2-0-g7cb3632
 init version: de40ad0
 Security Options:
  apparmor
  seccomp
   Profile: builtin
  cgroupns
 Kernel Version: 6.8.0-1018-oem
 Operating System: Ubuntu 24.04.1 LTS
 OSType: linux
 Architecture: x86_64
 CPUs: 128
 Total Memory: 125.6GiB
 Name: hostname
 ID: a97209c3-4df2-4440-8e3b-9300990077bb
 Docker Root Dir: /var/lib/docker
 Debug Mode: false
 Experimental: false
 Insecure Registries:
  127.0.0.0/8
 Live Restore Enabled: false
```

### Additional Info

_No response_


Discussion / hints:

Fix patch:
diff --git a/daemon/delete.go b/daemon/delete.go
index f93a89aa1858d..244f7acfa9032 100644
--- a/daemon/delete.go
+++ b/daemon/delete.go
@@ -125,6 +125,10 @@ func (daemon *Daemon) cleanupContainer(container *container.Container, config ba
 	container.Lock()
 	container.Dead = true
 
+	// Copy RWLayer for releasing and clear the reference while holding the container lock.
+	rwLayer := container.RWLayer
+	container.RWLayer = nil
+
 	// Save container state to disk. So that if error happens before
 	// container meta file got removed from disk, then a restart of
 	// docker should not make a dead container alive.
@@ -135,13 +139,17 @@ func (daemon *Daemon) cleanupContainer(container *container.Container, config ba
 
 	// When container creation fails and `RWLayer` has not been created yet, we
 	// do not call `ReleaseRWLayer`
-	if container.RWLayer != nil {
-		if err := daemon.imageService.ReleaseLayer(container.RWLayer); err != nil {
+	if rwLayer != nil {
+		if err := daemon.imageService.ReleaseLayer(rwLayer); err != nil {
+			// Restore the reference on error as it possibly was not released.
+			container.Lock()
+			container.RWLayer = rwLayer
+			container.Unlock()
+
 			err = errors.Wrapf(err, "container %s", container.ID)
 			container.SetRemovalError(err)
 			return err
 		}
-		container.RWLayer = nil
 	} else {
 		if daemon.UsesSnapshotter() {
 			ls := daemon.containerdClient.LeasesService()


Test patch: