Repository navigation
Fix read-and-copy overflow and trim per-chunk work in ChunkedUploadWrapperStream - #4570
Closed
martincostello wants to merge 1 commit into
Closed
martincostello wants to merge 1 commit into
martincostello wants to merge 1 commit into
Conversation
…apperStream The read-and-copy fill loops (used when the wrapped stream encrypts) always asked the wrapped stream for a full wrapped-stream-buffer-size read, even when fewer bytes fit in the remaining space of the chunk buffer. With a BufferSize that does not divide DefaultChunkSize (or exceeds it) the Buffer.BlockCopy into the chunk buffer throws mid-upload. Each read is now clamped to the space left in the chunk, and the interim read buffer is allocated once per stream, sized to at most a chunk, instead of once per fill. Per-chunk allocations are reduced at the same time: - The short final chunk is no longer copied into an exact-size array. SigV4 hashes only the bytes read through a new internal string-to-sign overload that takes an offset and count (the public overload keeps its contract and delegates to it), and SigV4a signs a MemoryStream slice. - The chunk header is encoded straight into the output buffer instead of going through a StringBuilder, a byte[] header and a byte[] trailer per chunk. - The SigV4 signing key is captured once per stream instead of being cloned by GetSigningKey() for every chunk and again for the trailer. - The empty-body hash in the string to sign uses the existing constant. Tests cover a multi-chunk payload round-tripping with every chunk signature (including the terminating chunk) recomputed independently from exact-size copies via the public string-to-sign helper, and an encrypting stream read with a buffer larger than a chunk, which fails with an ArgumentException before this change.
martincostello
marked this pull request as ready for review
October 27, 2026 11:08
martincostello
requested review from
dscpinheiro and
siddsriv
and
a balanced review from Copilot
and removed request for
a team
October 27, 2026 11:08
8 of 16 tasks
dscpinheiro
requested review from
boblodgett
and removed request for
siddsriv
October 27, 2026 15:30
jnunn-aws
self-requested a review
October 27, 2026 17:58
Contributor
|
Closing this PR in favor of this one we had been working on: #4533 |
Contributor
Author
|
There were some bits in this PR that weren't included in that one - I'll look to submit a new one with just the gaps on top. |
Contributor
Author
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Split out of #4566 as requested; this is the part of the chunked-upload work that is not covered by #4533 (which sizes the chunk buffers to the payload and is left untouched here).
Bug fix. The read-and-copy fill loops in
ChunkedUploadWrapperStream(used when the wrapped stream encrypts) always asked the wrapped stream for a fullBufferSizeread, regardless of how much room was left in the chunk buffer. With aBufferSizethat does not divideDefaultChunkSize(81920), or exceeds it, theBuffer.BlockCopyinto the chunk buffer throwsArgumentExceptionmid-upload. Each read is now clamped to the space left in the chunk, and the interim read buffer is allocated once per stream (at most a chunk in size) instead of once per fill.Per-chunk work trimmed at the same time:
BuildChunkedStringToSignkeeps its contract and delegates to it. SigV4a signs aMemoryStreamslice.StringBuilder, abyte[]header and abyte[]trailer per chunk.GetSigningKey()for every chunk and again for the trailer.AWS4Signer.EmptyBodySha256constant instead of hashing and hex-encoding an empty string per chunk.Motivation and Context
Measured with a mocked-HTTP harness (no network) that drives a full
PutObjectthrough the pipeline and consumes the request body: branch vsdevelopment(9ab8dcf), three interleaved baseline/branch rounds of 20 × 2000 operations after warm-up, process pinned to the P-cores of an i7-13700H, .NET 10 runtime against thenet8.0build of the SDK. Timings are the best round per side; allocations are exact (GC.GetAllocatedBytesForCurrentThread) and identical across rounds. Ratios are branch ÷ baseline.The 300 KB upload saves 66 KB per request (the exact-size copy of the 54 KB final chunk, the per-chunk header and trailer arrays, and the signing-key clones); its time is dominated by hashing the body, so the time ratio moves little. Both rows still include the ~160 KB of full-size chunk buffers that #4533 addresses, which is why the small-upload allocation ratio is close to 1. The overflow fix does not show in the numbers; it is covered by the new unit test, which fails on
developmentwithArgumentException: Offset and length were out of bounds for the array....Benchmark code
The harness is not checked in. It is a console project that references the SDK projects directly, replaces the HTTP client with a canned-response handler that consumes the request body (so the aws-chunked signing stream really runs), and measures a full client call through the pipeline. Build it once from the default branch and copy
bin/Release/net10.0toCoreBench-base, then build it from the branch and runab.sh "put putlarge" 3.CoreBench.csprojBenchmarks.csMockHttp.csProgram.csab.shTesting
AWSSDK.Core.NetStandard.csprojbuilds for all targets with no warnings.AWSSDK.UnitTests.Coreonnet472, filtered toChunkedUploadWrapperStreamandAWS4Signer: 41 passed. The existing chunked-upload tests pin the emitted content byte for byte, so the header rewrite and the final-chunk change are verified against the previous output.net8.0test target compiles but could not be executed locally (MSTest adapter/framework mismatch in this environment), so CI is the first run of that target.ChunkedUploadWrapperStreamTests: a multi-chunk payload (DefaultChunkSize + 12345bytes) round-trips with the expected chunk sizes and with every chunk signature, including the terminating chunk, recomputed independently from exact-size copies via the public string-to-sign helper; and an AES-encrypting wrapped stream read with aBufferSizeof twice the chunk size, which overflows before this change.Dry-runs
Breaking Changes Assessment
BuildChunkedStringToSign(string, string, string, string, int, byte[])keeps hashing the whole buffer whendataLength > 0, exactly as before; only the new internal overload hashes a range.Types of changes
Checklist
License
This change was generated by AI tools (Claude Code), and reviewed by Martin Costello (@martincostello).
🤖 Generated with Claude Code