Skip to content

s3/transfermanager: fix panic in concurrentReader when len(p) is smaller than cap(p) - #3388

Open
islishude wants to merge 3 commits into
aws:mainfrom
islishude:fix-s3-tm
Open

s3/transfermanager: fix panic in concurrentReader when len(p) is smaller than cap(p)#3388
islishude wants to merge 3 commits into
aws:mainfrom
islishude:fix-s3-tm

Conversation

@islishude

Copy link
Copy Markdown
Contributor

Summary

This fixes a panic in concurrentReader.read when the caller passes a slice where len(p) < cap(p).

The reader was using cap(p) to:

  • detect empty input
  • calculate the max part index to process
  • decide whether p[index:] was safe to slice

That is not safe for an io.Reader, because reads must be bounded by len(p), not cap(p).

Read reads up to len(p) bytes into p. It returns the number of bytes read (0 <= n <= len(p)) and any error encountered.

In this case, the old logic could treat an offset as valid and then panic with a slice-bounds error when evaluating p[index:].

Changes

  • replace cap(p) with len(p) in concurrentReader.read
  • keep part selection and direct chunk writes aligned with the actual readable window
  • add regression coverage for both paths that previously panicked:
    • reading from a buffered chunk
    • reading from a newly received chunk

Testing

  • added TestConcurrentReaderReadUsesSliceLenForBounds
  • ran the concurrent reader test file successfully

@islishude
islishude requested a review from a team as a code owner April 16, 2026 11:58
Copilot AI review requested due to automatic review settings April 16, 2026 11:58

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a slice-bounds panic in feature/s3/transfermanager’s concurrentReader.read by ensuring all read-window calculations and slicing are bounded by len(p) (per io.Reader contract), not cap(p).

Changes:

  • Replace cap(p) with len(p) for empty-buffer detection, max part selection, and safe slicing in concurrentReader.read.
  • Add a regression test covering both previously-panicking paths: reading from an already-buffered chunk and from a newly received chunk.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
feature/s3/transfermanager/concurrent_reader.go Bounds all indexing/slicing in read to len(p) to prevent panics with short slices.
feature/s3/transfermanager/concurrent_reader_test.go Adds regression coverage to ensure short len(p) with larger cap(p) does not panic and does not advance buffered chunk state.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread feature/s3/transfermanager/concurrent_reader_test.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants