From 92cdad0315d42b4d9bc36d1cd7aa1752a31f2db3 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Fri, 6 Jan 2023 13:07:05 -0500 Subject: [PATCH] Fix a potential defer logic error around locking in several top-level API functions. These are the first line of the function that contains them, which makes sense; we want to capture any error returned by the function. However, making this the first defer means that it is the last thing to run after the function returns - meaning that the container's `defer c.lock.Unlock()` has already fired, leading to a chance we modify the container without holding its lock. We could move the function around so it's no longer the first defer, but then we'd have to call it twice (immediately after `defer c.lock.Unlock()` if the container is not batched, and a second time in a new `else` block right after the lock/sync call to make sure we handle batched containers). Seems simpler to just leave it like this. [NO NEW TESTS NEEDED] Can't really test for DB corruption easily. Signed-off-by: Matthew Heon --- libpod/container_api.go | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/libpod/container_api.go b/libpod/container_api.go index d9bd4ea8b4..2397342077 100644 --- a/libpod/container_api.go +++ b/libpod/container_api.go @@ -85,6 +85,15 @@ func (c *Container) Init(ctx context.Context, recursive bool) error { func (c *Container) Start(ctx context.Context, recursive bool) (finalErr error) { defer func() { if finalErr != nil { + // Have to re-lock. + // As this is the first defer, it's the last thing to + // happen in the function - so `defer c.lock.Unlock()` + // below already fired. + if !c.batched { + c.lock.Lock() + defer c.lock.Unlock() + } + if err := saveContainerError(c, finalErr); err != nil { logrus.Debug(err) } @@ -125,6 +134,15 @@ func (c *Container) Update(res *spec.LinuxResources) error { func (c *Container) StartAndAttach(ctx context.Context, streams *define.AttachStreams, keys string, resize <-chan resize.TerminalSize, recursive bool) (retChan <-chan error, finalErr error) { defer func() { if finalErr != nil { + // Have to re-lock. + // As this is the first defer, it's the last thing to + // happen in the function - so `defer c.lock.Unlock()` + // below already fired. + if !c.batched { + c.lock.Lock() + defer c.lock.Unlock() + } + if err := saveContainerError(c, finalErr); err != nil { logrus.Debug(err) } @@ -212,6 +230,15 @@ func (c *Container) Stop() error { func (c *Container) StopWithTimeout(timeout uint) (finalErr error) { defer func() { if finalErr != nil { + // Have to re-lock. + // As this is the first defer, it's the last thing to + // happen in the function - so `defer c.lock.Unlock()` + // below already fired. + if !c.batched { + c.lock.Lock() + defer c.lock.Unlock() + } + if err := saveContainerError(c, finalErr); err != nil { logrus.Debug(err) }