fix: prevent concatenated gzip response truncation when read via GZIPInputStream - #7331
jencymaryjoseph wants to merge 5 commits into
Conversation
|
|
||
| public ResponseInputStream(ResponseT resp, AbortableInputStream in, Duration timeout) { | ||
| super(in); | ||
| super(new GzipAvailabilityInputStream(in)); |
There was a problem hiding this comment.
Hmm I don't think this is the right approach; it would make more sense to push the decision of whether this is a GZIP inputstream further down the stack (i.e. closer to the transport/HTTP layer). For example, is there some way we can check the content-type header, and if it's GZIP, wrap the stream there?
Another reason this is problematic is what happens if somewhere down the line we need to make another "workaround" for some other type of content stream?
There was a problem hiding this comment.
Looked into this further - and I think transport/header gating cannot reliably fix this. S3 ContentType and ContentEncoding(when Content-Encoding: gzip is set, the SDK already decompresses) are user controlled and may not identify gzip. Async transports also expose a Publisher, so available() doesn't exist until the blocking stream adapter that is exactly where ResponseInputStream sits.
Should we keep ResponseInputStream generic by applying GzipAvailabilityInputStream through an internal factory at the sync and async blocking stream boundaries, while reserving ExecutionInterceptor (modifyHttpResponseContent) hooks for future header based workarounds.
|
Should we also gate the "always return 1 for GZIP" behavior on something like an Maybe something like |
|
Updated the fix so the GZIP aware wrapper is applied where the SDK exposes blocking response streams. For sync responses, Added a default-on |
| * Enabled by default. | ||
| */ | ||
| public static final SdkAdvancedClientOption<Boolean> CONCATENATED_GZIP_STREAM_SUPPORT_ENABLED = | ||
| new SdkAdvancedClientOption<>(Boolean.class); |
There was a problem hiding this comment.
I think this is the wrong name for this since we're not adding any meaningful support for concatenated gzip data streams; i.e. if you're using something other than GZIPInputStream to wrap the response stream, that doesn't incorrectly close on available() == 0, then there's no issue.
maybe add "compatibility" in the name, like GZIP_INPUTSTREAM_COMPATIBILITY_SUPPORT_ENABLED?
| "type": "bugfix", | ||
| "category": "AWS SDK for Java v2", | ||
| "contributor": "", | ||
| "description": "Fixed an issue where concatenated (multi-member) gzip response streams could be truncated to the first member when decoded with GZIPInputStream, because a transient available()==0 at a member boundary was treated as end of stream. The corrected behavior is enabled by default and can be disabled with SdkAdvancedClientOption.CONCATENATED_GZIP_STREAM_SUPPORT_ENABLED." |
There was a problem hiding this comment.
"The correct behavior is enabled by default" is ambiguous here; does it mean we do or don't return 0 from available()?
| SdkClientConfiguration config = mergeServiceDefaults(configuration); | ||
| config = config.merge(c -> c.option(AwsAdvancedClientOption.ENABLE_DEFAULT_REGION_DETECTION, true) | ||
| .option(SdkAdvancedClientOption.DISABLE_HOST_PREFIX_INJECTION, false) | ||
| .option(SdkAdvancedClientOption.CONCATENATED_GZIP_STREAM_SUPPORT_ENABLED, true) |
There was a problem hiding this comment.
We should do a surface API review for this change and discuss as a team, but I think this behavior should be disabled by default
|
|
||
| @Override | ||
| public AsyncResponseTransformer<ResponseT, ResponseInputStream<ResponseT>> | ||
| withConcatenatedGzipStreamSupportEnabled(boolean enabled) { |
There was a problem hiding this comment.
This is strangely named; withConcatenatedGzipStreamSupportEnabled sounds like the returned transformer has the GZIP behavior enabled always, but that's not always true and depends on the enabled config
| * around {@link java.util.zip.GZIPInputStream} truncating concatenated (multi-member) gzip at a member boundary. | ||
| * Non-gzip content passes through unchanged and the original stream's {@code abort()} is preserved. | ||
| */ | ||
| private static AbortableInputStream wrapForConcatenatedGzipSupport( |
There was a problem hiding this comment.
Same comment here, the name suggests the wrapping always happens but it depends on the config value
| * Configures the caller's transformer before an S3 decorator can capture it by calling {@code split()}. | ||
| */ | ||
| @SdkInternalApi | ||
| final class ConcatenatedGzipStreamSupportS3AsyncClient extends DelegatingS3AsyncClient { |
There was a problem hiding this comment.
Hmm looking at this makes me rethink my earlier suggestion of pushing the logic out of ResponseTransformer/AsyncResponseTransformer. It's probably worth revisiting; it would simplify things greatly if the configuration were just on the transformer themselves, especially for AsyncResponseTransformer.toFile() since there's nothing you can do at the lower layers to influence the behavior since the InputStream only gets created at the transformer layer.
E.g.
static <ResponseT> ResponseTransformer<ResponseT, ResponseInputStream<ResponseT>> toInputStream(Duration timeout,
boolean gzipInputStreamCompatibilitySupportEnabled) {
...There was a problem hiding this comment.
Added new blocking stream transformer overloads with a gzipInputStreamCompatibilityEnabled parameter. Passing true applies the GZIP compatibility wrapper; passing false preserves the stream’s normal available() behavior.
Sync now provides toInputStream(boolean) and toInputStream(Duration, boolean), while async provides toBlockingInputStream(boolean). Existing overloads behave as false, so compatibility remains opt-in. This removes the client option, handler propagation and S3 specific decorator logic
Motivation and Context
When an SDK streaming response body is gzip-compressed and the caller decodes it with
java.util.zip.GZIPInputStream, a concatenated (multi-member) gzip stream can be silently truncated toonly its first member.
Root cause is in the JDK:
GZIPInputStream.readTrailer()decides whether another gzip member follows partlyfrom the underlying stream's
available(). A network-backed response body can transiently reportavailable() == 0exactly at a member boundary (the socket momentarily has no buffered bytes even though moredata is still in flight).
GZIPInputStreamtreats that transient0as end-of-stream, stops decoding, anddrops every remaining member — no error is raised, so the caller just receives partial data.
Modifications
GzipAvailabilityInputStream(@SdkInternalApi,core/sdk-core):1f 8b+ DEFLATE method08).available()as at least1while the stream is open and not atEOF, so
GZIPInputStreamkeeps reading across member boundaries instead of stopping on a transient0.available()through unchanged, so text/line consumers (e.g.BufferedReader) areunaffected.
release()to aReleasabledelegate, and preservesread/skip/mark/reset/closesemantics.ResponseTransformer.toInputStream(boolean)ResponseTransformer.toInputStream(Duration, boolean)AsyncResponseTransformer.toBlockingInputStream(boolean)The boolean parameter controls
GZIPInputStreamcompatibility. Passingtrueapplies the compatibility wrapper;passing
falsepreserves the response stream's normalavailable()behavior. Existing overloads behave asfalse, so the change is disabled by default and does not alter existing callers.For sync requests, the wrapper is applied by
ResponseTransformerwhen it creates the blockingResponseInputStream. For async requests,InputStreamResponseTransformerapplies it when adapting the responsepublisher into a blocking stream. Keeping the setting on the transformer also allows it to survive async
split()processing without client-option propagation or S3-specific decorators.Testing
GzipAvailabilityInputStreamTestcovers gzip detection, non-gzip pass-through, transient zero availability,concatenated and many-member decoding, EOF and close behavior, blocking EOF, skip, mark/reset, release, and
abort propagation.
ResponseTransformerToInputStreamTestcovers default-off and opt-in behavior, timeout overloads, non-gzipstreams, abort propagation, and concatenated gzip decoding.
InputStreamResponseTransformerTestcovers default-off and opt-in behavior, non-gzip streams, abortcancellation, and preservation of compatibility through async
split().GetObjectConcatenatedGzipTestverifies opt-in sync and async S3 downloads through the HTTP stack.SDK Core, AWS Core, and S3 unit and integration tests pass.
Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License