From 3280da0500e9f78901e86575ba2bdffd83fd1b66 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Fri, 12 Jul 2024 13:39:54 +0200 Subject: [PATCH] fix race conditions in start/attach logic The current code did something like this: lock() getState() unlock() if state != running lock() getState() == running -> error unlock() This of course is wrong because between the first unlock() and second lock() call another process could have modified the state. This meant that sometimes you would get a weird error on start because the internal setup errored as the container was already running. In general any state check without holding the lock is incorrect and will result in race conditions. As such refactor the code to combine both StartAndAttach and Attach() into one function that can handle both. With that we can move the running check into the locked code. Also use typed error for this specific error case then the callers can check and ignore the specific error when needed. This also allows us to fix races in the compat API that did a similar racy state check. This commit changes slightly how we output the result, previously a start on already running container would never print the id/name of the container which is confusing and sort of breaks idempotence. Now it will include the output except when --all is used. Then it only reports the ids that were actually started. Fixes #23246 Signed-off-by: Paul Holzinger --- libpod/container_api.go | 97 ++++++------------- libpod/container_internal.go | 5 + libpod/define/errors.go | 2 + libpod/oci_conmon_attach_common.go | 1 + pkg/api/handlers/compat/containers_start.go | 14 +-- pkg/domain/infra/abi/containers.go | 77 ++++++--------- .../infra/abi/terminal/terminal_common.go | 6 +- 7 files changed, 78 insertions(+), 124 deletions(-) diff --git a/libpod/container_api.go b/libpod/container_api.go index 525183c751..dc355b9c35 100644 --- a/libpod/container_api.go +++ b/libpod/container_api.go @@ -15,7 +15,6 @@ import ( "github.com/containers/common/pkg/resize" "github.com/containers/podman/v5/libpod/define" "github.com/containers/podman/v5/libpod/events" - "github.com/containers/podman/v5/pkg/signal" "github.com/containers/storage/pkg/archive" spec "github.com/opencontainers/runtime-spec/specs-go" "github.com/sirupsen/logrus" @@ -142,13 +141,12 @@ func (c *Container) Update(resources *spec.LinuxResources, restartPolicy *string return c.update(resources, restartPolicy, restartRetries) } -// StartAndAttach starts a container and attaches to it. -// This acts as a combination of the Start and Attach APIs, ensuring proper +// Attach to a container. +// The last parameter "start" can be used to also start the container. +// This will then Start and Attach APIs, ensuring proper // ordering of the two such that no output from the container is lost (e.g. the // Attach call occurs before Start). -// In overall functionality, it is identical to the Start call, with the added -// side effect that an attach session will also be started. -func (c *Container) StartAndAttach(ctx context.Context, streams *define.AttachStreams, keys string, resize <-chan resize.TerminalSize, recursive bool) (retChan <-chan error, finalErr error) { +func (c *Container) Attach(ctx context.Context, streams *define.AttachStreams, keys string, resize <-chan resize.TerminalSize, start bool) (retChan <-chan error, finalErr error) { defer func() { if finalErr != nil { // Have to re-lock. @@ -175,9 +173,27 @@ func (c *Container) StartAndAttach(ctx context.Context, streams *define.AttachSt } } - if err := c.prepareToStart(ctx, recursive); err != nil { - return nil, err + if c.state.State != define.ContainerStateRunning { + if !start { + return nil, errors.New("you can only attach to running containers") + } + + if err := c.prepareToStart(ctx, true); err != nil { + return nil, err + } } + + if !start { + // A bit awkward, technically passthrough never supports attach. We only pretend + // it does as we leak the stdio fds down into the container but that of course only + // works if we are the process that started conmon with the right fds. + if c.LogDriver() == define.PassthroughLogging { + return nil, fmt.Errorf("this container is using the 'passthrough' log driver, cannot attach: %w", define.ErrNoLogs) + } else if c.LogDriver() == define.PassthroughTTYLogging { + return nil, fmt.Errorf("this container is using the 'passthrough-tty' log driver, cannot attach: %w", define.ErrNoLogs) + } + } + attachChan := make(chan error) // We need to ensure that we don't return until start() fired in attach. @@ -194,7 +210,7 @@ func (c *Container) StartAndAttach(ctx context.Context, streams *define.AttachSt opts := new(AttachOptions) opts.Streams = streams opts.DetachKeys = &keys - opts.Start = true + opts.Start = start opts.Started = startedChan // attach and start the container on a different thread. waitForHealthy must @@ -213,7 +229,13 @@ func (c *Container) StartAndAttach(ctx context.Context, streams *define.AttachSt c.newContainerEvent(events.Attach) } - return attachChan, c.waitForHealthy(ctx) + if start { + if err := c.waitForHealthy(ctx); err != nil { + return nil, err + } + } + + return attachChan, nil } // RestartWithTimeout restarts a running container and takes a given timeout in uint @@ -315,61 +337,6 @@ func (c *Container) Kill(signal uint) error { return c.save() } -// Attach attaches to a container. -// This function returns when the attach finishes. It does not hold the lock for -// the duration of its runtime, only using it at the beginning to verify state. -func (c *Container) Attach(streams *define.AttachStreams, keys string, resize <-chan resize.TerminalSize) error { - if c.LogDriver() == define.PassthroughLogging { - return fmt.Errorf("this container is using the 'passthrough' log driver, cannot attach: %w", define.ErrNoLogs) - } - if c.LogDriver() == define.PassthroughTTYLogging { - return fmt.Errorf("this container is using the 'passthrough-tty' log driver, cannot attach: %w", define.ErrNoLogs) - } - if !c.batched { - c.lock.Lock() - 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) - } - - // HACK: This is really gross, but there isn't a better way without - // splitting attach into separate versions for StartAndAttach and normal - // attaching, and I really do not want to do that right now. - // Send a SIGWINCH after attach succeeds so that most programs will - // redraw the screen for the new attach session. - attachRdy := make(chan bool, 1) - if c.Terminal() { - go func() { - <-attachRdy - c.lock.Lock() - defer c.lock.Unlock() - if err := c.ociRuntime.KillContainer(c, uint(signal.SIGWINCH), false); err != nil { - logrus.Warnf("Unable to send SIGWINCH to container %s after attach: %v", c.ID(), err) - } - }() - } - - // Start resizing - if c.LogDriver() != define.PassthroughLogging && c.LogDriver() != define.PassthroughTTYLogging { - registerResizeFunc(resize, c.bundlePath()) - } - - opts := new(AttachOptions) - opts.Streams = streams - opts.DetachKeys = &keys - opts.AttachReady = attachRdy - - c.newContainerEvent(events.Attach) - return c.ociRuntime.Attach(c, opts) -} - // HTTPAttach forwards an attach session over a hijacked HTTP session. // HTTPAttach will consume and close the included httpCon, which is expected to // be sourced from a hijacked HTTP connection. diff --git a/libpod/container_internal.go b/libpod/container_internal.go index a9c076085a..46cd413459 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -821,6 +821,11 @@ func (c *Container) save() error { func (c *Container) prepareToStart(ctx context.Context, recursive bool) (retErr error) { // Container must be created or stopped to be started if !c.ensureState(define.ContainerStateConfigured, define.ContainerStateCreated, define.ContainerStateStopped, define.ContainerStateExited) { + // Special case: Let the caller know that is is already running, + // the caller can then decide to ignore/handle the error the way it needs. + if c.state.State == define.ContainerStateRunning { + return fmt.Errorf("container %s: %w", c.ID(), define.ErrCtrStateRunning) + } return fmt.Errorf("container %s must be in Created or Stopped state to be started: %w", c.ID(), define.ErrCtrStateInvalid) } diff --git a/libpod/define/errors.go b/libpod/define/errors.go index 2b3e3385b3..96f4ac6224 100644 --- a/libpod/define/errors.go +++ b/libpod/define/errors.go @@ -60,6 +60,8 @@ var ( // ErrCtrStateInvalid indicates a container is in an improper state for // the requested operation ErrCtrStateInvalid = errors.New("container state improper") + // ErrCtrStateRunning indicates a container is running. + ErrCtrStateRunning = errors.New("container is running") // ErrExecSessionStateInvalid indicates that an exec session is in an // improper state for the requested operation ErrExecSessionStateInvalid = errors.New("exec session state improper") diff --git a/libpod/oci_conmon_attach_common.go b/libpod/oci_conmon_attach_common.go index c69cbfbf55..c2e1b6351f 100644 --- a/libpod/oci_conmon_attach_common.go +++ b/libpod/oci_conmon_attach_common.go @@ -91,6 +91,7 @@ func (r *ConmonOCIRuntime) Attach(c *Container, params *AttachOptions) error { } params.Started <- true } + close(params.Started) if passthrough { return nil diff --git a/pkg/api/handlers/compat/containers_start.go b/pkg/api/handlers/compat/containers_start.go index 08ad3287ac..813f5e1f1d 100644 --- a/pkg/api/handlers/compat/containers_start.go +++ b/pkg/api/handlers/compat/containers_start.go @@ -1,6 +1,7 @@ package compat import ( + "errors" "net/http" api "github.com/containers/podman/v5/pkg/api/types" @@ -33,16 +34,11 @@ func StartContainer(w http.ResponseWriter, r *http.Request) { utils.ContainerNotFound(w, name, err) return } - state, err := con.State() - if err != nil { - utils.InternalServerError(w, err) - return - } - if state == define.ContainerStateRunning { - utils.WriteResponse(w, http.StatusNotModified, nil) - return - } if err := con.Start(r.Context(), true); err != nil { + if errors.Is(err, define.ErrCtrStateRunning) { + utils.WriteResponse(w, http.StatusNotModified, nil) + return + } utils.InternalServerError(w, err) return } diff --git a/pkg/domain/infra/abi/containers.go b/pkg/domain/infra/abi/containers.go index 4f0fd63094..b350c68e13 100644 --- a/pkg/domain/infra/abi/containers.go +++ b/pkg/domain/infra/abi/containers.go @@ -795,13 +795,6 @@ func (ic *ContainerEngine) ContainerAttach(ctx context.Context, nameOrID string, } ctr := containers[0] - conState, err := ctr.State() - if err != nil { - return fmt.Errorf("unable to determine state of %s: %w", ctr.ID(), err) - } - if conState != define.ContainerStateRunning { - return fmt.Errorf("you can only attach to running containers") - } // If the container is in a pod, also set to recursively start dependencies err = terminal.StartAttachCtr(ctx, ctr.Container, options.Stdout, options.Stderr, options.Stdin, options.DetachKeys, options.SigProxy, false) @@ -939,14 +932,9 @@ func (ic *ContainerEngine) ContainerStart(ctx context.Context, namesOrIds []stri // There can only be one container if attach was used for i := range containers { ctr := containers[i] - ctrState, err := ctr.State() - if err != nil { - return nil, err - } - ctrRunning := ctrState == define.ContainerStateRunning if options.Attach { - err = terminal.StartAttachCtr(ctx, ctr.Container, options.Stdout, options.Stderr, options.Stdin, options.DetachKeys, options.SigProxy, !ctrRunning) + err = terminal.StartAttachCtr(ctx, ctr.Container, options.Stdout, options.Stderr, options.Stdin, options.DetachKeys, options.SigProxy, true) if errors.Is(err, define.ErrDetach) { // User manually detached // Exit cleanly immediately @@ -970,16 +958,6 @@ func (ic *ContainerEngine) ContainerStart(ctx context.Context, namesOrIds []stri return reports, fmt.Errorf("attempting to start container %s would cause a deadlock; please run 'podman system renumber' to resolve", ctr.ID()) } - if ctrRunning { - reports = append(reports, &entities.ContainerStartReport{ - Id: ctr.ID(), - RawInput: ctr.rawInput, - Err: nil, - ExitCode: 0, - }) - return reports, err - } - if err != nil { reports = append(reports, &entities.ContainerStartReport{ Id: ctr.ID(), @@ -1008,34 +986,43 @@ func (ic *ContainerEngine) ContainerStart(ctx context.Context, namesOrIds []stri return reports, nil } // end attach - // Start the container if it's not running already. - if !ctrRunning { - // Handle non-attach start - // If the container is in a pod, also set to recursively start dependencies - report := &entities.ContainerStartReport{ - Id: ctr.ID(), - RawInput: ctr.rawInput, - ExitCode: 125, - } - if err := ctr.Start(ctx, true); err != nil { - report.Err = err - if errors.Is(err, define.ErrWillDeadlock) { - report.Err = fmt.Errorf("please run 'podman system renumber' to resolve deadlocks: %w", err) + // Handle non-attach start + // If the container is in a pod, also set to recursively start dependencies + report := &entities.ContainerStartReport{ + Id: ctr.ID(), + RawInput: ctr.rawInput, + ExitCode: 125, + } + if err := ctr.Start(ctx, true); err != nil { + // Already running is no error for the start command as it is idempotent. + if errors.Is(err, define.ErrCtrStateRunning) { + // If all is set we only want to output the actual started containers + // so do not include the entry in the result. + if !options.All { + report.ExitCode = 0 reports = append(reports, report) - continue - } - report.Err = fmt.Errorf("unable to start container %q: %w", ctr.ID(), err) - reports = append(reports, report) - if ctr.AutoRemove() { - if _, _, err := ic.removeContainer(ctx, ctr.Container, entities.RmOptions{}); err != nil { - logrus.Errorf("Removing container %s: %v", ctr.ID(), err) - } } continue } - report.ExitCode = 0 + + report.Err = err + if errors.Is(err, define.ErrWillDeadlock) { + report.Err = fmt.Errorf("please run 'podman system renumber' to resolve deadlocks: %w", err) + reports = append(reports, report) + continue + } + report.Err = fmt.Errorf("unable to start container %q: %w", ctr.ID(), err) + if ctr.AutoRemove() { + if _, _, err := ic.removeContainer(ctx, ctr.Container, entities.RmOptions{}); err != nil { + logrus.Errorf("Removing container %s: %v", ctr.ID(), err) + } + } reports = append(reports, report) + continue } + // no error set exit code to 0 + report.ExitCode = 0 + reports = append(reports, report) } return reports, nil } diff --git a/pkg/domain/infra/abi/terminal/terminal_common.go b/pkg/domain/infra/abi/terminal/terminal_common.go index e4f3afeb32..5ca0d8d2c4 100644 --- a/pkg/domain/infra/abi/terminal/terminal_common.go +++ b/pkg/domain/infra/abi/terminal/terminal_common.go @@ -89,11 +89,7 @@ func StartAttachCtr(ctx context.Context, ctr *libpod.Container, stdout, stderr, ProxySignals(ctr) } - if !startContainer { - return ctr.Attach(streams, detachKeys, resize) - } - - attachChan, err := ctr.StartAndAttach(ctx, streams, detachKeys, resize, true) + attachChan, err := ctr.Attach(ctx, streams, detachKeys, resize, startContainer) if err != nil { return err }