From bf74797c6906cf9c47a71c13caeff8f771639258 Mon Sep 17 00:00:00 2001
From: Paul Holzinger <pholzing@redhat.com>
Date: Tue, 27 Aug 2024 11:45:13 +0200
Subject: [PATCH] fix races in the HTTP attach API

This is very similar to commit 3280da0500, we cannot check the state
then unlock to then lock again and do the action. Everything must
happen under one lock. To fix this move the code into the HTTPAttach
function in libpod. The locking here is a bit weird because attach
blocks for the lifetime of attach which can be very long so we must
unlock before performing the attach.

Fixes #23757

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
---
 libpod/container_api.go                      | 28 +++++++++++++++-----
 pkg/api/handlers/compat/containers_attach.go | 18 -------------
 2 files changed, 21 insertions(+), 25 deletions(-)

diff --git a/libpod/container_api.go b/libpod/container_api.go
index 8a309c4cf9..02e35199ba 100644
--- a/libpod/container_api.go
+++ b/libpod/container_api.go
@@ -40,7 +40,10 @@ func (c *Container) Init(ctx context.Context, recursive bool) error {
 			return err
 		}
 	}
+	return c.initUnlocked(ctx, recursive)
+}
 
+func (c *Container) initUnlocked(ctx context.Context, recursive bool) error {
 	if !c.ensureState(define.ContainerStateConfigured, define.ContainerStateStopped, define.ContainerStateExited) {
 		return fmt.Errorf("container %s has already been created in runtime: %w", c.ID(), define.ErrCtrStateInvalid)
 	}
@@ -342,25 +345,36 @@ func (c *Container) HTTPAttach(r *http.Request, w http.ResponseWriter, streams *
 		close(hijackDone)
 	}()
 
+	locked := false
 	if !c.batched {
+		locked = true
 		c.lock.Lock()
+		defer func() {
+			if locked {
+				c.lock.Unlock()
+			}
+		}()
 		if err := c.syncContainer(); err != nil {
-			c.lock.Unlock()
-
 			return err
 		}
-		// We are NOT holding the lock for the duration of the function.
-		c.lock.Unlock()
 	}
-
-	if !c.ensureState(define.ContainerStateCreated, define.ContainerStateRunning) {
-		return fmt.Errorf("can only attach to created or running containers: %w", define.ErrCtrStateInvalid)
+	// For Docker compatibility, we need to re-initialize containers in these states.
+	if c.ensureState(define.ContainerStateConfigured, define.ContainerStateExited, define.ContainerStateStopped) {
+		if err := c.initUnlocked(r.Context(), c.config.Pod != ""); err != nil {
+			return fmt.Errorf("preparing container %s for attach: %w", c.ID(), err)
+		}
+	} else if !c.ensureState(define.ContainerStateCreated, define.ContainerStateRunning) {
+		return fmt.Errorf("can only attach to created or running containers - currently in state %s: %w", c.state.State.String(), define.ErrCtrStateInvalid)
 	}
 
 	if !streamAttach && !streamLogs {
 		return fmt.Errorf("must specify at least one of stream or logs: %w", define.ErrInvalidArg)
 	}
 
+	// We are NOT holding the lock for the duration of the function.
+	locked = false
+	c.lock.Unlock()
+
 	logrus.Infof("Performing HTTP Hijack attach to container %s", c.ID())
 
 	c.newContainerEvent(events.Attach)
diff --git a/pkg/api/handlers/compat/containers_attach.go b/pkg/api/handlers/compat/containers_attach.go
index 301be134b1..6dacdb29ee 100644
--- a/pkg/api/handlers/compat/containers_attach.go
+++ b/pkg/api/handlers/compat/containers_attach.go
@@ -4,11 +4,9 @@ package compat
 
 import (
 	"errors"
-	"fmt"
 	"net/http"
 
 	"github.com/containers/podman/v5/libpod"
-	"github.com/containers/podman/v5/libpod/define"
 	"github.com/containers/podman/v5/pkg/api/handlers/utils"
 	"github.com/containers/podman/v5/pkg/api/server/idle"
 	api "github.com/containers/podman/v5/pkg/api/types"
@@ -79,22 +77,6 @@ func AttachContainer(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	state, err := ctr.State()
-	if err != nil {
-		utils.InternalServerError(w, err)
-		return
-	}
-	// For Docker compatibility, we need to re-initialize containers in these states.
-	if state == define.ContainerStateConfigured || state == define.ContainerStateExited || state == define.ContainerStateStopped {
-		if err := ctr.Init(r.Context(), ctr.PodID() != ""); err != nil {
-			utils.Error(w, http.StatusConflict, fmt.Errorf("preparing container %s for attach: %w", ctr.ID(), err))
-			return
-		}
-	} else if !(state == define.ContainerStateCreated || state == define.ContainerStateRunning) {
-		utils.InternalServerError(w, fmt.Errorf("can only attach to created or running containers - currently in state %s: %w", state.String(), define.ErrCtrStateInvalid))
-		return
-	}
-
 	logErr := func(e error) {
 		logrus.Errorf("Error attaching to container %s: %v", ctr.ID(), e)
 	}