From 280bef9a6e93de9c5152200492a94195e5161bd7 Mon Sep 17 00:00:00 2001 From: Sendya <18x@loacg.com> Date: Mon, 1 Jun 2026 10:28:48 +0800 Subject: [PATCH 1/2] fix: prevent double-close in partsReader by advancing index after Close MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In partsReader.Read, when Close() returns an error after EOF, r.index was not incremented — causing the same already-closed reader to be closed again by the outer Close() method. In partsReader.WriteTo, none of the three error-return paths incremented r.index after closing the reader, leading to the same double-close bug. Fix: increment r.index before returning in both error paths, so that a closed reader is always marked as consumed regardless of Close/io.Copy outcome. Closes #57 Co-Authored-By: Claude Opus 4.7 --- pkg/iobuf/part_reader.go | 2 + pkg/iobuf/part_reader_test.go | 165 ++++++++++++++++++++++++++++++++++ 2 files changed, 167 insertions(+) create mode 100644 pkg/iobuf/part_reader_test.go diff --git a/pkg/iobuf/part_reader.go b/pkg/iobuf/part_reader.go index 8ea20bc..58ce292 100644 --- a/pkg/iobuf/part_reader.go +++ b/pkg/iobuf/part_reader.go @@ -42,6 +42,7 @@ func (r *partsReader) Read(p []byte) (n int, err error) { // If a part response fails, next part responses should be stopped immediately; // otherwise, the client will receive bad file content. if closeErr := r.R[r.index].Close(); closeErr != nil { + r.index++ return size, closeErr } r.index++ @@ -68,6 +69,7 @@ func (r *partsReader) WriteTo(w io.Writer) (n int64, err error) { nn, err = io.Copy(w, reader) n += nn if err != nil { + r.index++ if closeErr := reader.Close(); closeErr != nil { return n, closeErr } diff --git a/pkg/iobuf/part_reader_test.go b/pkg/iobuf/part_reader_test.go new file mode 100644 index 0000000..c3e23f1 --- /dev/null +++ b/pkg/iobuf/part_reader_test.go @@ -0,0 +1,165 @@ +package iobuf_test + +import ( + "bytes" + "errors" + "io" + "strings" + "testing" + + "github.com/omalloc/tavern/pkg/iobuf" +) + +// trackedReader wraps an io.Reader and tracks Close calls. +type trackedReader struct { + reader io.Reader + closeCount *int + closeErr error +} + +func (r *trackedReader) Read(p []byte) (int, error) { + return r.reader.Read(p) +} + +func (r *trackedReader) Close() error { + (*r.closeCount)++ + return r.closeErr +} + +// eofCloseErrReader returns data + io.EOF in a single Read, and returns the given error on Close. +type eofCloseErrReader struct { + data string + readOnce bool + closeErr error + closeCount *int +} + +func (r *eofCloseErrReader) Read(p []byte) (int, error) { + if r.readOnce { + return 0, io.EOF + } + r.readOnce = true + n := copy(p, r.data) + if n == 0 { + return 0, io.EOF + } + return n, io.EOF +} + +func (r *eofCloseErrReader) Close() error { + (*r.closeCount)++ + return r.closeErr +} + +func TestPartsReader_Read_CloseErrorAdvancesIndex(t *testing.T) { + closeErr := errors.New("close failed") + + var r1Closes, r2Closes int + + r1 := &eofCloseErrReader{ + data: "hello", + closeCount: &r1Closes, + } + r2 := &eofCloseErrReader{ + data: "world", + closeErr: closeErr, + closeCount: &r2Closes, + } + + pr := iobuf.PartsReadCloser(nil, r1, r2) + + buf := make([]byte, 1024) + + // Read r1: returns "hello" + EOF → close r1, advance to r2, err=nil + _, err := pr.Read(buf) + if err != nil { + t.Fatalf("unexpected error on first read: %v", err) + } + + // Read r2: returns "world" + EOF → close r2 returns closeErr + _, err = pr.Read(buf) + if !errors.Is(err, closeErr) { + t.Fatalf("expected closeErr, got %v", err) + } + + if r1Closes != 1 { + t.Errorf("r1 was closed %d times, expected 1", r1Closes) + } + + // Now Close the outer reader — should NOT close r2 again (r.index was incremented after Close failure) + _ = pr.Close() + + if r2Closes != 1 { + t.Errorf("r2 was closed %d times (expected 1) — double-close detected", r2Closes) + } +} + +func TestPartsReader_WriteTo_ErrorAdvancesIndex(t *testing.T) { + readErr := errors.New("read error") + + var r1Closes, r2Closes int + + r1 := &trackedReader{ + reader: strings.NewReader("hello"), + closeCount: &r1Closes, + } + r2 := &trackedReader{ + reader: &errReader{err: readErr}, + closeCount: &r2Closes, + } + + pr := iobuf.PartsReadCloser(nil, r1, r2) + wt, ok := pr.(io.WriterTo) + if !ok { + t.Fatal("PartsReadCloser does not implement io.WriterTo") + } + + var buf bytes.Buffer + n, err := wt.WriteTo(&buf) + + if !errors.Is(err, readErr) { + t.Fatalf("expected readErr, got %v", err) + } + if n != 5 { + t.Fatalf("expected 5 bytes written, got %d", n) + } + if r2Closes != 1 { + t.Errorf("r2 was closed %d times on error, expected 1", r2Closes) + } + + // Now Close the outer reader — should NOT close r2 again + _ = pr.Close() + + if r2Closes != 1 { + t.Errorf("r2 was closed %d times after outer Close (expected 1) — double-close detected", r2Closes) + } +} + +func TestPartsReader_WriteTo_AllSuccess(t *testing.T) { + var r1Closes, r2Closes int + + r1 := &trackedReader{ + reader: strings.NewReader("hello"), + closeCount: &r1Closes, + } + r2 := &trackedReader{ + reader: strings.NewReader("world"), + closeCount: &r2Closes, + } + + pr := iobuf.PartsReadCloser(nil, r1, r2) + wt := pr.(io.WriterTo) + + var buf bytes.Buffer + n, err := wt.WriteTo(&buf) + + if err != io.EOF { + t.Fatalf("expected io.EOF after consuming all parts, got %v", err) + } + if n != 10 { + t.Fatalf("expected 10 bytes written, got %d", n) + } + if buf.String() != "helloworld" { + t.Fatalf("expected 'helloworld', got %q", buf.String()) + } +} From d9f9a2e7f84485907b63587d4dee5372141cfbe3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 1 Jun 2026 13:43:07 +0000 Subject: [PATCH 2/2] Fix partsReader WriteTo to close readers on success --- pkg/iobuf/part_reader.go | 13 +++++++------ pkg/iobuf/part_reader_test.go | 14 ++++++++++++++ 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/pkg/iobuf/part_reader.go b/pkg/iobuf/part_reader.go index 58ce292..083d2a3 100644 --- a/pkg/iobuf/part_reader.go +++ b/pkg/iobuf/part_reader.go @@ -68,18 +68,19 @@ func (r *partsReader) WriteTo(w io.Writer) (n int64, err error) { for _, reader := range rrs { nn, err = io.Copy(w, reader) n += nn - if err != nil { - r.index++ - if closeErr := reader.Close(); closeErr != nil { - return n, closeErr - } + r.index++ + if closeErr := reader.Close(); closeErr != nil { + return n, closeErr + } + + if err != nil { if err != io.EOF { return n, err } return } - r.index++ + if r.index == len(r.R) { err = io.EOF } diff --git a/pkg/iobuf/part_reader_test.go b/pkg/iobuf/part_reader_test.go index c3e23f1..c5170e9 100644 --- a/pkg/iobuf/part_reader_test.go +++ b/pkg/iobuf/part_reader_test.go @@ -162,4 +162,18 @@ func TestPartsReader_WriteTo_AllSuccess(t *testing.T) { if buf.String() != "helloworld" { t.Fatalf("expected 'helloworld', got %q", buf.String()) } + if r1Closes != 1 { + t.Fatalf("r1 was closed %d times, expected 1", r1Closes) + } + if r2Closes != 1 { + t.Fatalf("r2 was closed %d times, expected 1", r2Closes) + } + + _ = pr.Close() + if r1Closes != 1 { + t.Fatalf("r1 was closed %d times after outer Close, expected 1", r1Closes) + } + if r2Closes != 1 { + t.Fatalf("r2 was closed %d times after outer Close, expected 1", r2Closes) + } }