Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix content corruption in a chunked response on HTTP/1 over TLS #3894

Merged
merged 1 commit into from Oct 25, 2021

Conversation

trustin
Copy link
Member

@trustin trustin commented Oct 25, 2021

Motivation:

There's a chance of HTTP content corruption when:

  • The current connection is HTTP/1 over TLS;
  • The current response is using chunked encoding; and
  • The content length is greater than 16378 (16384 - 6).

.. due to a bug in Netty: netty/netty#11792

Modifications:

  • Fixed the math mistake in Http1ObjectEncoder.MAX_TLS_DATA_LENGTH,
    to prevent HttpObjectEncoder from putting its static final ByteBuf
    in the first place of SslHandler's internal queue.

Result:

  • No more HTTP content corruption.

Motivation:

There's a chance of HTTP content corruption when:

- The current connection is TLS;
- The current response is using chunked encoding; and
- The content length is greater than 16378 (16384 - 6).

.. due to a bug in Netty: netty/netty#11792

Modifications:

- Fixed the math mistake in `Http1ObjectEncoder.MAX_TLS_DATA_LENGTH`,
  to prevent `HttpObjectEncoder` from putting its static final `ByteBuf`
  in the first place of `SslHandler`'s internal queue.

Result:

- No more HTTP content conrruption.
@trustin trustin added the defect label Oct 25, 2021
@trustin trustin added this to the 1.14.0 milestone Oct 25, 2021
@trustin trustin modified the milestones: 1.14.0, 1.13.2 Oct 25, 2021
@trustin trustin changed the title Fix content corruption in a chunked response on TLS Fix content corruption in a chunked response on HTTP/1 over TLS Oct 25, 2021
@codecov
Copy link

codecov bot commented Oct 25, 2021

Codecov Report

Merging #3894 (03f3da8) into master (25f810c) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3894      +/-   ##
============================================
+ Coverage     73.26%   73.27%   +0.01%     
- Complexity    15528    15529       +1     
============================================
  Files          1365     1365              
  Lines         59831    59831              
  Branches       7582     7582              
============================================
+ Hits          43833    43839       +6     
+ Misses        12151    12145       -6     
  Partials       3847     3847              
Impacted Files Coverage Δ
...rp/armeria/internal/common/Http1ObjectEncoder.java 70.94% <ø> (ø)
...nternal/common/AbstractHttp2ConnectionHandler.java 82.69% <0.00%> (-5.77%) ⬇️
...meria/common/stream/RegularFixedStreamMessage.java 82.89% <0.00%> (-2.64%) ⬇️
...rmeria/common/stream/ConcatArrayStreamMessage.java 77.77% <0.00%> (-2.23%) ⬇️
...p/armeria/common/stream/AbstractStreamMessage.java 79.20% <0.00%> (-1.99%) ⬇️
.../com/linecorp/armeria/server/RoutingPredicate.java 70.96% <0.00%> (-1.62%) ⬇️
...ia/common/stream/ConcatPublisherStreamMessage.java 80.62% <0.00%> (-0.78%) ⬇️
...ia/internal/common/stream/ByteBufDecoderInput.java 87.05% <0.00%> (+0.71%) ⬆️
...p/armeria/internal/common/eureka/InstanceInfo.java 52.63% <0.00%> (+0.87%) ⬆️
...necorp/armeria/client/HeapBasedEventLoopState.java 93.81% <0.00%> (+1.03%) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 25f810c...03f3da8. Read the comment docs.

Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Thanks, @trustin and @alexc-db!

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Thanks, @trustin and @alexc-db for thorough analayis! 🙇‍♂️

@trustin trustin merged commit 37d7eaa into line:master Oct 25, 2021
@trustin trustin deleted the jetty_corruption branch October 25, 2021 16:22
if (cause != null) {
caughtExceptions.add(cause);
}
try {
Copy link
Member Author

Choose a reason for hiding this comment

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

This has to be guarded by an else block. Let me push a fix commit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants