johannesboyne / gofakes3

A simple fake AWS S3 object storage (used for local test-runs against AWS S3 APIs)
MIT License
358 stars 81 forks source link

Data race: lock on abort/complete of multipart upload #90

Closed rodaine closed 4 months ago

rodaine commented 5 months ago

Hi, Thanks for this great module! Ran into a data race while interacting concurrently with the multipart capabilities. It appears that the uploader lock is not consistently obtained when completing or aborting a multipart upload, which ends up racing on bucket access. This patch now obtains the lock for these two methods. Data race output below for reference:

Data race trace ``` ================== WARNING: DATA RACE Read at 0x00c003823a10 by goroutine 601: runtime.mapaccess1_faststr() ~/.gimme/versions/go1.22.1.darwin.arm64/src/runtime/map_faststr.go:13 +0x40c github.com/johannesboyne/gofakes3.(*uploader).getUnlocked() ~/pkg/mod/github.com/johannesboyne/gofakes3@v0.0.0-20240217095638-c55a48f17be6/uploader.go:467 +0x124 github.com/johannesboyne/gofakes3.(*uploader).UploadPart() ~/pkg/mod/github.com/johannesboyne/gofakes3@v0.0.0-20240217095638-c55a48f17be6/uploader.go:380 +0xf4 github.com/johannesboyne/gofakes3.(*GoFakeS3).putMultipartUploadPart() ~/pkg/mod/github.com/johannesboyne/gofakes3@v0.0.0-20240217095638-c55a48f17be6/gofakes3.go:901 +0x5f0 github.com/johannesboyne/gofakes3.(*GoFakeS3).routeMultipartUpload() ~/pkg/mod/github.com/johannesboyne/gofakes3@v0.0.0-20240217095638-c55a48f17be6/routing.go:180 +0x108 github.com/johannesboyne/gofakes3.(*GoFakeS3).routeBase() ~/pkg/mod/github.com/johannesboyne/gofakes3@v0.0.0-20240217095638-c55a48f17be6/routing.go:43 +0x4cc github.com/johannesboyne/gofakes3.(*GoFakeS3).routeBase-fm() :1 +0x4c net/http.HandlerFunc.ServeHTTP() ~/.gimme/versions/go1.22.1.darwin.arm64/src/net/http/server.go:2166 +0x48 github.com/johannesboyne/gofakes3.(*withCORS).ServeHTTP() ~/pkg/mod/github.com/johannesboyne/gofakes3@v0.0.0-20240217095638-c55a48f17be6/cors.go:46 +0x51c github.com/johannesboyne/gofakes3.(*GoFakeS3).Server.(*GoFakeS3).timeSkewMiddleware.func1() ~/pkg/mod/github.com/johannesboyne/gofakes3@v0.0.0-20240217095638-c55a48f17be6/gofakes3.go:111 +0x18c net/http.HandlerFunc.ServeHTTP() ~/.gimme/versions/go1.22.1.darwin.arm64/src/net/http/server.go:2166 +0x48 net/http.serverHandler.ServeHTTP() ~/.gimme/versions/go1.22.1.darwin.arm64/src/net/http/server.go:3137 +0x298 net/http.(*conn).serve() ~/.gimme/versions/go1.22.1.darwin.arm64/src/net/http/server.go:2039 +0xf28 net/http.(*Server).Serve.gowrap3() ~/.gimme/versions/go1.22.1.darwin.arm64/src/net/http/server.go:3285 +0x4c Previous write at 0x00c003823a10 by goroutine 710: runtime.mapassign_faststr() ~/.gimme/versions/go1.22.1.darwin.arm64/src/runtime/map_faststr.go:203 +0x41c github.com/johannesboyne/gofakes3.(*bucketUploads).remove() ~/pkg/mod/github.com/johannesboyne/gofakes3@v0.0.0-20240217095638-c55a48f17be6/uploader.go:103 +0x94 github.com/johannesboyne/gofakes3.(*uploader).AbortMultipartUpload() ~/pkg/mod/github.com/johannesboyne/gofakes3@v0.0.0-20240217095638-c55a48f17be6/uploader.go:364 +0x27c github.com/johannesboyne/gofakes3.(*GoFakeS3).abortMultipartUpload() ~/pkg/mod/github.com/johannesboyne/gofakes3@v0.0.0-20240217095638-c55a48f17be6/gofakes3.go:912 +0x218 github.com/johannesboyne/gofakes3.(*GoFakeS3).routeMultipartUpload() ~/pkg/mod/github.com/johannesboyne/gofakes3@v0.0.0-20240217095638-c55a48f17be6/routing.go:182 +0x1e0 github.com/johannesboyne/gofakes3.(*GoFakeS3).routeBase() ~/pkg/mod/github.com/johannesboyne/gofakes3@v0.0.0-20240217095638-c55a48f17be6/routing.go:43 +0x4cc github.com/johannesboyne/gofakes3.(*GoFakeS3).routeBase-fm() :1 +0x4c net/http.HandlerFunc.ServeHTTP() ~/.gimme/versions/go1.22.1.darwin.arm64/src/net/http/server.go:2166 +0x48 github.com/johannesboyne/gofakes3.(*withCORS).ServeHTTP() ~/pkg/mod/github.com/johannesboyne/gofakes3@v0.0.0-20240217095638-c55a48f17be6/cors.go:46 +0x51c github.com/johannesboyne/gofakes3.(*GoFakeS3).Server.(*GoFakeS3).timeSkewMiddleware.func1() ~/pkg/mod/github.com/johannesboyne/gofakes3@v0.0.0-20240217095638-c55a48f17be6/gofakes3.go:111 +0x18c net/http.HandlerFunc.ServeHTTP() ~/.gimme/versions/go1.22.1.darwin.arm64/src/net/http/server.go:2166 +0x48 net/http.serverHandler.ServeHTTP() ~/.gimme/versions/go1.22.1.darwin.arm64/src/net/http/server.go:3137 +0x298 net/http.(*conn).serve() ~/.gimme/versions/go1.22.1.darwin.arm64/src/net/http/server.go:2039 +0xf28 net/http.(*Server).Serve.gowrap3() ~/.gimme/versions/go1.22.1.darwin.arm64/src/net/http/server.go:3285 +0x4c Goroutine 601 (running) created at: net/http.(*Server).Serve() ~/.gimme/versions/go1.22.1.darwin.arm64/src/net/http/server.go:3285 +0x674 net/http/httptest.(*Server).goServe.func1() ~/.gimme/versions/go1.22.1.darwin.arm64/src/net/http/httptest/server.go:310 +0xb0 Goroutine 710 (running) created at: net/http.(*Server).Serve() ~/.gimme/versions/go1.22.1.darwin.arm64/src/net/http/server.go:3285 +0x674 net/http/httptest.(*Server).goServe.func1() ~/.gimme/versions/go1.22.1.darwin.arm64/src/net/http/httptest/server.go:310 +0xb0 ================== ```
sonarcloud[bot] commented 5 months ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

codecov-commenter commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 65.99%. Comparing base (c55a48f) to head (e4a373f).

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #90 +/- ## ========================================== + Coverage 65.94% 65.99% +0.05% ========================================== Files 28 28 Lines 2693 2697 +4 ========================================== + Hits 1776 1780 +4 Misses 651 651 Partials 266 266 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.