locking issue in podman healtcheck

[Luap99]

# podman [options] healthcheck run c-h-tRvxOJ
           panic: operation not permitted
         
           goroutine 1 [running]:
           github.com/containers/podman/v5/libpod/lock.(*SHMLock).Unlock(0xc00044c0a0?)
           	/var/tmp/go/src/github.com[/containers/podman/libpod/lock/shm_lock_manager_linux.go:135](https://github.com/containers/podman/blob/6a080328552664d1ae234a3980fe401daa669d02/libpod/lock/shm_lock_manager_linux.go#L135) +0x48
           github.com/containers/podman/v5/libpod.(*Container).cleanupExecBundle(0xc00044c0a0, {0xc00015cf80?, 0xc0000774e0?})
           	/var/tmp/go/src/github.com[/containers/podman/libpod/container_exec.go:993](https://github.com/containers/podman/blob/6a080328552664d1ae234a3980fe401daa669d02/libpod/container_exec.go#L993) +0x14c
           github.com/containers/podman/v5/libpod.(*Container).healthCheckExec.func2()
           	/var/tmp/go/src/github.com[/containers/podman/libpod/container_exec.go:862](https://github.com/containers/podman/blob/6a080328552664d1ae234a3980fe401daa669d02/libpod/container_exec.go#L862) +0x2b
           github.com/containers/podman/v5/libpod.(*Container).healthCheckExec(0xc00044c0a0, 0xc00002d4a0, 0xb2d05e00, 0xc00024fb90)
           	/var/tmp/go/src/github.com[/containers/podman/libpod/container_exec.go:888](https://github.com/containers/podman/blob/6a080328552664d1ae234a3980fe401daa669d02/libpod/container_exec.go#L888) +0x785
           github.com/containers/podman/v5/libpod.(*Container).runHealthCheck(0xc00044c0a0, {0x1f729d0, 0x2d62160}, 0x0)
           	/var/tmp/go/src/github.com[/containers/podman/libpod/healthcheck.go:100](https://github.com/containers/podman/blob/6a080328552664d1ae234a3980fe401daa669d02/libpod/healthcheck.go#L100) +0x679
           github.com/containers/podman/v5/libpod.(*Runtime).HealthCheck(0x0?, {0x1f729d0, 0x2d62160}, {0x7fffe8e585bb?, 0xa?})
           	/var/tmp/go/src/github.com[/containers/podman/libpod/healthcheck.go:45](https://github.com/containers/podman/blob/6a080328552664d1ae234a3980fe401daa669d02/libpod/healthcheck.go#L45) +0x185
           github.com/containers/podman/v5/pkg/domain/infra/abi.(*ContainerEngine).HealthCheckRun(0x0?, {0x1f729d0?, 0x2d62160?}, {0x7fffe8e585bb?, 0xaaac029ce99440d5?}, {})
           	/var/tmp/go/src/github.com[/containers/podman/pkg/domain/infra/abi/healthcheck.go:13](https://github.com/containers/podman/blob/6a080328552664d1ae234a3980fe401daa669d02/pkg/domain/infra/abi/healthcheck.go#L13) +0x29
           github.com/containers/podman/v5/cmd/podman/healthcheck.run(0x2c48960?, {0xc000609970?, 0x1?, 0x1?})
           	/var/tmp/go/src/github.com[/containers/podman/cmd/podman/healthcheck/run.go:34](https://github.com/containers/podman/blob/6a080328552664d1ae234a3980fe401daa669d02/cmd/podman/healthcheck/run.go#L34) +0x55
           github.com/spf13/cobra.(*Command).execute(0x2c48960, {0xc000102fb0, 0x1, 0x1})
           	/var/tmp/go/src/github.com[/containers/podman/vendor/github.com/spf13/cobra/command.go:1015](https://github.com/containers/podman/blob/6a080328552664d1ae234a3980fe401daa669d02/vendor/github.com/spf13/cobra/command.go#L1015) +0xaaa
           github.com/spf13/cobra.(*Command).ExecuteC(0x2c44fa0)
           	/var/tmp/go/src/github.com[/containers/podman/vendor/github.com/spf13/cobra/command.go:1148](https://github.com/containers/podman/blob/6a080328552664d1ae234a3980fe401daa669d02/vendor/github.com/spf13/cobra/command.go#L1148) +0x46f
           github.com/spf13/cobra.(*Command).Execute(...)
           	/var/tmp/go/src/github.com[/containers/podman/vendor/github.com/spf13/cobra/command.go:1071](https://github.com/containers/podman/blob/6a080328552664d1ae234a3980fe401daa669d02/vendor/github.com/spf13/cobra/command.go#L1071)
           github.com/spf13/cobra.(*Command).ExecuteContext(...)
           	/var/tmp/go/src/github.com[/containers/podman/vendor/github.com/spf13/cobra/command.go:1064](https://github.com/containers/podman/blob/6a080328552664d1ae234a3980fe401daa669d02/vendor/github.com/spf13/cobra/command.go#L1064)
           main.Execute()
           	/var/tmp/go/src/github.com[/containers/podman/cmd/podman/root.go:120](https://github.com/containers/podman/blob/6a080328552664d1ae234a3980fe401daa669d02/cmd/podman/root.go#L120) +0x57
           main.main()
           	/var/tmp/go/src/github.com[/containers/podman/cmd/podman/main.go:74](https://github.com/containers/podman/blob/6a080328552664d1ae234a3980fe401daa669d02/cmd/podman/main.go#L74) +0x4f2

https://api.cirrus-ci.com/v1/artifact/task/6171517366566912/html/int-podman-debian-13-root-host-sqlite.log.html#t--Podman-healthcheck-run-podman-healthcheck-health-timeout--1

If I read the stack trace correctly we end up unlocking twice, the parent is already unlocked but then there is another Unlock() in cleanupExecBundle() which is invalid.

[baude]

nice catch @Luap99 ... i guess the question is whether we spot fix this or we examine our locks and implement some sort of internal check for the existence of a lock and the performance ramifications of that. @mheon you have done a lot of the lock work, wdyt ?

[mheon]

This was a deliberate design choice when we wrote the SHM locks - double-unlock panics, as it does for other Go locks. I don't see a reason to change this. Double unlocking is a very bad code smell and not something we want to just handle and try and recover from - odds are we'll end up with state corruption if we continue.

The biggest problem is functions that most of our internal functions are built around a "the container is always locked" assumption, and we then introduce functions like healthCheckExec() that unlock the container for a time to unblock other operations. Add a defer or two and you have a recipe for disaster. @Luap99 mentioned an idea for a Rust-like approach to locking to me at one point (have c.lock() accept a function that is done under the lock, with it automatically unlocking on return) but I don't know if that'd address this fully given the complexity of the lock -> operations -> unlock -> long operation -> relock workflow we have here.

[Luap99]

Yeah I still have plan to rewrite our locking API's as part of my university work once I get my stuff sorted out.