fix: do not close caller-owned writer in container DefaultHandler#2608
Open
officialasishkumar wants to merge 1 commit into
Open
fix: do not close caller-owned writer in container DefaultHandler#2608officialasishkumar wants to merge 1 commit into
officialasishkumar wants to merge 1 commit into
Conversation
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 buildpacks#1214 Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
f9f1d39 to
441b8d7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When
packruns a lifecycle phase,container.DefaultHandlercopies the container's stdout/stderr into the phase writers and, once copying finishes, callsoptionallyCloseWriteron each writer. That helper closed any writer that implementedio.Closer.For untrusted builders the phase writers are
*logging.PrefixWriter, whoseCloseonly flushes a buffered partial line and leaves the wrapped writer open, so closing them is correct and necessary. For trusted builders, however, the logger's writer is passed through unwrapped. When a consumer builds the logger withos.Stdout(an*os.File, and therefore anio.Closer), running a build closedos.Stdoutas a side effect.The
packCLI never noticed because it exits immediately afterwards, but it is surprising and breaks programs that embedpackas a library and expect the writer they passed in to stay open (reported in #1214).This change restricts
optionallyCloseWriterto*logging.PrefixWriterso that only the buffering writerspackitself creates are flushed, and writers owned by the caller are left untouched.Output
Using
packas a library with a trusted builder:Before
os.Stdoutis closed onceBuildreturns, so any subsequent writes to it fail.After
os.Stdoutstays open afterBuildreturns. Prefixed phase output for untrusted builders is still flushed exactly as before.Documentation
Related
Resolves #1214