diff --git a/internal/shim/task/io.go b/internal/shim/task/io.go index 49807eb9..a0cf9d77 100644 --- a/internal/shim/task/io.go +++ b/internal/shim/task/io.go @@ -46,7 +46,7 @@ func generateStreamID(prefix string) string { return fmt.Sprintf("%s-%d-%s", prefix, time.Now().UnixNano(), base64.RawURLEncoding.EncodeToString(b[:])) } -func (s *service) forwardIO(ctx context.Context, ss streamCreator, idPrefix string, sio stdio.Stdio) (stdio.Stdio, func(ctx context.Context) error, <-chan struct{}, func() error, error) { +func (s *service) forwardIO(ctx context.Context, ss streamCreator, idPrefix string, sio stdio.Stdio) (stdio.Stdio, func(ctx context.Context) error, <-chan struct{}, func(context.Context) error, error) { pio := sio if pio.IsNull() { return pio, nil, nil, nil, nil diff --git a/internal/shim/task/io_copystreams.go b/internal/shim/task/io_copystreams.go index 5f798017..86c9ac75 100644 --- a/internal/shim/task/io_copystreams.go +++ b/internal/shim/task/io_copystreams.go @@ -19,56 +19,168 @@ package task import ( "context" "io" + "sync" "github.com/containerd/log" ) -// copyStdinUntilClose reads from f and writes raw bytes to sc until closeCh -// is closed (CloseIO) or f delivers EOF. On either exit it calls -// sc.CloseWrite() to send OP_SHUTDOWN(SEND) in-order on the vsock stdin +// startStdinForward launches the goroutine that copies the stdin FIFO/pipe f to +// the guest over sc, and returns the stdinEOF callback CloseIO invokes to drain +// f and send the in-band EOF. It registers on cwg like the other stream copiers. +func startStdinForward(ctx context.Context, cwg *sync.WaitGroup, sc stdinStreamWriteCloser, f io.ReadCloser) func(context.Context) error { + // closeCh delivers the CloseIO request context to the copy goroutine; + // drainDone is closed once the in-band EOF has been sent. + closeCh := make(chan context.Context, 1) + drainDone := make(chan struct{}) + cwg.Add(1) + go func() { + cwg.Done() + p := bufPool.Get().(*[]byte) + defer bufPool.Put(p) + copyStdinUntilClose(ctx, sc, f, *p, closeCh) + // copyStdinUntilClose has delivered the in-band EOF (CloseWrite), so + // signal CloseIO now rather than after f.Close, which could block. + close(drainDone) + // Do NOT Close sc here; ioShutdown/forwardIO owns the transport so it + // outlives the in-band EOF and the host can close its end cleanly after + // the guest drains. Closing f also releases any background read still + // parked on it. + f.Close() + }() + return stdinEOFFunc(closeCh, drainDone) +} + +// stdinEOFFunc builds the stdinEOF callback invoked by CloseIO. It delivers the +// CloseIO request context to the copy goroutine on closeCh and blocks until the +// goroutine has sent the in-band EOF (drainDone) or the caller's context is +// done. Binding the drain to the CloseIO context means a peer that requests +// CloseIO without closing the FIFO write end cannot wedge CloseIO forever: the +// drain lasts only as long as the caller is willing to wait. +func stdinEOFFunc(closeCh chan<- context.Context, drainDone <-chan struct{}) func(context.Context) error { + return func(cctx context.Context) error { + // Non-blocking send so an already-cancelled cctx can't short-circuit + // delivery and leave stdin open. closeCh is buffered (cap 1) and drained + // at most once, so the request lands on the first CloseIO and is dropped + // on a repeat or once the goroutine has exited (drainDone). + select { + case closeCh <- cctx: + case <-drainDone: + // Goroutine already exited (pipe/FIFO EOF): EOF already delivered. + return nil + default: + } + // cctx bounds only how long we wait for the drain, not delivery. + select { + case <-drainDone: + return nil + case <-cctx.Done(): + // drainDone and cctx.Done() can be ready together; prefer success so + // a drain that finished isn't reported as a spurious CloseIO error. + select { + case <-drainDone: + return nil + default: + return cctx.Err() + } + } + } +} + +// copyStdinUntilClose reads from f and writes raw bytes to sc until a CloseIO +// context arrives on closeCh (CloseIO) or f delivers EOF. On either exit it +// calls sc.CloseWrite() to send OP_SHUTDOWN(SEND) in-order on the vsock stdin // stream, guaranteeing the guest sees EOF after all data already written — // not via an out-of-band RPC that could race in-flight bytes. +// +// Reads always run on a background goroutine feeding readCh, so no read is ever +// performed synchronously in a select. That matters on the CloseIO drain path: +// draining to EOF requires the FIFO write end to be closed, but a peer can +// issue CloseIO without ever closing it. The drain is therefore bound to the +// CloseIO request context delivered on closeCh — if the caller cancels or its +// deadline elapses, the drain stops and still delivers the in-band EOF instead +// of wedging forever. On the conforming path (writer closes on CloseIO) the +// drain reads through to EOF with no data lost. func copyStdinUntilClose(ctx context.Context, sc interface { io.Writer CloseWrite() error -}, f io.Reader, buf []byte, closeCh <-chan struct{}) { +}, f io.Reader, buf []byte, closeCh <-chan context.Context) { type readResult struct { n int err error } readCh := make(chan readResult, 1) - for { + // read spawns a single background read into buf. At most one read is ever + // in flight: the next read is only started after the current result has + // been consumed and its bytes written, so buf is never shared concurrently. + read := func() { go func() { n, err := f.Read(buf) readCh <- readResult{n, err} }() + } + closeWrite := func() { + if err := sc.CloseWrite(); err != nil { + log.G(ctx).WithError(err).Warn("error sending stdin EOF via CloseWrite") + } + } + // writeChunk forwards buf[:res.n] to the guest, reporting a write error. + writeChunk := func(res readResult) error { + if res.n == 0 { + return nil + } + _, err := sc.Write(buf[:res.n]) + return err + } + + var ( + // closed is set once CloseIO fires; its Done() then bounds the drain so a + // peer that requests CloseIO without closing the FIFO write end cannot wedge + // us on a read that never completes. + closed context.Context + + // abort set to closed.Done() once CloseIO fires. + abort <-chan struct{} + ) + + read() + for { + if closed != nil { + abort = closed.Done() + } + select { - case <-closeCh: - // CloseIO fired: drain the pending read then send in-band EOF. - res := <-readCh - if res.n > 0 { - if _, err := sc.Write(buf[:res.n]); err != nil { - log.G(ctx).WithError(err).Warn("error writing stdin on CloseIO") - } - } - if err := sc.CloseWrite(); err != nil { - log.G(ctx).WithError(err).Warn("error sending stdin EOF via CloseWrite") - } - return + case closed = <-closeCh: + // CloseIO fired: keep draining, now bounded by closed.Done(). + closeCh = nil case res := <-readCh: - if res.n > 0 { - if _, err := sc.Write(buf[:res.n]); err != nil { - log.G(ctx).WithError(err).Warn("error writing stdin") - return - } + if err := writeChunk(res); err != nil { + log.G(ctx).WithError(err).Warn("error writing stdin") + closeWrite() + return } - if res.err != nil { - // Pipe/named-pipe EOF: client closed its write end. - if err := sc.CloseWrite(); err != nil { - log.G(ctx).WithError(err).Warn("error sending stdin EOF on pipe close") - } + // A read error (EOF: writer closed) always ends the copy; once + // draining, a zero-length read means the FIFO is fully drained too. + if res.err != nil || (closed != nil && res.n == 0) { + closeWrite() return } + read() + case <-abort: + // The CloseIO caller gave up before the FIFO delivered EOF: send the + // in-band EOF rather than wedge. select is random, so abort can win + // with a completed read's bytes still on readCh; a best-effort non-blocking + // drain forwards those first (a still-in-flight read cannot be recovered + // but is bound by fifo closure at the end of the copy goroutine). + select { + case res := <-readCh: + if err := writeChunk(res); err != nil { + log.G(ctx).WithError(err).Warn("error writing stdin on drain abort") + } + default: + } + log.G(ctx).WithError(closed.Err()).Warn("stdin drain aborted; CloseIO context done before FIFO EOF") + closeWrite() + return } } } diff --git a/internal/shim/task/io_copystreams_test.go b/internal/shim/task/io_copystreams_test.go new file mode 100644 index 00000000..1bf04201 --- /dev/null +++ b/internal/shim/task/io_copystreams_test.go @@ -0,0 +1,270 @@ +/* + Copyright The containerd Authors. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package task + +import ( + "bytes" + "context" + "io" + "sync" + "testing" +) + +// recordingStreamConn records everything written to it and whether CloseWrite +// has been called, so a test can assert that all data was forwarded before EOF +// was signalled to the guest. +type recordingStreamConn struct { + mu sync.Mutex + buf bytes.Buffer + closeWriteCalled bool + bytesAfterClose int +} + +func (c *recordingStreamConn) Write(p []byte) (int, error) { + c.mu.Lock() + defer c.mu.Unlock() + if c.closeWriteCalled { + c.bytesAfterClose += len(p) + } + return c.buf.Write(p) +} + +func (c *recordingStreamConn) CloseWrite() error { + c.mu.Lock() + defer c.mu.Unlock() + c.closeWriteCalled = true + return nil +} + +// gatedChunkReader returns pre-set chunks one per Read, delivering EOF after +// the last chunk. The first Read signals on started and then blocks until gate +// is closed. +type gatedChunkReader struct { + chunks [][]byte + idx int + started chan struct{} + gate <-chan struct{} + gated bool +} + +func (r *gatedChunkReader) Read(p []byte) (int, error) { + if !r.gated { + close(r.started) + <-r.gate + r.gated = true + } + if r.idx >= len(r.chunks) { + return 0, io.EOF + } + n := copy(p, r.chunks[r.idx]) + r.idx++ + return n, nil +} + +// TestCopyStdinUntilCloseDrainsOnCloseIO is a regression test for stdin +// truncation on CloseIO. When CloseIO fires while the stdin reader has fallen +// behind (more than one buffer's worth of data still buffered), the closeCh +// branch must drain all remaining data before calling CloseWrite. +func TestCopyStdinUntilCloseDrainsOnCloseIO(t *testing.T) { + chunks := [][]byte{ + []byte("AAAA"), + []byte("BBBB"), + []byte("CCCC"), + } + const want = "AAAABBBBCCCC" + + gate := make(chan struct{}) + closeCh := make(chan context.Context, 1) + sc := &recordingStreamConn{} + // Buffer larger than a single chunk so each Read returns exactly one + // chunk (mirroring one FIFO read per iteration). + buf := make([]byte, 8) + r := &gatedChunkReader{chunks: chunks, started: make(chan struct{}), gate: gate} + + done := make(chan struct{}) + go func() { + copyStdinUntilClose(context.Background(), sc, r, buf, closeCh) + close(done) + }() + + // Wait until the first read has begun and parked on gate. From here the + // read channel cannot become ready until we close gate, so firing CloseIO + // now forces the select down the closeCh branch deterministically. The + // CloseIO context is never cancelled, so the drain runs through to EOF. + <-r.started + closeCh <- context.Background() + + // Release the reader so the closeCh branch can drain every remaining chunk. + close(gate) + + select { + case <-done: + case <-t.Context().Done(): + t.Fatal("copyStdinUntilClose did not return") + } + + if got := sc.buf.String(); got != want { + t.Fatalf("stdin truncated on CloseIO: got %q (%d bytes), want %q (%d bytes)", + got, len(got), want, len(want)) + } + if !sc.closeWriteCalled { + t.Fatal("CloseWrite was not called") + } + if sc.bytesAfterClose != 0 { + t.Fatalf("%d bytes written after CloseWrite; EOF was signalled before draining", + sc.bytesAfterClose) + } +} + +// blockingReader parks on every Read until release, then reports EOF. It models +// a FIFO whose write end is never closed, so a read can never complete on its +// own. +type blockingReader struct { + release <-chan struct{} +} + +func (r *blockingReader) Read(p []byte) (int, error) { + <-r.release + return 0, io.EOF +} + +// TestStdinEOFFuncDeliversEOFWhenContextAlreadyCancelled is a regression test +// for CloseIO leaving stdin open. When CloseIO is invoked with a context that is +// already cancelled, stdinEOFFunc must still signal the copy goroutine so it +// sends the in-band EOF (CloseWrite); it must not short-circuit on the cancelled +// context and return with the request undelivered. +func TestStdinEOFFuncDeliversEOFWhenContextAlreadyCancelled(t *testing.T) { + closeCh := make(chan context.Context, 1) + drainDone := make(chan struct{}) + release := make(chan struct{}) + sc := &recordingStreamConn{} + buf := make([]byte, 8) + r := &blockingReader{release: release} + + go func() { + copyStdinUntilClose(context.Background(), sc, r, buf, closeCh) + close(drainDone) + }() + + stdinEOF := stdinEOFFunc(closeCh, drainDone) + + // CloseIO fires with an already-cancelled context. The drain cannot read to + // EOF (the reader is parked), so delivery of the in-band EOF depends solely + // on the request reaching the copy goroutine. + cctx, cancel := context.WithCancel(context.Background()) + cancel() + if err := stdinEOF(cctx); err != nil && err != context.Canceled { + t.Fatalf("stdinEOF returned unexpected error: %v", err) + } + + select { + case <-drainDone: + case <-t.Context().Done(): + t.Fatal("copy goroutine was not signalled; in-band EOF never delivered") + } + + // Let the parked read return so its goroutine does not leak. + close(release) + + if !sc.closeWriteCalled { + t.Fatal("CloseWrite was not called; stdin left open despite CloseIO") + } +} + +// stallingReader delivers its chunks one per Read and then blocks forever, +// modelling a FIFO whose write end is never closed (so a read can never see +// EOF). blocking is closed when the reader parks; release lets the parked read +// finally return so the test leaks no goroutine. +type stallingReader struct { + chunks [][]byte + idx int + blocking chan struct{} + release <-chan struct{} +} + +func (r *stallingReader) Read(p []byte) (int, error) { + if r.idx >= len(r.chunks) { + // No buffered data left and the writer never closed: block like a + // real FIFO read waiting on data or an EOF that never arrives. + close(r.blocking) + <-r.release + return 0, io.EOF + } + n := copy(p, r.chunks[r.idx]) + r.idx++ + return n, nil +} + +// TestCopyStdinUntilCloseAbortsDrainOnContextCancel is a regression test for +// the stdin drain wedging when CloseIO is issued but the FIFO write end is +// never closed. Because the drain is bound to the CloseIO request context, +// cancelling that context (the caller giving up) must forward all buffered +// data, then unblock the drain and still deliver the in-band EOF. +func TestCopyStdinUntilCloseAbortsDrainOnContextCancel(t *testing.T) { + chunks := [][]byte{ + []byte("AAAA"), + []byte("BBBB"), + } + const want = "AAAABBBB" + + closeCh := make(chan context.Context, 1) + release := make(chan struct{}) + sc := &recordingStreamConn{} + buf := make([]byte, 8) + r := &stallingReader{chunks: chunks, blocking: make(chan struct{}), release: release} + + // cctx models the CloseIO request context; cancelling it stands in for the + // caller cancelling or hitting its deadline. + cctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + done := make(chan struct{}) + go func() { + copyStdinUntilClose(context.Background(), sc, r, buf, closeCh) + close(done) + }() + + // CloseIO fires, but the writer is never closed: the drain forwards the + // buffered chunks and then parks on a read that can never complete. + closeCh <- cctx + + // Once the drain is parked on the never-completing read, cancelling the + // CloseIO context must unblock it deterministically (no timing dependence). + <-r.blocking + cancel() + + select { + case <-done: + case <-t.Context().Done(): + t.Fatal("copyStdinUntilClose did not return after context cancel") + } + + // Let the abandoned background read return so no goroutine is leaked. + close(release) + + if got := sc.buf.String(); got != want { + t.Fatalf("stdin data lost before cancel abort: got %q (%d bytes), want %q (%d bytes)", + got, len(got), want, len(want)) + } + if !sc.closeWriteCalled { + t.Fatal("CloseWrite was not called on context cancel") + } + if sc.bytesAfterClose != 0 { + t.Fatalf("%d bytes written after CloseWrite; EOF was signalled before draining", + sc.bytesAfterClose) + } +} diff --git a/internal/shim/task/io_copystreams_unix.go b/internal/shim/task/io_copystreams_unix.go index 889ffa76..0d99ae53 100644 --- a/internal/shim/task/io_copystreams_unix.go +++ b/internal/shim/task/io_copystreams_unix.go @@ -41,10 +41,12 @@ type stdinStreamWriteCloser interface { CloseWrite() error } -// copyStreams returns a stdinEOF function that, when called (by CloseIO), -// signals the stdin goroutine to stop reading the FIFO and send the -// OP_SHUTDOWN(SEND) in-band EOF to the guest. It is nil when stdin is empty. -func copyStreams(ctx context.Context, streams [3]io.ReadWriteCloser, stdin, stdout, stderr string, done chan struct{}) (stdinEOF func() error, err error) { +// copyStreams returns a stdinEOF function that, when called (by CloseIO with +// the CloseIO request context), drains the stdin FIFO and sends the +// OP_SHUTDOWN(SEND) in-band EOF to the guest. It blocks until the EOF has been +// delivered or the passed context is cancelled, binding the drain to the +// CloseIO RPC. It is nil when stdin is empty. +func copyStreams(ctx context.Context, streams [3]io.ReadWriteCloser, stdin, stdout, stderr string, done chan struct{}) (stdinEOF func(context.Context) error, err error) { var cwg sync.WaitGroup var copying atomic.Int32 copying.Store(2) @@ -143,25 +145,7 @@ func copyStreams(ctx context.Context, streams [3]io.ReadWriteCloser, stdin, stdo if err != nil { return nil, fmt.Errorf("containerd-shim: opening %s failed: %s", stdin, err) } - // closeCh is closed by the stdinEOF function (triggered by CloseIO). - closeCh := make(chan struct{}) - cwg.Add(1) - go func() { - cwg.Done() - p := bufPool.Get().(*[]byte) - defer bufPool.Put(p) - copyStdinUntilClose(ctx, sc, f, *p, closeCh) - // Do NOT Close sc here; deferred to ioShutdown/forwardIO cleanup - // so the transport outlives the in-band EOF and the host can - // close its end cleanly after the guest drains. - f.Close() - }() - stdinEOF = func() error { - // Signal the goroutine to stop reading the FIFO and send - // OP_SHUTDOWN(SEND) in-order on the stdin stream. - close(closeCh) - return nil - } + stdinEOF = startStdinForward(ctx, &cwg, sc, f) } cwg.Wait() return stdinEOF, nil diff --git a/internal/shim/task/io_copystreams_windows.go b/internal/shim/task/io_copystreams_windows.go index 81d036f8..9dbd16d5 100644 --- a/internal/shim/task/io_copystreams_windows.go +++ b/internal/shim/task/io_copystreams_windows.go @@ -38,7 +38,7 @@ type stdinStreamWriteCloser interface { CloseWrite() error } -func copyStreams(ctx context.Context, streams [3]io.ReadWriteCloser, stdin, stdout, stderr string, done chan struct{}) (stdinEOF func() error, err error) { +func copyStreams(ctx context.Context, streams [3]io.ReadWriteCloser, stdin, stdout, stderr string, done chan struct{}) (stdinEOF func(context.Context) error, err error) { var cwg sync.WaitGroup var copying atomic.Int32 copying.Store(2) @@ -142,19 +142,7 @@ func copyStreams(ctx context.Context, streams [3]io.ReadWriteCloser, stdin, stdo return nil, fmt.Errorf("containerd-shim: opening %s failed: %s", stdin, err) } } - closeCh := make(chan struct{}) - cwg.Add(1) - go func() { - cwg.Done() - p := bufPool.Get().(*[]byte) - defer bufPool.Put(p) - copyStdinUntilClose(ctx, sc, f, *p, closeCh) - f.Close() - }() - stdinEOF = func() error { - close(closeCh) - return nil - } + stdinEOF = startStdinForward(ctx, &cwg, sc, f) } cwg.Wait() return stdinEOF, nil diff --git a/internal/shim/task/service.go b/internal/shim/task/service.go index 280d94b3..73d12961 100644 --- a/internal/shim/task/service.go +++ b/internal/shim/task/service.go @@ -152,11 +152,12 @@ type container struct { // ioDone is closed when the host-side copy goroutines for the init // process have fully drained output to the destination FIFO. ioDone <-chan struct{} - // stdinEOF, when non-nil, signals the host stdin goroutine to stop - // reading the FIFO and send OP_SHUTDOWN(SEND) in-order on the stdin - // stream. Called by CloseIO instead of forwarding the RPC out-of-band, - // guaranteeing the EOF arrives after all in-flight stdin bytes. - stdinEOF func() error + // stdinEOF, when non-nil, drains the host stdin FIFO and sends + // OP_SHUTDOWN(SEND) in-order on the stdin stream. Called by CloseIO with + // the CloseIO request context instead of forwarding the RPC out-of-band, + // guaranteeing the EOF arrives after all in-flight stdin bytes. It blocks + // until the EOF is delivered or the context is cancelled. + stdinEOF func(context.Context) error // forwarder is the UNIX socket forwarder for this specific container. forwarder *socketForwarder @@ -168,7 +169,7 @@ type container struct { // before the caller can issue Delete. execIODone map[string]<-chan struct{} // execStdinEOF holds the in-band stdin EOF sender per exec ID. - execStdinEOF map[string]func() error + execStdinEOF map[string]func(context.Context) error } // shutdown shuts down the container's IO streams, socket forwarding, and all @@ -490,7 +491,7 @@ func (s *service) Create(ctx context.Context, r *taskAPI.CreateTaskRequest) (_ * stdinEOF: initStdinEOF, execShutdowns: make(map[string]func(context.Context) error), execIODone: make(map[string]<-chan struct{}), - execStdinEOF: make(map[string]func() error), + execStdinEOF: make(map[string]func(context.Context) error), } guestOpts, err := guestRuncOptions(ctx, r.Options) @@ -772,7 +773,7 @@ func (s *service) CloseIO(ctx context.Context, r *taskAPI.CloseIORequest) (*ptyp // stream, preventing truncation caused by an out-of-band RPC on a // separate vsock connection racing in-flight stdin bytes. s.mu.Lock() - var stdinEOF func() error + var stdinEOF func(context.Context) error if c, ok := s.containers[r.ID]; ok { if r.ExecID != "" { stdinEOF = c.execStdinEOF[r.ExecID] @@ -783,7 +784,7 @@ func (s *service) CloseIO(ctx context.Context, r *taskAPI.CloseIORequest) (*ptyp s.mu.Unlock() if stdinEOF != nil { - if err := stdinEOF(); err != nil { + if err := stdinEOF(ctx); err != nil { log.G(ctx).WithError(err).WithFields(log.Fields{ "id": r.ID, "exec": r.ExecID,