Merge pull request #23766 from Luap99/http-attach

fix races in the HTTP attach API

cmd/podman/containers/cleanup.go

@@ -53,6 +53,11 @@ func init() {
 
 	flags.BoolVar(&cleanupOptions.Remove, "rm", false, "After cleanup, remove the container entirely")
 	flags.BoolVar(&cleanupOptions.RemoveImage, "rmi", false, "After cleanup, remove the image entirely")
+
+	stoppedOnlyFlag := "stopped-only"
+	flags.BoolVar(&cleanupOptions.StoppedOnly, stoppedOnlyFlag, false, "Only cleanup when the container is in the stopped state")
+	_ = flags.MarkHidden(stoppedOnlyFlag)
+
 	validate.AddLatestFlag(cleanupCommand, &cleanupOptions.Latest)
 }
 

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()
 	}
-
+	// For Docker compatibility, we need to re-initialize containers in these states.
-	if !c.ensureState(define.ContainerStateCreated, define.ContainerStateRunning) {
+	if c.ensureState(define.ContainerStateConfigured, define.ContainerStateExited, define.ContainerStateStopped) {
-		return fmt.Errorf("can only attach to created or running containers: %w", define.ErrCtrStateInvalid)
+		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)
@@ -771,8 +785,10 @@ func (c *Container) WaitForConditionWithInterval(ctx context.Context, waitTimeou
 }
 
 // Cleanup unmounts all mount points in container and cleans up container storage
-// It also cleans up the network stack
+// It also cleans up the network stack.
-func (c *Container) Cleanup(ctx context.Context) error {
+// onlyStopped is set by the podman container cleanup to ensure we only cleanup a stopped container,
+// all other states mean another process already called cleanup before us which is fine in such cases.
+func (c *Container) Cleanup(ctx context.Context, onlyStopped bool) error {
 	if !c.batched {
 		c.lock.Lock()
 		defer c.lock.Unlock()
@@ -794,6 +810,9 @@ func (c *Container) Cleanup(ctx context.Context) error {
 	if !c.ensureState(define.ContainerStateConfigured, define.ContainerStateCreated, define.ContainerStateStopped, define.ContainerStateStopping, define.ContainerStateExited) {
 		return fmt.Errorf("container %s is running or paused, refusing to clean up: %w", c.ID(), define.ErrCtrStateInvalid)
 	}
+	if onlyStopped && !c.ensureState(define.ContainerStateStopped) {
+		return fmt.Errorf("container %s is not stopped and only cleanup for a stopped container was requested: %w", c.ID(), define.ErrCtrStateInvalid)
+	}
 
 	// if the container was not created in the oci runtime or was already cleaned up, then do nothing
 	if c.ensureState(define.ContainerStateConfigured, define.ContainerStateExited) {

libpod/pod_api.go

@@ -180,7 +180,7 @@ func (p *Pod) stopWithTimeout(ctx context.Context, cleanup bool, timeout int) (m
 			}
 
 			if cleanup {
-				err := c.Cleanup(ctx)
+				err := c.Cleanup(ctx, false)
 				if err != nil && !errors.Is(err, define.ErrCtrStateInvalid) && !errors.Is(err, define.ErrCtrStopped) {
 					return err
 				}
@@ -299,7 +299,7 @@ func (p *Pod) Cleanup(ctx context.Context) (map[string]error, error) {
 		c := ctr
 		logrus.Debugf("Adding parallel job to clean up container %s", c.ID())
 		retChan := parallel.Enqueue(ctx, func() error {
-			return c.Cleanup(ctx)
+			return c.Cleanup(ctx, false)
 		})
 
 		ctrErrChan[c.ID()] = retChan

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

pkg/domain/entities/containers.go

@@ -371,6 +371,7 @@ type ContainerCleanupOptions struct {
 	Latest      bool
 	Remove      bool
 	RemoveImage bool
+	StoppedOnly bool // Only cleanup if the container is stopped, i.e. was running before
 }
 
 // ContainerCleanupReport describes the response from a

pkg/domain/infra/abi/containers.go

@@ -313,7 +313,7 @@ func (ic *ContainerEngine) ContainerStop(ctx context.Context, namesOrIds []strin
 				}
 			}
 		} else {
-			if err = c.Cleanup(ctx); err != nil {
+			if err = c.Cleanup(ctx, false); err != nil {
 				// The container could still have been removed, as we unlocked
 				// after we stopped it.
 				if errors.Is(err, define.ErrNoSuchCtr) || errors.Is(err, define.ErrCtrRemoved) {
@@ -1320,7 +1320,7 @@ func (ic *ContainerEngine) ContainerCleanup(ctx context.Context, namesOrIds []st
 				report.RmErr = fmt.Errorf("failed to clean up and remove container %v: %w", ctr.ID(), err)
 			}
 		} else {
-			err := ctr.Cleanup(ctx)
+			err := ctr.Cleanup(ctx, options.StoppedOnly)
 			// ignore error if ctr is removed or cannot be cleaned up, likely the ctr was already restarted by another process
 			if err != nil && !errors.Is(err, define.ErrNoSuchCtr) && !errors.Is(err, define.ErrCtrStateInvalid) {
 				report.CleanErr = fmt.Errorf("failed to clean up container %v: %w", ctr.ID(), err)

pkg/specgenutil/util.go

@@ -311,7 +311,9 @@ func CreateExitCommandArgs(storageConfig storageTypes.StoreOptions, config *conf
 		command = append(command, "--module", module)
 	}
 
-	command = append(command, []string{"container", "cleanup"}...)
+	// --stopped-only is used to ensure we only cleanup stopped containers and do not race
+	// against other processes that did a cleanup() + init() again before we had the chance to run
+	command = append(command, []string{"container", "cleanup", "--stopped-only"}...)
 
 	if rm {
 		command = append(command, "--rm")