From dd3f24deef1da119c5030750e1a508d7d389fb4a Mon Sep 17 00:00:00 2001 From: oliverpool Date: Thu, 17 Jul 2025 12:31:38 +0200 Subject: [PATCH] fix: storage(minio): prevent io.Reader close (#8541) Fixes #8529, reverts #8527. I was able to reproduce the problem in a test: - it triggered only when the reader was an io.Reader - and the size was provided (-1 takes another code path in minio) https://codeberg.org/oliverpool/forgejo/commit/287b1f21e1e5fc3513fcca4eb21ee4607520813c should fail when running: ``` docker run --rm -e MINIO_DOMAIN=minio -e MINIO_ROOT_USER=123456 -e MINIO_ROOT_PASSWORD=12345678 -p 9000:9000 data.forgejo.org/oci/bitnami/minio:2024.8.17 ``` and ``` TEST_MINIO_ENDPOINT=localhost:9000 go test -v -run ^TestMinioStorageIterator$ ./modules/storage ``` ### Tests - I added test coverage for Go changes... - [x] in their respective `*_test.go` for unit tests. - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server. ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [x] I do not want this change to show in the release notes. - [ ] I want the title to show in the release notes with a link to this pull request. - [ ] I want the content of the `release-notes/.md` to be be used for the release notes instead of the title. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8541 Reviewed-by: Michael Kriese Reviewed-by: Earl Warren Co-authored-by: oliverpool Co-committed-by: oliverpool --- modules/storage/minio.go | 2 +- modules/storage/storage_test.go | 36 +++++++++++++++------ routers/api/packages/container/container.go | 16 ++++++--- 3 files changed, 39 insertions(+), 15 deletions(-) diff --git a/modules/storage/minio.go b/modules/storage/minio.go index bf51a1642a..424000a0d8 100644 --- a/modules/storage/minio.go +++ b/modules/storage/minio.go @@ -209,7 +209,7 @@ func (m *MinioStorage) Save(path string, r io.Reader, size int64) (int64, error) m.ctx, m.bucket, m.buildMinioPath(path), - r, + io.NopCloser(r), // prevent minio from closing the reader size, minio.PutObjectOptions{ ContentType: "application/octet-stream", diff --git a/modules/storage/storage_test.go b/modules/storage/storage_test.go index af3dd9520e..76589d941a 100644 --- a/modules/storage/storage_test.go +++ b/modules/storage/storage_test.go @@ -5,6 +5,7 @@ package storage import ( "bytes" + "io" "testing" "forgejo.org/modules/setting" @@ -13,22 +14,39 @@ import ( "github.com/stretchr/testify/require" ) +type spyCloser struct { + io.Reader + closed int +} + +func (s *spyCloser) Close() error { + s.closed++ + return nil +} + +var _ io.ReadCloser = &spyCloser{} + func testStorageIterator(t *testing.T, typStr Type, cfg *setting.Storage) { l, err := NewStorage(typStr, cfg) require.NoError(t, err) - testFiles := [][]string{ - {"a/1.txt", "a1"}, - {"/a/1.txt", "aa1"}, // same as above, but with leading slash that will be trim - {"ab/1.txt", "ab1"}, - {"b/1.txt", "b1"}, - {"b/2.txt", "b2"}, - {"b/3.txt", "b3"}, - {"b/x 4.txt", "bx4"}, + testFiles := []struct { + path, content string + size int64 + }{ + {"a/1.txt", "a1", -1}, + {"/a/1.txt", "aa1", -1}, // same as above, but with leading slash that will be trim + {"ab/1.txt", "ab1", 3}, + {"b/1.txt", "b1", 2}, // minio closes when the size is set + {"b/2.txt", "b2", -1}, + {"b/3.txt", "b3", -1}, + {"b/x 4.txt", "bx4", -1}, } for _, f := range testFiles { - _, err = l.Save(f[0], bytes.NewBufferString(f[1]), -1) + sc := &spyCloser{bytes.NewBufferString(f.content), 0} + _, err = l.Save(f.path, sc, f.size) require.NoError(t, err) + assert.Equal(t, 0, sc.closed) } expectedList := map[string][]string{ diff --git a/routers/api/packages/container/container.go b/routers/api/packages/container/container.go index 9a7fd03aa8..117fc6ab88 100644 --- a/routers/api/packages/container/container.go +++ b/routers/api/packages/container/container.go @@ -399,7 +399,12 @@ func EndUploadBlob(ctx *context.Context) { } return } - defer uploader.Close() + doClose := true + defer func() { + if doClose { + uploader.Close() + } + }() if ctx.Req.Body != nil { if err := uploader.Append(ctx, ctx.Req.Body); err != nil { @@ -432,10 +437,11 @@ func EndUploadBlob(ctx *context.Context) { return } - // There was a strange bug: the "Close" fails with error "close .../tmp/package-upload/....: file already closed" - // AFAIK there should be no other "Close" call to the uploader between NewBlobUploader and this line. - // At least it's safe to call Close twice, so ignore the error. - _ = uploader.Close() + doClose = false + if err := uploader.Close(); err != nil { + apiError(ctx, http.StatusInternalServerError, err) + return + } if err := container_service.RemoveBlobUploadByID(ctx, uploader.ID); err != nil { apiError(ctx, http.StatusInternalServerError, err)