From 5bb8fa30b04e08761df6b412e7ef3c7cc0970650 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Thu, 4 Mar 2021 16:22:41 -0500 Subject: Do not return from c.stop() before re-locking Unlocking an already unlocked lock is a panic. As such, we have to make sure that the deferred c.lock.Unlock() in c.StopWithTimeout() always runs on a locked container. There was a case in c.stop() where we could return an error after we unlock the container to stop it, but before we re-lock it - thus allowing for a double-unlock to occur. Fix the error return to not happen until after the lock has been re-acquired. Fixes #9615 Signed-off-by: Matthew Heon --- libpod/container_internal.go | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) (limited to 'libpod') diff --git a/libpod/container_internal.go b/libpod/container_internal.go index 7e8226de4..bace18825 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -1307,9 +1307,7 @@ func (c *Container) stop(timeout uint) error { c.lock.Unlock() } - if err := c.ociRuntime.StopContainer(c, timeout, all); err != nil { - return err - } + stopErr := c.ociRuntime.StopContainer(c, timeout, all) if !c.batched { c.lock.Lock() @@ -1318,13 +1316,23 @@ func (c *Container) stop(timeout uint) error { // If the container has already been removed (e.g., via // the cleanup process), there's nothing left to do. case define.ErrNoSuchCtr, define.ErrCtrRemoved: - return nil + return stopErr default: + if stopErr != nil { + logrus.Errorf("Error syncing container %s status: %v", c.ID(), err) + return stopErr + } return err } } } + // We have to check stopErr *after* we lock again - otherwise, we have a + // change of panicing on a double-unlock. Ref: GH Issue 9615 + if stopErr != nil { + return stopErr + } + // Since we're now subject to a race condition with other processes who // may have altered the state (and other data), let's check if the // state has changed. If so, we should return immediately and log a -- cgit v1.2.3-54-g00ecf