fix races in the HTTP attach API

This is very similar to commit 3280da0, 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 <[email protected]>

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)

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)
 	}