From 441b8d73c866be63db5e2e16e6e2e3d9b70a2634 Mon Sep 17 00:00:00 2001 From: Asish Kumar Date: Wed, 20 May 2026 12:49:30 +0530 Subject: [PATCH] fix: do not close caller-owned writer in container DefaultHandler When pack runs a lifecycle phase, the container DefaultHandler copies the container's stdout/stderr to the phase writers and, once copying finishes, calls optionallyCloseWriter on each. That helper closed any writer that implemented io.Closer. For untrusted builders the phase writers are *logging.PrefixWriter, whose Close only flushes a buffered partial line and leaves the wrapped writer open, so closing them is correct. For trusted builders, however, the logger's writer is passed through unwrapped. When a library consumer constructs the logger with os.Stdout (an *os.File, and therefore an io.Closer), running a build closed os.Stdout as a side effect. The pack CLI did not notice because it exits right after, but it is surprising and breaks programs that embed pack and expect their writer to stay open. Restrict optionallyCloseWriter to *logging.PrefixWriter so only the buffering writers pack creates are flushed, and writers owned by the caller are left untouched. Add unit tests covering both cases. Resolves #1214 Signed-off-by: Asish Kumar --- internal/container/run.go | 14 ++++++- internal/container/run_test.go | 74 ++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 2 deletions(-) create mode 100644 internal/container/run_test.go diff --git a/internal/container/run.go b/internal/container/run.go index eeeda7d2c..899fb054d 100644 --- a/internal/container/run.go +++ b/internal/container/run.go @@ -9,6 +9,8 @@ import ( dcontainer "github.com/moby/moby/api/types/container" dockerClient "github.com/moby/moby/client" "github.com/pkg/errors" + + "github.com/buildpacks/pack/pkg/logging" ) type Handler func(bodyChan <-chan dcontainer.WaitResponse, errChan <-chan error, reader io.Reader) error @@ -87,9 +89,17 @@ func DefaultHandler(out, errOut io.Writer) Handler { } } +// optionallyCloseWriter flushes writers that pack created for a phase and that +// buffer output, namely *logging.PrefixWriter, whose Close flushes any pending +// partial line without closing the writer it wraps. +// +// Writers owned by the caller must not be closed here. When a builder is +// trusted the logger's writer (often os.Stdout) is passed through unwrapped, and +// closing an arbitrary io.Closer would close the caller's stream. That is +// surprising and breaks consumers using pack as a library. See #1214. func optionallyCloseWriter(writer io.Writer) error { - if closer, ok := writer.(io.Closer); ok { - return closer.Close() + if prefixWriter, ok := writer.(*logging.PrefixWriter); ok { + return prefixWriter.Close() } return nil diff --git a/internal/container/run_test.go b/internal/container/run_test.go new file mode 100644 index 000000000..11b776bda --- /dev/null +++ b/internal/container/run_test.go @@ -0,0 +1,74 @@ +package container + +import ( + "bytes" + "io" + "testing" + + "github.com/heroku/color" + "github.com/sclevine/spec" + "github.com/sclevine/spec/report" + + "github.com/buildpacks/pack/pkg/logging" +) + +func TestContainerRun(t *testing.T) { + color.Disable(true) + defer color.Disable(false) + + spec.Run(t, "container/run", testContainerRun, spec.Report(report.Terminal{}), spec.Parallel()) +} + +// recordingWriteCloser models a writer owned by the caller that also happens to +// implement io.Closer, such as os.Stdout passed to pack via the logger. +type recordingWriteCloser struct { + bytes.Buffer + closed bool +} + +func (w *recordingWriteCloser) Close() error { + w.closed = true + return nil +} + +var _ io.WriteCloser = &recordingWriteCloser{} + +func testContainerRun(t *testing.T, when spec.G, it spec.S) { + when("#optionallyCloseWriter", func() { + it("does not close writers owned by the caller", func() { + writer := &recordingWriteCloser{} + + if err := optionallyCloseWriter(writer); err != nil { + t.Fatalf("expected no error, got: %v", err) + } + + // Closing a caller-owned writer (e.g. os.Stdout) is surprising and + // breaks consumers using pack as a library. See #1214. + if writer.closed { + t.Fatal("expected caller-owned writer to remain open, but it was closed") + } + }) + + it("flushes buffered output from a PrefixWriter", func() { + var buf bytes.Buffer + prefixWriter := logging.NewPrefixWriter(&buf, "some-prefix") + + // A line without a trailing newline stays buffered until the writer + // is flushed. + if _, err := prefixWriter.Write([]byte("buffered output")); err != nil { + t.Fatalf("writing to prefix writer: %v", err) + } + if got := buf.String(); got != "" { + t.Fatalf("expected partial line to stay buffered, got: %q", got) + } + + if err := optionallyCloseWriter(prefixWriter); err != nil { + t.Fatalf("expected no error, got: %v", err) + } + + if got := buf.String(); !bytes.Contains([]byte(got), []byte("buffered output")) { + t.Fatalf("expected buffered output to be flushed, got: %q", got) + } + }) + }) +}