From db751ea3c7767b4f9dcde367708a4608676a3eca Mon Sep 17 00:00:00 2001 From: Jan Kaluza Date: Tue, 27 Jan 2026 13:53:50 +0100 Subject: [PATCH] Do not access `uncompressed` reader after close. Before this commit, when the `applyDiff` function returned, the `defer uncompressed.Close()` was executed. This dealocated the resources which might still be used by another goroutine trying to read them. This commit fixes it by reusing existing `UploadReader` logic from the `image` package. Since the `UploadReader` is now used by both `image` and `storage`, it is now moved to `storage` and renamed to `TerminableReader` (it is no longer used just for "upload"). The `TerminableReader` wraps the `Read` function and if its `Terminate` is called, the `Read` function returns an error. This effectively ensures that the real closed `io.Reader` under the `TerminableReader` is never used after `close()` and the crash is fixed. Fixes #148. Signed-off-by: Jan Kaluza --- image/docker/docker_image_dest.go | 4 ++-- storage/layers.go | 6 ++++-- .../pkg/terminablereader/terminable_reader.go | 18 +++++++++--------- .../terminablereader/terminable_reader_test.go | 8 ++++---- 4 files changed, 19 insertions(+), 17 deletions(-) rename image/internal/uploadreader/upload_reader.go => storage/pkg/terminablereader/terminable_reader.go (77%) rename image/internal/uploadreader/upload_reader_test.go => storage/pkg/terminablereader/terminable_reader_test.go (80%) diff --git a/image/docker/docker_image_dest.go b/image/docker/docker_image_dest.go index 2172d0adc6..a5fb97c202 100644 --- a/image/docker/docker_image_dest.go +++ b/image/docker/docker_image_dest.go @@ -31,11 +31,11 @@ import ( "go.podman.io/image/v5/internal/set" "go.podman.io/image/v5/internal/signature" "go.podman.io/image/v5/internal/streamdigest" - "go.podman.io/image/v5/internal/uploadreader" "go.podman.io/image/v5/manifest" "go.podman.io/image/v5/pkg/blobinfocache/none" compressiontypes "go.podman.io/image/v5/pkg/compression/types" "go.podman.io/image/v5/types" + "go.podman.io/storage/pkg/terminablereader" ) type dockerImageDestination struct { @@ -183,7 +183,7 @@ func (d *dockerImageDestination) PutBlobWithOptions(ctx context.Context, stream stream = io.TeeReader(stream, sizeCounter) uploadLocation, err = func() (*url.URL, error) { // A scope for defer - uploadReader := uploadreader.NewUploadReader(stream) + uploadReader := terminablereader.NewTerminableReader(stream) // This error text should never be user-visible, we terminate only after makeRequestToResolvedURL // returns, so there isn’t a way for the error text to be provided to any of our callers. defer uploadReader.Terminate(errors.New("Reading data from an already terminated upload")) diff --git a/storage/layers.go b/storage/layers.go index d485c9b4fa..42d9db8728 100644 --- a/storage/layers.go +++ b/storage/layers.go @@ -35,6 +35,7 @@ import ( "go.podman.io/storage/pkg/stringid" "go.podman.io/storage/pkg/system" "go.podman.io/storage/pkg/tarlog" + "go.podman.io/storage/pkg/terminablereader" "go.podman.io/storage/pkg/truncindex" ) @@ -2619,7 +2620,8 @@ func applyDiff(layerOptions *LayerOptions, diff io.Reader, tarSplitFile *os.File if err != nil { return -1, err } - defer uncompressed.Close() + var uncompressed_reader = terminablereader.NewTerminableReader(uncompressed) + defer uncompressed_reader.Terminate(errors.New("Reading data from an already terminated stream")) idLogger, err := tarlog.NewLogger(func(h *tar.Header) { if !strings.HasPrefix(path.Base(h.Name), archive.WhiteoutPrefix) { uidLog[uint32(h.Uid)] = struct{}{} @@ -2635,7 +2637,7 @@ func applyDiff(layerOptions *LayerOptions, diff io.Reader, tarSplitFile *os.File if uncompressedDigester != nil { uncompressedWriter = io.MultiWriter(uncompressedWriter, uncompressedDigester.Hash()) } - payload, err := asm.NewInputTarStream(io.TeeReader(uncompressed, uncompressedWriter), metadata, storage.NewDiscardFilePutter()) + payload, err := asm.NewInputTarStream(io.TeeReader(uncompressed_reader, uncompressedWriter), metadata, storage.NewDiscardFilePutter()) if err != nil { return -1, err } diff --git a/image/internal/uploadreader/upload_reader.go b/storage/pkg/terminablereader/terminable_reader.go similarity index 77% rename from image/internal/uploadreader/upload_reader.go rename to storage/pkg/terminablereader/terminable_reader.go index b95370af76..82abb4deea 100644 --- a/image/internal/uploadreader/upload_reader.go +++ b/storage/pkg/terminablereader/terminable_reader.go @@ -1,11 +1,11 @@ -package uploadreader +package terminablereader import ( "io" "sync" ) -// UploadReader is a pass-through reader for use in sending non-trivial data using the net/http +// TerminableReader is a pass-through reader for use in sending non-trivial data using the net/http // package (http.NewRequest, http.Post and the like). // // The net/http package uses a separate goroutine to upload data to a HTTP connection, @@ -16,21 +16,21 @@ import ( // As a result, any data used/updated by the io.Reader() provided as the request body may be // used/updated even after http.Client.Do returns, causing races. // -// To fix this, UploadReader provides a synchronized Terminate() method, which can block for +// To fix this, TerminableReader provides a synchronized Terminate() method, which can block for // a not-completely-negligible time (for a duration of the underlying Read()), but guarantees that // after Terminate() returns, the underlying reader is never used any more (unlike calling // the cancellation callback of context.WithCancel, which returns before any recipients may have // reacted to the cancellation). -type UploadReader struct { +type TerminableReader struct { mutex sync.Mutex // The following members can only be used with mutex held reader io.Reader terminationError error // nil if not terminated yet } -// NewUploadReader returns an UploadReader for an "underlying" reader. -func NewUploadReader(underlying io.Reader) *UploadReader { - return &UploadReader{ +// NewTerminableReader returns an TerminableReader for an "underlying" reader. +func NewTerminableReader(underlying io.Reader) *TerminableReader { + return &TerminableReader{ reader: underlying, terminationError: nil, } @@ -38,7 +38,7 @@ func NewUploadReader(underlying io.Reader) *UploadReader { // Read returns the error set by Terminate, if any, or calls the underlying reader. // It is safe to call this from a different goroutine than Terminate. -func (ur *UploadReader) Read(p []byte) (int, error) { +func (ur *TerminableReader) Read(p []byte) (int, error) { ur.mutex.Lock() defer ur.mutex.Unlock() @@ -53,7 +53,7 @@ func (ur *UploadReader) Read(p []byte) (int, error) { // reader will never be used any more. // // It is safe to call this from a different goroutine than Read. -func (ur *UploadReader) Terminate(err error) { +func (ur *TerminableReader) Terminate(err error) { ur.mutex.Lock() // May block for some time if ur.reader.Read() is in progress defer ur.mutex.Unlock() diff --git a/image/internal/uploadreader/upload_reader_test.go b/storage/pkg/terminablereader/terminable_reader_test.go similarity index 80% rename from image/internal/uploadreader/upload_reader_test.go rename to storage/pkg/terminablereader/terminable_reader_test.go index c91967a15d..abf219920e 100644 --- a/image/internal/uploadreader/upload_reader_test.go +++ b/storage/pkg/terminablereader/terminable_reader_test.go @@ -1,4 +1,4 @@ -package uploadreader +package terminablereader import ( "bytes" @@ -10,18 +10,18 @@ import ( "github.com/stretchr/testify/require" ) -func TestUploadReader(t *testing.T) { +func TestTerminableReader(t *testing.T) { // This is a smoke test in a single goroutine, without really testing the locking. data := bytes.Repeat([]byte{0x01}, 65535) // No termination - ur := NewUploadReader(bytes.NewReader(data)) + ur := NewTerminableReader(bytes.NewReader(data)) read, err := io.ReadAll(ur) require.NoError(t, err) assert.Equal(t, data, read) // Terminated - ur = NewUploadReader(bytes.NewReader(data)) + ur = NewTerminableReader(bytes.NewReader(data)) readLen := len(data) / 2 read, err = io.ReadAll(io.LimitReader(ur, int64(readLen))) require.NoError(t, err)